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.

Sign up for GitHub

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

Jump to bottom

feat: PR to Support GitHub Enterprise Cloud with Data Residency #4367

Merged
merged 11 commits into from
Jan 28, 2025
Merged

feat: PR to Support GitHub Enterprise Cloud with Data Residency #4367

merged 11 commits into from
Jan 28, 2025

Conversation

neethu-p
Copy link
Contributor

@neethu-p neethu-p commented Jan 17, 2025 •
edited
Loading

Hello team,
This PR is raised to enhance the module’s functionality to support GitHub Enterprise Cloud with data residency.
Reference issue - #4364

Below are the details of the changes:

  1. The PR introduces support for GitHub Enterprise Cloud instances hosted on a dedicated subdomain of ghe.com, in addition to the current handling of github.com and GHES.

2.The primary difference between github.com and ghe.com is the API access URL. While github.com uses https://api.github.com, GHE Cloud instances use the format https://api.companyname.ghe.com.

  1. To accommodate this, I have utilised the existing ghes_url parameter. The new ghe.com URL can be passed to this parameter for smooth integration.

  2. This change has been thoroughly tested across all GitHub instances: GHES, github.com, and ghe.com, ensuring compatibility and reliability.
    Please review the PR and let me know if you have any feedback or suggestions.

@neethu-p neethu-p requested review from a team as code owners January 17, 2025 12:37
@neethu-p neethu-p mentioned this pull request Jan 17, 2025
Open
@neethu-p neethu-p changed the title PR to Support GitHub Enterprise Cloud with Data Residency feat: PR to Support GitHub Enterprise Cloud with Data Residency Jan 17, 2025
jervi
jervi reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@jervi jervi left a comment

Choose a reason for hiding this comment

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

Just some nitpicking - but there's a chance that the tests for ghe.com gives a false positive (e.g. fooghe.com) - it's safer to include the preceding period.

README.md Outdated Show resolved Hide resolved
lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts Outdated Show resolved Hide resolved
lambdas/functions/control-plane/src/scale-runners/scale-up.ts Outdated Show resolved Hide resolved
@npalm npalm self-requested a review January 20, 2025 12:46
@npalm
Copy link
Member

npalm commented Jan 20, 2025

Great to see the work is done to support GH enterprise cloud. Not time to review the PR yet. But do you also see any chance to adjust docs and make clear GHES configuraiton also support enterprise cloud.

@neethu-p
Copy link
Contributor Author

Yes, the docs and README files are updated accordingly. Kindly have a look

@neethu-p
Copy link
Contributor Author

Hello @npalm Have you got a chance to review the PR ?

@npalm
Copy link
Member

npalm commented Jan 24, 2025

Sorry this week was too busy. Will check the PR asap. Thanks for all the support on GHEC

npalm
npalm requested changes Jan 27, 2025
View reviewed changes
Copy link
Member

@npalm npalm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@neethu-p took a bit of time to review. Looks good made a view tiny remarks. Will test the change as well, will update the PR comments once done.

lambdas/functions/control-plane/src/scale-runners/scale-up.ts Show resolved Hide resolved
modules/multi-runner/README.md Show resolved Hide resolved
modules/runners/README.md Show resolved Hide resolved
@npalm
Copy link
Member

npalm commented Jan 27, 2025

Tested a deployment for both pool and scale-up. All working as epxtected

@neethu-p
Copy link
Contributor Author

@npalm The review comments are addressed!

npalm
npalm approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@npalm npalm left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@npalm npalm merged commit e5ebd5f into github-aws-runners:main Jan 28, 2025
1 check passed
npalm added a commit that referenced this pull request Jan 28, 2025
@npalm
Revert "feat: Support GitHub Enterprise Cloud with Data Residency (#4367
245adeb 
)"

This reverts commit e5ebd5f.
@npalm
Copy link
Member

npalm commented Jan 28, 2025

@neethu-p]reverting the PR since the ci is borken, strange it should break on the PR:
https://github.com/github-aws-runners/terraform-aws-github-runner/actions/runs/13007945341/job/36278943305

@npalm npalm mentioned this pull request Jan 28, 2025
Merged
npalm added a commit that referenced this pull request Jan 28, 2025
@npalm
chore: "Revert Support GitHub Enterprise Cloud with Data Residency " (#…
bbf8a4e 
…4389)

Reverts #4367

Broken CI:
https://github.com/github-aws-runners/terraform-aws-github-runner/actions/runs/13007945341/job/36278943305

@neethu-p can you have a look?
@runners-releaser runners-releaser bot mentioned this pull request Jan 28, 2025
Merged
@npalm
Copy link
Member

npalm commented Jan 28, 2025

@neethu-p please can you re-create the PR, soe we can see how we can fix the CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jervi jervi jervi left review comments

@npalm npalm npalm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neethu-p @npalm @jervi