-
Notifications
You must be signed in to change notification settings - Fork 301
feat: Add configuration option to scope application listing / getting to particular namespace #1112
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: Add configuration option to scope application listing / getting to particular namespace #1112
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1112 +/- ##
==========================================
+ Coverage 63.49% 63.60% +0.10%
==========================================
Files 15 15
Lines 2326 2341 +15
==========================================
+ Hits 1477 1489 +12
- Misses 758 760 +2
- Partials 91 92 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I was unsure how to add tests for this change. I saw these tests, but I'm not sure how to set up something similar to assert that "if the client is configured with an |
@grahamalama thanks for the contribution. Can you please fix the DCO check failure? Is this PR also intended to address issue #1111 ? |
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
703dd62
to
700e3a5
Compare
@chengfang just pushed the DCO fix.
It could be I suppose! For me, my goal was to fix #902 specifically. But I think it'd addresses both. |
can we add tests with non-nil appNamespace, just for some basic sanity check? |
@grahamalama can you also update docs/install/reference.md to document the new flag? (https://argocd-image-updater.readthedocs.io/en/latest/install/reference/ |
@grahamalama can you also take a look at the comment on the related issue #1111 (comment). Although it covers a different issue (still related to this one), I think the comment about using listApp vs getApp is also revelant here. Some namespace may contain large number of apps and list operation in this namespace could still be a performance bottleneck. If we know image-updater is configured to 1 namespace, we should be able to just use getApp. |
Commented in #1111, and depending on the outcome of that conversation I'll update this PR accordingly. I will say though if the decision is to add some sort of caching option, I think that would be better suited for a separate PR. |
@grahamalama from the discussion here and in #1111, we are in agreement to improve this PR by using getApp instead of listApp when single namespace is configured in image-updater. |
…onfigured Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
20cd46a
to
3f7f71e
Compare
…application-namespace
@chengfang, made the changes we discussed in #1111 and added the requested documentation |
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
… to particular namespace (argoproj-labs#1112) Signed-off-by: Graham Beckley <gbeckley@mozilla.com>
In #854, AIU made changes so that it scanned applications across all namespaces. This introduced a few regressions for some use cases, namely #902.
In this PR, I introduce a config option so that users can optionally scope the K8s client to a specific namespace (as was the case before #854). Since searching across all namespaces was recently introduced as the default, I opted to keep that the default.