-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
GitHub app credentials #971
GitHub app credentials #971
Conversation
Codecov Report
@@ Coverage Diff @@
## master #971 +/- ##
==========================================
- Coverage 71.49% 70.31% -1.18%
==========================================
Files 67 69 +2
Lines 5564 5778 +214
==========================================
+ Hits 3978 4063 +85
- Misses 1272 1388 +116
- Partials 314 327 +13
Continue to review full report at Codecov.
|
@unRob is this functional already? Would love to help test this as GitHub app integration is a blocker for our migration to our new GitHub enterprise setup (service-users are no longer allowed under corp policy and need to be replaced by apps). |
@donaldpiret It is functional but extremely experimental; I still have to write tests, and would like to setup a terraform test repo and play around before calling this ready. Bear in mind this only implements credentials, and does not go any further in doing fun stuff like posting check comments (see #936) nor automatically registering repositories with Atlantis via webhooks (which I'd love to work on next). Finally, the maintainers have not had a chance yet to send along their feedback, I would not feel comfortable recommending this PR be used in any mission-critical pipelines until they do. That being said, I've just now made sure the app-creation workflow is functional like so:
Feel free to test it out yourself, and please let me know if you run into trouble. I'll do my best to help debug! |
Hey Roberto, sorry for the delay responding. This looks amazing. What can I do to help get this merged? Is the code ready to review and test out yet? |
@lkysow I'll work this week on adding tests for Code is mostly ready though; I think testing will always reveal some places to improve. Hope that answers your questions, and let me know if there's other considerations I might have missed for seeing this PR through. Thanks! |
@lkysow I’m starting a job next week and will likely have less time to see this through, could you please review at your earliest convenience? Thanks! Ran e2e tests and addressed some remaining quirks. I’d love some guidance on testing and the way I’ve routed around access tokens. |
split fixtures out elsewhere, add working dir test
more docs, add installation step
Decided to squash commits since this PR is getting harder to rebase |
Got a panic while testign:
Looks like I still have to pass |
Thanks for the report!
I did, however, notice that the token was useless because I was still relying on per-repo webhooks, so I addressed that by activating the app-wide Webhook and listening to
In running these again, I noticed a probable cause for that panic: not passing in a key file (or otherwise having problems initializing |
// GithubAppWorkingDir implements WorkingDir. | ||
// It acts as a proxy to an instance of WorkingDir that refreshes the app's token | ||
// before every clone, given Github App tokens expire quickly | ||
type GithubAppWorkingDir struct { |
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.
Really awesome how clean this is.
DataDir: userConfig.DataDir, | ||
CheckoutMerge: userConfig.CheckoutStrategy == "merge", | ||
} | ||
// provide fresh tokens before clone from the GitHub Apps integration, proxy workingDir | ||
if githubAppEnabled { | ||
if !userConfig.WriteGitCreds { |
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.
I've changed this in my branch and instead will require users to set --write-git-creds. I didn't want to assume users wanted that file written, especially because it clobbers the old file. I've also changed that a bit in my branch so it just replaces the line that needs replacing.
This will be in the next Atlantis release. Amazing work!!! 🎉 🎉 |
Maybe there's something we can do about #418 ? Opening this PR for visibility, hoping to get some feedback on the approach, I'm happy to change directions if someone's got a better idea in mind!
This PR relies on https://github.com/bradleyfalzon/ghinstallation and their dependencies, which might not be fine. At this point its very incomplete, but basically my approach was to expose a few more config values to pass: a github app id, app installation and key (which at this point i had to get myself). These get picked up during
server.NewServer
and turned into the new typevcs.GithubCredentials
(implemented byvcs.GithubUserCredentials
andvcs.GithubAppCredentials
) before being passed tovcs.NewGithubClient
which finally creates the client for those credentials.Tests were minimally changed to pass, so this is still very much WIP
~/.gitconfig
credentials before clone with short-lived tokensIf the user doesn't have an app id and private key, the proposed flow is:
atlantis.host/github-app/setup
, you're prompted to begin the app creation,gh-app-id
,gh-app-key
, andgh-webhook-secret
,Otherwise, it's only steps 4 and 5.