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(ghes): Support for GitHub Enterprise Server #412

Merged
merged 10 commits into from
Jan 14, 2021

Conversation

mcaulifn
Copy link
Contributor

  • Updates lambdas to support GHES URL
  • Updates TF to support GHES and deploying lambda in VPC

mcaulifn and others added 2 commits December 17, 2020 14:39
- Updates lambdas to support GHES URL
- Updates TF to support GHES and deploying lambda in VPC
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.

@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.

modules/runners/variables.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
modules/runner-binaries-syncer/runner-binaries-syncer.tf Outdated Show resolved Hide resolved
modules/runners/scale-down.tf Outdated Show resolved Hide resolved
modules/runners/scale-up.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
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.

@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
Copy link
Member

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?

Copy link
Contributor Author

@mcaulifn mcaulifn Jan 8, 2021

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.

@npalm
Copy link
Member

npalm commented Jan 11, 2021

@gertjanmaas please can you have a look to this PR? To me it looks all good.

Copy link
Collaborator

@gertjanmaas gertjanmaas left a 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 :)

modules/runners/lambdas/runners/.prettierrc Outdated Show resolved Hide resolved
@mcaulifn
Copy link
Contributor Author

@npalm Anything else keeping this from being merged?

@npalm npalm merged commit 1f105a9 into philips-labs:develop Jan 14, 2021
@mcaulifn mcaulifn deleted the nmcauliffe/ghes branch January 14, 2021 19:32
@npalm
Copy link
Member

npalm commented Jan 14, 2021

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!

@jonico
Copy link
Contributor

jonico commented Jan 16, 2021

@mcaulifn: When testing this with GitHub Enterprise 2.22, I get a 415 status code for the checks API in the scale-up lambda:

2021-01-16T00:50:28.129Z f7712571-de6f-571f-ba28-7d0de4e0db49 ERROR RequestError [HttpError]: If you would like to help us test the Checks API during its preview period, you must specify a custom media type in the 'Accept' header. Please see the docs for full details.
at /var/task/index.js:2756:23
at processTicksAndRejections (internal/process/task_queues.js:97:5) {
status: 415,
headers: {
'access-control-allow-origin': '*',
'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset',
connection: 'close',
'content-length': '288',
'content-security-policy': "default-src 'none'",
'content-type': 'application/json; charset=utf-8',
date: 'Sat, 16 Jan 2021 00:50:28 GMT',
'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
server: 'GitHub.com',
status: '415 Unsupported Media Type',
'strict-transport-security': 'max-age=31536000; includeSubdomains',
'x-content-type-options': 'nosniff',
'x-frame-options': 'deny',
'x-github-enterprise-version': '2.22.5',
'x-github-media-type': 'github.v3; format=json',
'x-github-request-id': '16446f5c-f402-42b9-96dc-f109ce3f7b9e',
'x-ratelimit-limit': '5000',
'x-ratelimit-remaining': '4996',
'x-ratelimit-reset': '1610761771',
'x-runtime-rack': '0.054488',
'x-xss-protection': '1; mode=block'
},
request: {
method: 'GET',
url: 'https://octodemo.com/api/v3/repos/baseline/baseline.github.io/check-runs/503',
headers: {
accept: 'application/vnd.github.v3+json',
'user-agent': 'octokit-rest.js/18.0.12 octokit-core.js/3.2.4 Node.js/12.19.0 (linux; x64)',
authorization: 'token [REDACTED]'
},
request: { hook: [Function: bound bound register] }
},
documentation_url: 'https://docs.github.com/enterprise/2.22/rest/reference/checks#get-a-check-run'
}

Could this be because GitHub.com no longer needs the application/vnd.github.antiope-preview+jsonpreview header and newer versions of octokit do no longer include it?

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?

@jonico
Copy link
Contributor

jonico commented Jan 16, 2021

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 🎉

@jonico
Copy link
Contributor

jonico commented Jan 16, 2021

In case anybody likes to have the runners lambda working with GitHub Enterprise Server 2.22: runners.zip

@npalm
Copy link
Member

npalm commented Jan 16, 2021

@mcaulifn do you have the same issue, just checked. Octokit/rest was update early december by dependabot. (#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.

@mcaulifn
Copy link
Contributor Author

@npalm I can test on Tuesday, along with the actual patch version of 2.22 we have.

@mcaulifn
Copy link
Contributor Author

mcaulifn commented Jan 19, 2021

Seeing same issue against 2.22.3. I opened https://github.com/octokit/rest.js/issues/1983 to address. Suggest we revert #386 for now.
Opened #478 to investigate further.

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

5 participants