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

feat: add support for github enterprise server #132

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

kennyg
Copy link
Contributor

@kennyg kennyg commented Jul 30, 2023

Description

This PR adds support for Github Enterprise. I used this PR as a guide, and added a small test.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added support for Github Enterprise servers.

@kennyg kennyg requested a review from brikis98 as a code owner July 30, 2023 21:33
Copy link
Contributor

@andyfeller andyfeller left a comment

Choose a reason for hiding this comment

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

For what it's worth, these changes look good to me :fishsticks:

GithubHostname := os.Getenv("GITHUB_HOSTNAME")
baseUrl := fmt.Sprintf("https://%s/", GithubHostname)

githubClient, _ = github.NewEnterpriseClient(baseUrl, baseUrl, tc)
Copy link
Contributor

Choose a reason for hiding this comment

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

NewEnterpriseClient function checks out 👌 ... I was initially asking whether this took into account the path aspects for GHES API endpoints, but confirmed that it should be handled appropriately.

@kennyg
Copy link
Contributor Author

kennyg commented Aug 27, 2023

@brikis98 can you help with an approval for this PR?

@andyfeller
Copy link
Contributor

@brikis98 @zackproser : is there someone else at Gruntworks who might be capable of reviewing and approving this PR? 🙏

@zackproser
Copy link
Contributor

@andyfeller you could try @josh-padnick or @hongil0316 ?

@josh-padnick
Copy link

@gitsstewart Could you identify an SME to help review this PR?

@gitsstewart
Copy link

I will ask the team if anyone is available quickly for a review

Copy link
Contributor

@hongil0316 hongil0316 left a comment

Choose a reason for hiding this comment

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

Small nit comment but overall LGTM. Let me trigger the test pipeline

@@ -52,8 +52,20 @@ func ConfigureGithubClient() GithubClient {

tc := oauth2.NewClient(context.Background(), ts)

var githubClient *github.Client

if os.Getenv("GITHUB_HOSTNAME") != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we change the environment variable name to something like: GITHUB_URL and expect the user to pass in the full URL? This would potentially remove confusion from users whether they would have to include https: at the front or not.

@hongil0316
Copy link
Contributor

@hongil0316
Copy link
Contributor

Confirmed that all the tests passed. Will merge.

@hongil0316 hongil0316 merged commit 070d1a6 into gruntwork-io:master Nov 30, 2023
2 checks passed
@thirstydeveloper
Copy link

This will be very helpful to my team!

@hongil0316 - could you release a new version with this feature?

@josh-padnick
Copy link

@hongil0316 I'll go ahead and issue the release since the only thing it includes is this change.

@josh-padnick
Copy link

Released in v0.1.11

@hongil0316
Copy link
Contributor

Oh sorry about that @thirstydeveloper, I must have missed this. Thanks @josh-padnick for taking it! :)

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.

None yet

7 participants