Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

grahamalama
Copy link

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.

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.65%. Comparing base (61ea007) to head (700e3a5).

Files with missing lines Patch % Lines
cmd/run.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1112      +/-   ##
==========================================
+ Coverage   63.49%   63.65%   +0.15%     
==========================================
  Files          15       15              
  Lines        2326     2336      +10     
==========================================
+ Hits         1477     1487      +10     
  Misses        758      758              
  Partials       91       91              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@grahamalama
Copy link
Author

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 appNamespace, it will only search for that namespace". Maybe asserting the fake client was called with certain parameters? Otherwise I think we'd end up testing the mock.

@chengfang
Copy link
Collaborator

@grahamalama thanks for the contribution. Can you please fix the DCO check failure? Is this PR also intended to address issue #1111 ?

@grahamalama grahamalama force-pushed the optionally-scope-application-namespace branch from 703dd62 to 700e3a5 Compare April 29, 2025 19:02
@grahamalama
Copy link
Author

grahamalama commented Apr 29, 2025

@chengfang just pushed the DCO fix.

Is this PR also intended to address issue #1111?

It could be I suppose! For me, my goal was to fix #902 specifically. But I think it'd addresses both.

@grahamalama grahamalama changed the title feat: Add confition option to scope application listing / getting to particular namespace feat: Add configuration option to scope application listing / getting to particular namespace Apr 29, 2025
@chengfang
Copy link
Collaborator

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 appNamespace, it will only search for that namespace". Maybe asserting the fake client was called with certain parameters? Otherwise I think we'd end up testing the mock.

can we add tests with non-nil appNamespace, just for some basic sanity check?

@chengfang
Copy link
Collaborator

@grahamalama can you also update docs/install/reference.md to document the new flag? (https://argocd-image-updater.readthedocs.io/en/latest/install/reference/

@chengfang
Copy link
Collaborator

@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.

@grahamalama
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants