feat: support github enterprise api#256
Conversation
ricardojdsilva87
requested review from
jmeridth,
sutterj and
zkoppert
as code owners
|
Thank you for your contribution. Unfortunately we can't assume just because an enterprise url is provided that the GitHub App is also in the enterprise. It could be outside the enterprise and still need to auth against the default api endpoint. We need to have the user let us know the app's scope. As mentioned in your discussion, allowing apps to be scoped to just the enterprise is a new feature. Thank you for the concatenation to f-strings. I do agree, that is more readable. Reviewing your PR now. |
|
Hello @jmeridth, thanks for your input, I've noticed that the github library has 2 different authentication methods depending if it's for github or github enterprise: Like you've mentioned most likely there is the need for that scenario where an app is created outside the GHE and still needs to authenticate against github api, but then the remaining actions must be done against the ghe endpoint. Not sure how to tackle this without more inputs. Also I have ran prettier against the README file to fix the lint errors. Regarding these, they were added to put some bullet points in the table, should we add the rule to the ignore list? Thanks |
Agreed. I think we'll have to request the user provide another environment variable like
I'm hesitant to do that but the bulleted list does read better. Are we able to ignore list the specific elements for |
|
Hello again,
Those marked with ❓ I'm not sure if they are already working in the current code. Also since you know better the code, feel free to do the necessary changes 🙂 Thanks |
.github/linters/.markdown-lint.yml
Outdated
| # MD033/no-inline-html - Inline HTML | ||
| MD033: | ||
| # Allowed elements | ||
| allowed_elements: [br, li, ul] No newline at end of file |
There was a problem hiding this comment.
Nice
Nit: newline at end of file please
Your change handles the last one. The 3rd and 4th should be covered already but don't mind being double checked. |
|
Can you please add the new environment variable to |
|
@jmeridth seems that the Docker CI step is stuck for 4 hours. The remaining steps seem to have ran successfully, also no lint issues. |
Yeah, the timeout happens once in a while and a re-run works. It did again in this case. Will look at your PR in whole again soon. When we merge we'll need these additions in the other GitHub OSPO Actions (I can give links), if you're interested. If not, no worries, I can get them knocked out while pointing here for your kickoff. Let me know. UPDATE: The other GitHub OSPO Actions |
3 participants
Pull Request
Proposed Changes
This pull requests makes possible the authentication against the GitHub Enterprise API with a GitHub Enterprise App using the mentioned environment variables:
GH_APP_IDGH_APP_INSTALLATION_IDGH_APP_PRIVATE_KEYA new environment variable
GITHUB_APP_ENTERPRISE_ONLYneeds to be set totrueso that the api endpoint can also be adjusted.If the GHE endpoint is provided a different login is made, also every function checks if the
ghevariable is set to modify the API endpoint using by default the endpoint `https://api.github.com.Reusing the existing code a simple check is made to switch the connector on the github library
All the following scenarios should be covered since there was no change in the previous implementation, just a switch on the login type was added for the previous environment variables:
Should fix github/issue-metrics#364
This PR also includes some code "cleanup" mostly on the prints to make it easier to read using f"" instead of the + signs.
Readiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducing@jeffrey-luszczmake lint results
make test results
Reviewer
fix,documentation,enhancement,infrastructure,maintenanceorbreaking