fix(appset): improve git generator repourl fallback #21167
+732
−132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
blakepettersson
changed the title
blakepettersson
changed the title
blakepettersson
force-pushed
the
fix/project-scoped-git-generator
branch
7 times, most recently
from
390fcca to
6205485
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21167 +/- ##
==========================================
+ Coverage 55.20% 55.21% +0.01%
==========================================
Files 338 337 -1
Lines 57090 57056 -34
==========================================
- Hits 31516 31506 -10
+ Misses 22878 22857 -21
+ Partials 2696 2693 -3 ☔ View full report in Codecov by Sentry. |
blakepettersson
force-pushed
the
fix/project-scoped-git-generator
branch
from
6205485 to
63c7a0f
Compare
blakepettersson
changed the title
gdsoumya
requested changes
Dec 16, 2024
util/repository/repository.go
Outdated
Comment on lines
30
to
32
| if len(foundRepos) == 1 && appProject == "" { | ||
| return foundRepos[0], nil | ||
| } |
There was a problem hiding this comment.
We are already returning any matching repo cred whether it has a project specified or not in case appProject is empty and there's only one rep cred found so i think in the case where we have more than 1 matching repos we should still return one of the repo creds we find instead of throwing an error. We can add an extra step to prioritise the repo cred that's missing the project but if that doesn't exist we should just return the first matching cred.
blakepettersson
force-pushed
the
fix/project-scoped-git-generator
branch
from
cf3b175 to
b727302
Compare
With argoproj#18388, we added the possibility of being able to create a repository credential with the same URL per AppProject (previously one could only create one credential globally for the same URL). Sadly, this caused a regression in regards to ApplicationSets. Now, any credential that is used for an appset needs to have an unset appproject. This fixes it by using the same semantics as the API server (used when doing `argocd repo get` or `argocd repo delete`: * If there is only one repo credential with the given URL and there is no AppProject set on the Git genertor - use that. * If an AppProject is specified on the Git generator and the single repo cred does not match it, return an error. * If there are multiple repo credentials with the same URL, return an error if none matches the given project passed in from the git generator This could potentially be split into two PRs; one to allow for a more flexible retrieval of a _single_ URL, which can be backported to 2.12 - 2.13, and another with the CRD changes which would allow a project to be specified on the Git generator, which would go in 2.14+. Signed-off-by: Blake Pettersson <[email protected]>
For now, let's focus on unblocking users which have issues using git generators. There is a provision to easily extend this by adding a project field to the Git Generator, but this needs more discussion. Signed-off-by: Blake Pettersson <[email protected]>
In case there is no repo with a matching url and an empty project, return the first repo matching the repo url. Signed-off-by: Blake Pettersson <[email protected]>
blakepettersson
force-pushed
the
fix/project-scoped-git-generator
branch
3 times, most recently
from
05f0764 to
bd4c647
Compare
Stick to using getrepository and pass through the project values from `appSet.Spec.Template.Spec.Project`. Signed-off-by: Blake Pettersson <[email protected]>
blakepettersson
force-pushed
the
fix/project-scoped-git-generator
branch
from
bd4c647 to
4ac4211
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With #18388, we added the possibility of being able to create a repository credential with the same URL per AppProject (previously it was only possible to create one credential globally for the same URL). Sadly, this caused a regression in regards to ApplicationSets using Git Generators. Now, any credential that is used for an appset needs to have an unset appproject - which leads to a lot of hard to diagnose errors from users that are inadvertently using a repo cred that is scoped to a project.
This PR fixes it by using similar semantics as the API server (used when doingargocd repo getorargocd repo delete):* If there is only one repo credential with the given URL - use that.* If there are multiple repo credentials with the same URL - first find a repo cred with an unset approject* Otherwise, If there are multiple repo credentials with the same URL - use the first one found (order undefined)* Otherwise, fail with an errorThis has a provision to be extended with a project field on the Git generator itself. This would require CRD changes; for the sake of expediency we'll punt this for another PR.This PR addresses this by sending in the project field of an appset into
db.GetRepository. This has similar semantics to what already exists when the app controller is trying to resolve a repository credential for anApplication.This should only apply if the project field is not templated (i.e it does not contain
{{) - otherwise only a repository credential with an unset project can be used with the given appset (as today).This should be backported to 2.12 - 2.14.
Checklist: