-
Notifications
You must be signed in to change notification settings - Fork 173
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
copilot: Add an extension point for credentials resolution #1260
base: main
Are you sure you want to change the base?
copilot: Add an extension point for credentials resolution #1260
Conversation
Changed Packages
|
Signed-off-by: Scott Guymer <[email protected]>
b51477b
to
983085d
Compare
Signed-off-by: Scott Guymer <[email protected]>
Signed-off-by: Scott Guymer <[email protected]>
const credentials = await this.credentialsProvider.getCredentials({ | ||
url: apiBaseUrl, | ||
}); |
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 think this needs to change to work for most people.
For most users of github they have github.com
as the host in their integrations config. The code here means that even if they configure github.com in the copilot section it will try to resolve api.github.com and fail (most people would not have this configured in their config)
So i thin kthis needs to be url: host
to make it work for most people.
There is also the issue that the integrations config can be done with a github app which would not have access to the enterprise API at all. This is how we have it configured and probably most users of GH will use an app (I'm guessing).
I think that the default here could be to have a specific token in the copilot section as this token is specific to accessing the enterprise API.
@awanlin i know you suggested this change here but im not sure it works in this instance as this enterprise API is a bit specific and i dont think enterprise APIs are used anywhere else that i have seen in backstage (yet).
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 feel like an extension point is a bit of an overkill, as adopters would need to reimplement their GithubCredentialsProvider
anyway. I would suggest implementing the missing features of the DefaultGithubCredentialsProvider
instead.
@vinzscam that is a lot of work to a core API just to get this plugin to work. Some of the content in #1261 may be able to work correctly with this configuration. But certainly not for enterprise APIs. The token required for the enterprise APIS is very powerful with enterprise level permissions over potentially dozens of organisations. It could very well be that lots of organisations want to keep that safe in some external store rather than in the backend config. |
@vinzscam where do we go from here? Should I change this PR so that the plugin uses its own config? At the moment I don't think this plugin is usable for people who use GitHub Apps to authenticate in the integrations section. The original author of this plugin does not use GH in backstage for anything other than accessing these metrics so they were not to know that this would not work for most users. |
#1244
Hey, I just made a Pull Request!
#1244
Added in an extension point for the copilot backend plugin to allow for custom resolution of credentials for the GitHub Enterprise Copilot API
✔️ Checklist
Signed-off-by
line in the message. (more info)