-
Notifications
You must be signed in to change notification settings - Fork 275
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: Allow applications in any namespace #612
Conversation
1294f52
to
6ce47d1
Compare
👋 Looking for feedback on this, specifically around the default behavior of this flag. Ideally it should default to be global (i e I suspect I will have to add documentation updates as well as RBAC and helm chart updates, happy to do so if this is something the maintainers think is a good idea. |
da55471
to
b378e4b
Compare
Codecov Report
@@ Coverage Diff @@
## master #612 +/- ##
==========================================
+ Coverage 65.35% 65.48% +0.13%
==========================================
Files 22 22
Lines 2084 2092 +8
==========================================
+ Hits 1362 1370 +8
Misses 588 588
Partials 134 134
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@iandelahorne Thanks a lot for your contribution, and apologies that I again haven't found the time to react timely on it. I will try to review your PR the coming days. |
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.
First quick review, have a couple of comments.
Generally, I believe the pattern should follow a similar pattern than what Argo CD does, i.e. instead of having a bool
to turn it on, give it a list of namespaces that it's allowed to dig into. Then, ignore those applications that are popping up and that are not in that namespace.
pkg/argocd/argocd.go
Outdated
@@ -176,17 +176,16 @@ func FilterApplicationsForUpdate(apps []v1alpha1.Application, patterns []string, | |||
var appsForUpdate = make(map[string]ApplicationImages) | |||
|
|||
for _, app := range apps { | |||
logCtx := log.WithContext().AddField("application", app.GetName()) | |||
appNSName := fmt.Sprintf("%s/%s", app.GetNamespace(), app.GetName()) |
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.
You can use app.QualifiedName()
to get the representation of <namespace>/<name>
of an app instead of rolling your own here :)
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.
Oh nice, i'll go ahead and use those.
// Check whether application has our annotation set | ||
annotations := app.GetAnnotations() | ||
if _, ok := annotations[common.ImageUpdaterAnnotation]; !ok { | ||
logCtx.Tracef("skipping app '%s' of type '%s' because required annotation is missing", app.GetName(), app.Status.SourceType) |
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.
Why did you remove this trace statement?
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 honestly thought I had added it and was removing it before commiting, good catch.
continue | ||
} | ||
|
||
// Check for valid application type | ||
if !IsValidApplicationType(&app) { | ||
logCtx.Warnf("skipping app '%s' of type '%s' because it's not of supported source type", app.GetName(), app.Status.SourceType) |
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.
Why did you remove this warning statement?
pkg/argocd/argocd.go
Outdated
@@ -198,16 +197,16 @@ func FilterApplicationsForUpdate(apps []v1alpha1.Application, patterns []string, | |||
|
|||
// Check if application carries requested label | |||
if !matchAppLabels(app.GetName(), app.GetLabels(), appLabel) { | |||
logCtx.Debugf("Skipping app '%s' because it does not carry requested label", app.GetName()) | |||
logCtx.Debugf("Skipping app '%s' because it does not carry requested label", appNSName) |
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.
As mentioned earlier, you can use app.QualifiedName()
here
pkg/argocd/argocd.go
Outdated
continue | ||
} | ||
|
||
logCtx.Tracef("processing app '%s' of type '%v'", app.GetName(), app.Status.SourceType) | ||
logCtx.Tracef("processing app '%s' of type '%v'", appNSName, app.Status.SourceType) |
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.
As mentioned earlier, you can use app.QualifiedName()
here
pkg/argocd/argocd.go
Outdated
imageList := parseImageList(annotations) | ||
appImages := ApplicationImages{} | ||
appImages.Application = app | ||
appImages.Images = *imageList | ||
appsForUpdate[app.GetName()] = appImages | ||
appsForUpdate[appNSName] = appImages |
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.
As mentioned earlier, you can use app.QualifiedName()
here
Thanks for the feedback. It does sound cleaner to specify the list of namespaces or wildcard, and if none is specifed default to the current namespace. I'll take a stab at implementing that soon if that's the route you think we should go. In the meantime I addressed the most immediate PR feedback. |
This cluster role would be needed as well:
|
Hope it gets merged soon. much needed feature! :) |
Any news on this? |
Add `--namespaced` flag, that when true will only monitor the configured namespace for applications. If it is false, then monitor all namespaces for applications. By default this is set to `true` to preserve the current behavior. fixes argoproj-labs#611 Signed-off-by: Ian Delahorne <[email protected]> Signed-off-by: Ian Delahorne <[email protected]>
Use `app.QualifiedName()` instead of creating a namespace/name string. In addition, I had erroneously removed trace statements, which are now back. Signed-off-by: Ian Delahorne <[email protected]>
949b420
to
de9d4e4
Compare
@villisco Sorry for the delay, I've been busy with my day job. I'm starting to take a look at refactoring this to take a list of namespaces. |
Been using this PR image and works great so far! Thank you @iandelahorne Great work! :) |
This PR does not address the git write back method for applications in any namespace. Application overrides are still written to the file |
Any updates on this one? |
@torfjor thanks for pointing that out, i'll be sure to address it
@ElanHasson I'll try to address this over the holiday break - like I've
mentioned above my day job got in the way.
…On Wed, Dec 20, 2023 at 8:58 PM Elan Hasson ***@***.***> wrote:
Any updates on this one?
—
Reply to this email directly, view it on GitHub
<#612 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABNFBWIA3RJ2LHPAOUG4TTYKO6WDAVCNFSM6AAAAAA3XJQKYSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRVGQ4DCNRVGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
// ian
|
@iandelahorne I understand. I am happy to help in any way I can, just let me know! I also know @renchap wants this feature as well. |
@iandelahorne any news on this? I get this error in the logs when using this branch with git write back:
When I cherry-picked
|
@mcanevet @ElanHasson Apologies for the delays and for abandoning this. I just started a new position and ArgoCD is no longer a part of my daily life. If someone else wants to take this up and move it forward, I still believe this is a valuable feature. I just can't commit currently to updating this at any point in the near future sadly. |
Thanks @iandelahorne - I updated your PR and re-submitted it here. This PR can be closed now. |
Thanks @rossigee for taking this over! |
Add
--namespaced
flag, that when true will only monitor the configured namespace for applications. If it is false, then monitor all namespaces for applications.By default this is set to
true
to preserve the current behavior.fixes #601