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

copilot: Add an extension point for credentials resolution #1260

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ScottGuymer
Copy link

@ScottGuymer ScottGuymer commented Sep 19, 2024

#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

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@ScottGuymer ScottGuymer requested a review from a team as a code owner September 19, 2024 14:31
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Sep 19, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-copilot-backend workspaces/copilot/plugins/copilot-backend minor v0.1.2

Signed-off-by: Scott Guymer <[email protected]>
@ScottGuymer ScottGuymer changed the title Add an extension point for credentials resolution copliot: Add an extension point for credentials resolution Sep 19, 2024
Signed-off-by: Scott Guymer <[email protected]>
Comment on lines +89 to +91
const credentials = await this.credentialsProvider.getCredentials({
url: apiBaseUrl,
});
Copy link
Author

@ScottGuymer ScottGuymer Sep 19, 2024

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).

Copy link
Member

@vinzscam vinzscam left a 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.

@nickboldt nickboldt changed the title copliot: Add an extension point for credentials resolution copilot: Add an extension point for credentials resolution Sep 24, 2024
@ScottGuymer
Copy link
Author

@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.

@ScottGuymer
Copy link
Author

@vinzscam where do we go from here?

Should I change this PR so that the plugin uses its own config?
Add a default implementation of this extension that allows to access that config?
Something else?

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.

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.

2 participants