Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [sc-105230] particle-api-js getEventStream does not honor setContext / X-Particle-Tool http header correctly #147

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

joegoggins
Copy link
Contributor

@joegoggins joegoggins commented Jun 24, 2022

Description

Fixes bug sc-105230 by passing X-Particle-Tool or X-Particle-Project http headers accordingly to event stream http requests that honor particleAPI.setContext('tool|project'). This work makes it so that Delorean CLI telemetry will be accurate when event stream consuming commands are introduced.

Also, eliminates problem where PARTICLE_API_TOKEN had to be specified for many npm run commands by moving *.js files that require these to an e2e-tests that does NOT run in CI. This is not a step down in terms of quality; as these tests were not designed to be run in this CI context, but rather manually by developers working on the event stream.

How to test?

Get the branch setup: git checkout, cd into repo, nvm use, npm i 👍

  1. npm run compile
  2. npm run test:ci; observe no failures due to PARTICLE_API_TOKEN not being set
  3. Put this foo.js snippet in the parent dir where your git checkout is
const ParticleAPI = require('.');

async function main() {
    const api = new ParticleAPI();
    api.setDefaultAuth('TODO');
    api.setContext('tool', {name: 'yoyo', version: '6.6.6'});
    const stream = await api.getEventStream({deviceId: 'mine'});
    console.log(stream);
}
main();
  1. Populate TODO with valid token and run node foo.js
  2. In the debug output, observe _headers has [email protected] in it (thus proving http headers are now honored)

Screen Shot 2022-06-24 at 6 08 41 PM

Joe Goggins added 9 commits June 24, 2022 17:06
…e-Project http headers

get set as we'd expect when using setContext('tool') or setContext('project')
Specifically, this change makes this linter error go away:

    /Users/me/git/particle-api-js/test/Particle.spec.js:941:78: Parsing error: Unexpected token => [Error]`
This makes it so that PARTICLE_API_TOKEN and PARTICLE_API_BASE_URL don't
have to be specified for `npm run test:unit` and other npm run commands
@joegoggins joegoggins force-pushed the sc-105230/event-stream-http-header-bugfix branch from 586d595 to fbf6100 Compare June 24, 2022 23:00
@joegoggins joegoggins marked this pull request as ready for review June 24, 2022 23:16
@joegoggins joegoggins requested review from deleonn and busticated June 24, 2022 23:16
Copy link
Contributor

@busticated busticated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good - thanks for doing battle 🙏 🙌

prior to merging, let's remove the env vars over in CircleCI to verify we do not have any lingering usages 👍

Screen Shot 2022-06-28 at 3 39 38 PM

Comment on lines -37 to -38
The `Agent` integration tests ([source](./test/Agent.integration.js)) depend on a real HTTP api backend and a valid Particle access token. Be sure to set relevant environment variables to avoid test failures. You can prefix commands test commands like this `PARTICLE_API_BASE_URL=<url> PARTICLE_API_TOKEN=<token> npm test`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joegoggins

Also, eliminates problem where PARTICLE_API_TOKEN had to be specified for many npm run commands by moving *.js files that require these to an e2e-tests that does NOT run in CI. This is not a step down in terms of quality; as these tests were not designed to be run in this CI context, but rather manually by developers working on the event stream.

fwiw, @deleonn updated CI over in #142 to ensure at least the node-side event e2e tests ran with their required perquisites - here's an example of them running and "passing":

https://app.circleci.com/pipelines/github/particle-iot/particle-api-js/53/workflows/cd0aa933-14fd-4836-a96b-024f13e31457/jobs/156?invite=true#step-103-42

further complicating things, the reference to "Agent integration tests" in these docs is incorrect - it's actually the test/EventStream-e2e-node.js file which is running in CI.

all that said, at this point i'd just delete all of those (e2e-tests directory and related) as they're likely to cause more confusion than help.

@joegoggins
Copy link
Contributor Author

Cool. I'll remove circle ci vars, nuke e2e-tests, merge, and cut a release. Thanks. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants