Skip to content
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!: Filter labels on the server instead of client to allow more label filtering options #832

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

jnovick
Copy link
Contributor

@jnovick jnovick commented Aug 13, 2024

Filter labels on the server instead of client

  • This will allow supporting filtering by more label options
    • Multiple labels
    • Negative matching
    • Label exists regardless of value
  • This should reduce cpu, memory, and networking resources needed
  • Updated docs to reflect new capabilities

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.47%. Comparing base (65698c5) to head (b567975).

Files Patch % Lines
cmd/run.go 33.33% 2 Missing ⚠️
pkg/argocd/argocd.go 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
- Coverage   73.53%   73.47%   -0.07%     
==========================================
  Files          31       31              
  Lines        3140     3114      -26     
==========================================
- Hits         2309     2288      -21     
+ Misses        695      692       -3     
+ Partials      136      134       -2     

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

@jannfis
Copy link
Contributor

jannfis commented Aug 13, 2024

Will this work when using the Argo CD API instead of K8s to get the list of applications, too?

@jnovick
Copy link
Contributor Author

jnovick commented Aug 13, 2024

Will this work when using the Argo CD API instead of K8s to get the list of applications, too?

I tested submitting requests to my ArgoCD instance in the same format as the kubernetes api and it worked. I would love assistance validating it with the code, but I believe this should work. I am relatively new to adding contributions to this code so I was a bit confused why there is a kubernetes and an ArgoCD implementation of GetApplications, but I updated both. Which one is actually used? Or are they both used?

I will be building this image tomorrow to test it myself (Along with the other PR I submitted today). If I see any issues, I will be sure to fix it in this PR and update you, but my local tests seemed to work.

@jannfis
Copy link
Contributor

jannfis commented Aug 13, 2024

Hey, yeah, seems you have caught the right spot already. Thanks. I think I'm ok with this change :)

Can you please fix the linter issue? Thanks.

* This will allow supporting filtering by more label options
  * Multiple labels
  * Negative matching
  * Label exists regardless of value
* This should reduce cpu, memory, and networking resources needed
* Updated docs to reflect new capabilities

Signed-off-by: Joshua Novick <[email protected]>
@jnovick jnovick force-pushed the allow-multiple-labels branch from b567975 to 7cc61e5 Compare August 13, 2024 22:56
@jannfis jannfis changed the title Filter labels on the server instead of client to allow more label filtering options feat!: Filter labels on the server instead of client to allow more label filtering options Aug 14, 2024
Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this change makes sense.

It is a breaking change I suppose, so should be mentioned in the release notes CC @chengfang @pasha-codefresh

@jannfis jannfis merged commit 0ad5b94 into argoproj-labs:master Aug 14, 2024
10 checks passed
@chengfang chengfang added the release-note-item should be included in release note label Aug 14, 2024
@jnovick
Copy link
Contributor Author

jnovick commented Aug 14, 2024

I'm not sure it is a breaking change since any label set before will work still with the same behavior externally, but I do agree that it is worth putting in the release notes. I can tell you that putting this into our environment today has allowed us to shard the image updater by label which has allowed us to do a lot more updates in parallel. The change works exactly how I hoped that it would. Thank you so much for the quick response to my PRs.

Tchoupinax pushed a commit to Tchoupinax/argocd-image-updater that referenced this pull request Oct 23, 2024
…bel filtering options (argoproj-labs#832)

Signed-off-by: Joshua Novick <[email protected]>
Signed-off-by: Tchoupinax <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-item should be included in release note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants