-
Notifications
You must be signed in to change notification settings - Fork 654
feat(ghes): Support for GitHub Enterprise Server #412
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
Conversation
mcaulifn
commented
Dec 17, 2020
- Updates lambdas to support GHES URL
- Updates TF to support GHES and deploying lambda in VPC
- Updates lambdas to support GHES URL - Updates TF to support GHES and deploying lambda in VPC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcaulifn thanks for this great contribution, you did really a great job not only by adding GHES but also updating and improving the code. Just a few remarks for now.
I only was able to verify the cloud runners, no impact noted. The only TF change is passing the GHES URL to the lambda, in the cloud case empty of source. I was not able to verify the GHES runners since I have no access to a GHES setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcaulifn looks all great, great all the work you did. Just one simple question
@@ -78,7 +78,7 @@ variable "runners_lambda_zip" { | |||
variable "runners_scale_up_lambda_timeout" { | |||
description = "Time out for the scale down lambda in seconds." | |||
type = number | |||
default = 60 | |||
default = 180 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small question, I assume you increased this default since it take a bit more time to boot a lambda in a VPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, lambda launches in VPC are much faster now. And I think this timer starts after it boots. When there were issues connecting to GHES, the lambda would timeout before the connection to GH would. This exposes that error. The other option would be to see if octokit can shorten the timeout. A quick check of the docs and its not super intuitive.
Consisent format requirements
@gertjanmaas please can you have a look to this PR? To me it looks all good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment regarding code formatting, otherwise looks good :)
@npalm Anything else keeping this from being merged? |
Thanks for pinging me, works quite qell merging via the GitHub app. @mcaulifn once agina, thanks for the great contribution. At this moment we haven't the option to test our selves on GHES. Expecting we have this option in the near feature. For that reason it tooks a bit longer with some discussion on how to proceed. Will make asap a release with this great new feature! |
@mcaulifn: When testing this with GitHub Enterprise 2.22, I get a 415 status code for the checks API in the scale-up lambda:
Could this be because GitHub.com no longer needs the As the header has been removed on GitHub.com pretty recently October 1st 2020 - I could imagine that you have been testing with an older version of octo-rest-js (< 10.0.12), could you point me to the exact version you have used? |
Update: When I reverted to octokit/rest-js/18.0.9 and removed the octokit/types reference in package.json, and rebuilt the lambda - the error went away 🎉 |
In case anybody likes to have the runners lambda working with GitHub Enterprise Server 2.22: runners.zip |
@mcaulifn do you have the same issue, just checked. Octokit/rest was update early december by dependabot. (https://github.com/philips-labs/terraform-aws-github-runner/pull/386). In case remvoing types solved the problem most likey an issue is introduced in version 18.0.11 see https://github.com/octokit/rest.js/releases. |
@npalm I can test on Tuesday, along with the actual patch version of 2.22 we have. |
Seeing same issue against |