feat(applicationset): reuse repo-creds for an existing GitHub App

[iamnoah]

Closes #10079

[codecov]

Codecov Report

Merging #10092 (d9f5a61) into master (5181bc1) will increase coverage by 0.11%.
The diff coverage is 15.62%.

❗ Current head d9f5a61 differs from pull request most recent head ed3cc89. Consider uploading reports for the commit ed3cc89 to get more accurate results

@@            Coverage Diff             @@
##           master   #10092      +/-   ##
==========================================
+ Coverage   45.92%   46.04%   +0.11%     
==========================================
  Files         229      232       +3     
  Lines       28278    27908     -370     
==========================================
- Hits        12987    12849     -138     
+ Misses      13518    13321     -197     
+ Partials     1773     1738      -35     

Impacted Files	Coverage Δ
applicationset/generators/pull_request.go	42.85% <0.00%> (-3.30%)	⬇️
applicationset/generators/scm_provider.go	35.57% <0.00%> (-5.09%)	⬇️
applicationset/services/pull_request/github.go	14.00% <0.00%> (ø)
applicationset/services/pull_request/github_app.go	0.00% <0.00%> (ø)
applicationset/services/scm_provider/github.go	81.17% <0.00%> (ø)
applicationset/services/scm_provider/github_app.go	0.00% <0.00%> (ø)
...is/applicationset/v1alpha1/applicationset_types.go	34.69% <ø> (-3.24%)	⬇️
util/db/repo_creds.go	0.00% <0.00%> (ø)
util/db/repository.go	39.28% <ø> (ø)
util/db/repository_secrets.go	68.60% <0.00%> (-1.09%)	⬇️
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jetersen]

overall looks good to me.

I can wonder why there is almost no code reuse from the existing implementation for github app authentication or refactor of existing code.

Consider my comment nitpicking.

[crenshaw-dev]

Looks like this will be a pretty awesome enhancement!

I think you might have started down the path of auto-discovering the repo creds. Did that end up not being a viable option?

[iamnoah]

why there is almost no code reuse from the existing implementation for github app authentication

I went down this path and the answer is that after ghinstallation.New, the logic for checking out a repo vs. making API calls diverges significantly. repo-server uses the git protocol naturally, vs. REST for the GitHub API.

[iamnoah]

I think you might have started down the path of auto-discovering the repo creds. Did that end up not being a viable option?

I thought better of it for a few reasons: 1. You might want a different app to checkout code than the one that scans for PRs or branches. 2. If you had a credential you didn't expect to be used this way, consuming some rate limit and making unexpected calls just seems like a good way to freak people out.

[crenshaw-dev]

@iamnoah makes sense. Did you find that there's something acting as a hard barrier to auto-discovery, or simply that it was too involved to tackle in this PR?

Just curious, in case someone decides to pick up auto-discovery in the future.

[jetersen]

I went down this path and the answer is that after ghinstallation.New, the logic for checking out a repo vs. making API calls diverges significantly. repo-server uses the git protocol naturally, vs. REST for the GitHub API.

Thanks for taking the time detailing it 👍 Makes sense.

[iamnoah]

Did you find that there's something acting as a hard barrier to auto-discovery, or simply that it was too involved to tackle in this PR?

No, I had some code that would inject the argocddb, try each credential, and note if there was a 401 then stopping using it for one hour. But I realized that PRs need a separate permission and felt like the unintended consequences were too great.

I would want auto-discovery to:

• For PRs, first use the GitHub API to make sure the App has permission to read PRs
• Load the organization(s) the App is valid for and match that up to the configuration (don't use a credential you know will fail)
• Some sort of opt-in/out at in argocd-cm and per credential.
• Maybe refuse to automatically use any app credentials if there are more than one that could work.
• Optional: the ability to use multiple credentials.

[crenshaw-dev]

@iamnoah do you have time to resolve conflicts?

[crenshaw-dev]

lgtm - thanks @iamnoah!