-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor(api-key): remove unsupported api-key feature #1148
Conversation
1cdc2c6
to
c8efd48
Compare
export const AUTHORIZATION_HEADER = 'Authorization'; | ||
/** @hidden */ | ||
export const API_KEY_HEADER = 'api-key'; | ||
export const AUTHORIZATION_HEADER = 'authorization'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like this change should break things but i'm not sure if we add both versions of the authorization header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests passed and the api shoudn’t care about the casing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or what are you worried about will break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the api shoudn’t care about the casing
I'm worried about this part. If we're using the same constant everywhere in the repo (which we probably do), i'm not surprised tests are working fine. Do we have a case that we pass this header manually from the outside of the sdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is not a blocker though, just nitpicking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check how easy it is to undo the change. But we run integration tests so the API accepts the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then don't worry too much about it, we would be testing this sdk version anyway (and it would be almost instant to figure it out)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place in fusion
repo that we manually pass the authorization header: https://github.com/cognitedata/fusion/blob/a3b007c8255cb0475878f6767e95a658ba8c34d9/libs/shared/cogidp-sdk/src/lib/client.ts#L226-L228. But we're not using js sdk there so should be good imo
As part of the v10 release, we would like to clean up api-key references in the SDK as api-keys isn't supported by CDF anymore (and hasn't been supported for a long time - years).
Jira: https://cognitedata.atlassian.net/browse/AUTH-3104