-
-
Notifications
You must be signed in to change notification settings - Fork 15
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:(GitHub Auth): Deprecate getAuthUsername #585
base: master
Are you sure you want to change the base?
Conversation
if (username && apiToken) { | ||
this.setAuth(username, apiToken); | ||
} |
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.
should be able to remove these parameters entirely -- or maybe take the api key and do the header modification here?
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.
parameters removed, but I'm not sure how to bake the token and header in there
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.
I'm happy with this change as long as it does not require people to create a GitHub app (think other people using Craft).
That said I think the registry
target is quite specific to Sentry and I'm guessing this mostly affects that and cloning in Lambda?
* | ||
* @returns GitHub auth header | ||
*/ | ||
export function getGitHubAuthHeader(): Array<string> { |
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.
I found something arguably easier and more like the old ways here: https://stackoverflow.com/a/66156992/90297
git clone https://oauth2:[email protected]/username/repo.git
So, maybe just do that to keep things simple and hopefully more compatible?
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.
afaict both ways work with user tokens or app tokens -- fun fact the username does not matter for the clone-url approach just that it cannot be empty
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.
Wowza, alright then let's go with the old way as it seems simpler?
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.
* | ||
* @returns GitHub auth header | ||
*/ | ||
export function getGitHubAuthHeader(): Array<string> { |
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.
afaict both ways work with user tokens or app tokens -- fun fact the username does not matter for the clone-url approach just that it cannot be empty
Currently we are using
https://username:token@github/owner/repo
to perform auth will talking to GitHub, it does not work well with GitHub Apps, hence deprecating it and bake the authentication token into authentication headers.