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

Add ghe config option #559

Merged
merged 5 commits into from
Oct 8, 2024
Merged

Conversation

SebastianBezold
Copy link
Contributor

Description

This PR introduces support for self hosted GitHub Enterprise instances with URLs different than github.com.
With these changes, an environment variable ALLSTAR_GHE_URL can used to configure a custom GitHub URL.
If set, all GitHub clients created are initialized using this URL.

Additional Info

For some reason, this setup only works if additionally to ALLSTAR_GHE_URL, you also define GH_HOST with the same host, just except the protocol (i.e: ALLSTAR_GHE_URL="https://my-ghe.example.com" and GH_HOST="my-ghe.example.com").

I have tested this PR on our own internal GitHub enterprise instance with KEY_SECRET="direct" and providing the PRIVATe_KEY as plain-text .pem

Closes #552

Sebastian Bezold [email protected], Mercedes-Benz Tech Innovation GmbH, legal info/impressum

@SebastianBezold SebastianBezold requested a review from a team as a code owner August 15, 2024 15:13
operator.md Outdated
Comment on lines 79 to 80
export ALLSTAR_GHE_URL="https://my-ghe.example.com"
export GH_HOST="my-ghe.example.com" # This is somehow used within a dependency and (as of now) is unclear how to set it programmatically.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GH_HOST is used by the Scorecard dependency when setting up our clients to interact with GitHub. We don't currently have a way of setting this without using an environment variable. If this is a problem, I'm sure we could consider alternatives.

https://github.com/ossf/scorecard/blob/4303b741ea5afbb9b0d0420788043defb24f3740/clients/githubrepo/repo.go#L123-L132

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it is fine to just mention it in the docs. Thanks for the explanation 👍

Copy link
Member

@jeffmendoza jeffmendoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks great! Just a couple nits.

.gitignore Outdated
@@ -2,3 +2,4 @@ cmd/allstar/allstar
*.pem
.vscode
allstar.ref
.idea/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline at end of file

if i == 0 {
tr, err = ghinstallationNewAppsTransport(ctr, operator.AppID, g.key)
appTransport, _ := ghinstallationNewAppsTransport(ctr, operator.AppID, g.key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check error

tr, err = ghinstallationNewAppsTransport(ctr, operator.AppID, g.key)
appTransport, _ := ghinstallationNewAppsTransport(ctr, operator.AppID, g.key)
// other than clien.WithEnterpriseUrls, setting the BaseUrl plainly, we need to ensure the /api/v3 ending
appTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap in if operator.GitHubEnterpriseUrl != "" {

} else {
tr, err = ghinstallationNew(ctr, operator.AppID, i, g.key)
ghiTransport, _ := ghinstallationNew(ctr, operator.AppID, i, g.key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check error

@SebastianBezold
Copy link
Contributor Author

Hi @spencerschrock and @jeffmendoza,

sorry for the very late response. Have been on a longer private absence and was not able to work on this.
Thank you for your input. I'm right back working on it!

@SebastianBezold
Copy link
Contributor Author

Hi @jeffmendoza,

are the fixes to your comments ok? Is there anything else, that I should adapt?

@jeffmendoza jeffmendoza merged commit 38c2e3b into ossf:main Oct 8, 2024
7 checks passed
@jeffmendoza
Copy link
Member

Thanks for the reminder. Looks great. Thank you!!

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.

Interested in support for self-hosted GHE installation
3 participants