akuity / kargo

Application lifecycle orchestration
https://kargo.akuity.io/
Apache License 2.0
1.39k stars 114 forks source link

Promotion / create PR feature for GitHub Enterprise server #1355

Open MarkusNeuron opened 5 months ago

MarkusNeuron commented 5 months ago

Checklist

Proposed Feature

With 0.3.0 we can now create PR as part of the promotion. This is the only remaining feature that blocks us to onboard Kargo.

Motivation

Enterprise corps use Github-Enterprise installations ;)

Suggested Implementation

Because of the similarities of Github- / Github Enterprise-APIs I hope its fairly simple to support also Github Enterprise servers.

krancour commented 5 months ago

Hi @MarkusNeuron!

@jessesuen designed the PR support so that it should be relatively easy to add support for additional git providers. It was always our intention to start with one and add more over time -- or recruit community assistance to add more over time.

If memory serves, the main difference between GitHub and GitHub Enterprise is in how the client is constructed. i.e. In the case of GitHub, there's no need to provide a base URL, but for GitHub Enterprise, one must. Again, if memory serves, once that's done, everything else mostly works the same. So, just as you guess, I think most of what @jessesuen already did for GitHub could probably be re-used.

The "hard" part in this case is figuring out one or both of:

@jessesuen idk if you might want to add any insight into this?

jessesuen commented 5 months ago

Yes, I think support for this might be as simple as introducing logic to infer that the hostname is enterprise github, then calling go-github with the WithEnterpriseURLs option:

NewClient(httpClient).WithEnterpriseURLs(baseURL, uploadURL) 

How I see the UX for this:

  1. Today, if we detect if the word "github" is in the hostname, we assume that it is GitHub. I trust that this will always be a safe assumption. However, currently we don't properly configure the client to use the new hostname. So the only thing left to do is: if "github" is in the hostname, but the hostname is not exactly "github.com", then we must call WithEnterpriseURLs(baseURL, uploadURL).

  2. Even with the above, there would be a case we wouldn't handle, which is: a user's github enterprise server might have a hostname like: git.mycompany.com. In this case, we couldn't/wouldn't assume it is github, and so we would need a system-wide setting to explicitly indicate git.mycompany.com is of type GitHub (and not BitBucket, etc...).

@MarkusNeuron option 1 should be quite easy to implement with a few lines of code, but not sure if hostname guessing would work for your use case, or if you would need an explicit system setting to indictate it.

jessesuen commented 5 months ago

how can users provide a hint (through a new CRD attribute, probably)

I think we were mostly saying the same thing. The difference in my proposal is for the hint to be done at a system level, as opposed to a CRD-level attribute.

krancour commented 5 months ago

The difference in my proposal is for the hint to be done at a system level, as opposed to a CRD-level attribute.

Agree. In hindsight, the CRD attribute suggestion suggestion didn't make sense.

MarkusNeuron commented 5 months ago

Hi @krancour and @jessesuen, first let me thank you to your great response times and cool suggestions. Mid- / Log-term I see now way around to manually configure the git provider & corresponding URL in some way ( CRD, annotations, ...) to flexible support different git providers.

As a quick hack the proposed integration...

but the hostname is not exactly "github.com", then we must call WithEnterpriseURLs(baseURL, uploadURL)

... would close our gap. Anyhow thanks for consideration.