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

Use OAUTH token if set for analytics #2159

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

maxjeffos
Copy link
Contributor

What does this PR do?

Update Analytics to use the OAUTH token if it is set.

@maxjeffos maxjeffos requested review from a team as code owners August 11, 2021 20:50
headers: {
host: 'localhost:12345',
accept: 'application/json',
authorization: 'Bearer oauth-jwt-token',
Copy link

Choose a reason for hiding this comment

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

We can be less verbose in this test case and just match on the authorization header since that's all we care about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

expect(code).toBe(0);

const lastRequest = server.popRequest();
expect(lastRequest).toMatchObject({
Copy link

Choose a reason for hiding this comment

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

Could do something like not.toHaveProperty('headers.authorization') to be less verbose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

const oauthToken: string | undefined = getOAuthToken();
const dockerToken: string | undefined = getDockerToken();
const apiToken: string | undefined = api();
return Boolean(oauthToken || dockerToken || apiToken);
Copy link

Choose a reason for hiding this comment

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

We can inline these variables. I don't think naming them adds much, makes it more noisy (esp. with type annotations). And maybe check for api() first as it's the most common.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@maxjeffos maxjeffos force-pushed the fix/use-oauth-token-for-analytics branch 3 times, most recently from e105239 to 210c44c Compare August 12, 2021 14:08
@maxjeffos maxjeffos force-pushed the fix/use-oauth-token-for-analytics branch from 210c44c to 14a0b76 Compare August 12, 2021 17:09
@maxjeffos maxjeffos merged commit bf56735 into master Aug 12, 2021
@maxjeffos maxjeffos deleted the fix/use-oauth-token-for-analytics branch August 12, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant