8000 feat: add ability to exclude files when using git file generator by nitishfy · Pull Request #22734 · argoproj/argo-cd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add ability to exclude files when using git file generator #22734

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
May 15, 2025
Merged

feat: add ability to exclude files when using git file generator #22734

merged 15 commits into from
May 15, 2025

Conversation

nitishfy
Copy link
Member
@nitishfy nitishfy commented Apr 21, 2025 •
edited
Loading

Closes #13690

After

image

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with t 8000 he community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@bunnyshell Bunnyshell
Copy link
bunnyshell bot commented Apr 21, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@nitishfy nitishfy mentioned this pull request Apr 21, 2025
Closed
13 tasks
@codecov Codecov
Copy link
codecov bot commented Apr 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@9675487). Learn more about missing BASE report.
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
applicationset/generators/git.go 94.73% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #22734   +/-   ##
=========================================
  Coverage          ?   60.01%           
=========================================
  Files             ?      343           
  Lines             ?    57826           
  Branches          ?        0           
=========================================
  Hits              ?    34705           
  Misses            ?    20351           
  Partials          ?     2770

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

@nitishfy nitishfy force-pushed the nitish/git-file-generator branch from 0eacf01 to c084da2 Compare April 22, 2025 08:09
@nitishfy nitishfy marked this pull request as ready for review April 22, 2025 08:55
@nitishfy nitishfy requested review from a team as code owners April 22, 2025 08:55
rumstead
rumstead reviewed Apr 22, 2025
View reviewed changes
rumstead
rumstead reviewed Apr 22, 2025
View reviewed changes
rumstead
rumstead reviewed Apr 22, 2025
View reviewed changes
rumstead
rumstead reviewed Apr 22, 2025
View reviewed changes
@nitishfy nitishfy requested a review from rumstead April 22, 2025 15:30
@cardoe
Copy link
Contributor
cardoe commented Apr 22, 2025

These changes made sense to me when I read through them.

rumstead
rumstead approved these changes Apr 22, 2025
View reviewed changes
Copy link
Member
@rumstead rumstead left a comment

Choose a reason for hiding this comment

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

LGTM

@nitishfy nitishfy changed the title feat: add ability to exclude files when using git generator feat: add ability to exclude files when using git file generator Apr 23, 2025
rumstead
rumstead requested changes Apr 24, 2025
View reviewed changes
Copy link
Member
@rumstead rumstead left a comment

Choose a reason for hiding this comment

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

based on the contributor meeting we should respect the globbing function passed in by the user

agaudreault
agaudreault requested changes Apr 24, 2025
View reviewed changes
rumstead
rumstead approved these changes Apr 30, 2025
View reviewed changes
Copy link
Member
@rumstead rumstead left a comment

Choose a reason for hiding this comment

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

LGTM

@nitishfy
Copy link
Member Author
nitishfy commented May 1, 2025

This PR is completely ready for review (possibly merge too). Here are the things I've done after the last feedbac:

  1. Instead of matching the files with only new globbing pattern library (i.e doublestar), we are respecting the user's option now. We are retrieving the files and matches the include/exclude pattern and finally remove those from the map that matches the exclude pattern. Please read this. Based on @crenshaw-dev feedback, we can parametrize to do this on the repo-server side as a part of future enhancement.
  2. I've spent major time testing this feature. The following tests have been added to check the functionality:
    • Test to check the feature with new globbing pattern
    • Test to check the feature with old globbing pattern
    • IMPORTANT: Test to check the working of LsFiles function and making sure the git file generator files retrieves the same files that were tested as a part of tests for LsFiles. In other words, the Git File Generator tests makes sure that the output you get after doing pattern matching is same as you'd get from the LsFiles function (test here).

Please let me know if you've any questions. Thanks!

@nitishfy nitishfy requested review from crenshaw-dev and rumstead May 1, 2025 11:57
todaywasawesome
todaywasawesome reviewed May 1, 2025
View reviewed changes
Copy link
Contributor
@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

@nitishfy I assume a change to an ignored file will still trigger a rebuild of the application set so it can check what changed etc. I don't think that's a big issue in most scenarios.

todaywasawesome
todaywasawesome reviewed May 1, 2025
View reviewed changes
Copy link
Contributor
@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

The only thing making me nervous in all these PRs are the new comments on unchanged functions. I feel like they need to be double checked or removed. Did you go through and write them all?

@nitishfy
Copy link
Member Author
nitishfy commented May 1, 2025

@nitishfy I assume a change to an ignored file will still trigger a rebuild of the application set so it can check what changed etc. I don't think that's a big issue in most scenarios.

How is that related to the change? Would you mind providing details about it?

The only thing making me nervous in all these PRs are the new comments on unchanged functions. I feel like they need to be double checked or removed. Did you go through and write them all?

Yep! I did go through them all. Those are mere refactoring stuff and we can ignore if we would want to but I read those function definitions and created a short docs. I did this before too.

8000
@nitishfy
Copy link
Member Author
nitishfy commented May 1, 2025

I went through the comments again. Those seem to be correct as I was playing with those functions while trying to understand codebase. So I don't think we'd be required to remove those function comments anymore.

@nitishfy nitishfy force-pushed the nitish/git-file-generator branch from 888f97f to f1bd41c Compare May 13, 2025 04:48
@nitishfy
fix conflicts
266b523 
Signed-off-by: Nitish Kumar <nitishkumar@192.168.1.8>
Signed-off-by: nitishfy <justnitish06@gmail.com>
@nitishfy nitishfy force-pushed the nitish/git-file-generator branch from f1bd41c to 266b523 Compare May 13, 2025 04:49
rumstead
rumstead reviewed May 13, 2025
View reviewed changes
rumstead
rumstead reviewed May 13, 2025
View reviewed changes
rumstead
rumstead reviewed May 13, 2025
View reviewed changes
@nitishfy
setup dir and files in one go
695b6b1 
Signed-off-by: nitishfy <justnitish06@gmail.com>
@nitishfy nitishfy force-pushed the nitish/git-file-generator branch from ef0805f to 695b6b1 Compare May 13, 2025 14:41
@rumstead
Copy link
Member

LGTM - there have been many great comments on the PR, and i don't want to merge before folks get a second to chime in. @crenshaw-dev, @todaywasawesome, @agaudreault any final feedback?

@blakepettersson
Copy link
Member

@nitishfy I think this generally looks good! I don't know if anyone else would agree, but perhaps instead of adding a new field to the CRD, we'd instead make use of negation, i.e any exclusion would be prefixed with an !.

This is how this has been done elsewhere in the codebase, and IMO could make for a more consistent and intuitive experience. What do y'all think?

@nitishfy
Copy link
Member Author

@nitishfy I think this generally looks good! I don't know if anyone else would agree, but perhaps instead of adding a new field to the CRD, we'd instead make use of negation, i.e any exclusion would be prefixed with an !.

This is how this has been done elsewhere in the codebase, and IMO could make for a more consistent and intuitive experience. What do y'all think?

This is another approach to do the same behaviour. Rn, we are trying to make it consistent with how the git directory generator generall works - it has the exclude field in it. I was discussing with @crenshaw-dev about some future enhancements that was about adding a globbing option in the CRD spec. I think what you just mentioned can be put together along with adding a glob field in the CRD as a part of future enhancement. This way we are reducing a field in the CRD spec as well as adding a new field to determine the globbing pattern.

@rumstead rumstead enabled auto-merge (squash) May 14, 2025 10:30
agaudreault
agaudreault reviewed May 14, 2025
View reviewed changes
@agaudreault
Update docs/operator-manual/applicationset/Generators-Git.md
eab73ec 
Signed-off-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
@rumstead rumstead merged commit dd5a878 into argoproj:master May 15, 2025
26 checks passed
@nitishfy nitishfy deleted the nitish/git-file-generator branch May 15, 2025 05:37
@nitishfy nitishfy mentioned this pull request May 15, 2025
Open
LyhengTep pushed a commit to LyhengTep/argo-cd that referenced this pull request May 15, 2025
@nitishfy @LyhengTep
feat: add ability to exclude files when using git file generator (arg…
0db1c02 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
LyhengTep pushed a commit to LyhengTep/argo-cd that referenced this pull request May 15, 2025
@nitishfy @LyhengTep
feat: add ability to exclude files when using git file generator (arg…
f19ab39 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: Lyheng <lyhengtep@gmail.com>
ranakan19 pushed a commit to ranakan19/argo-cd that referenced this pull request May 20, 2025
@nitishfy @ranakan19
feat: add ability to exclude files when using git file generator (arg…
0f6f199 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: Kanika Rana <krana@redhat.com>
olivergondza pushed a commit to olivergondza/argo-cd that referenced this pull request May 20, 2025
@nitishfy @olivergondza
feat: add ability to exclude files when using git file generator (arg…
3ee3912 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
@nitishfy nitishfy mentioned this pull request May 27, 2025
Open
3 tasks
tylerrosnett pushed a commit to StateFarmIns/argo-cd that referenced this pull request May 27, 2025
@nitishfy @tylerrosnett
feat: add ability to exclude files when using git file generator (arg…
1577325 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
chansuke pushed a commit to chansuke/argo-cd that referenced this pull request Jun 4, 2025
@nitishfy @chansuke
feat: add ability to exclude files when using git file generator (arg…
1b886e2 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
@nitishfy @dsuhinin
feat: add ability to exclude files when using git file generator (arg…
0c26666 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025
@nitishfy @dsuhinin
feat: add ability to exclude files when using git file generator (arg…
97b3ffa 
…oproj#22734)

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@todaywasawesome todaywasawesome todaywasawesome left review comments

@cardoe cardoe cardoe approved these changes

@rumstead rumstead rumstead approved these changes

@agaudreault agaudreault agaudreault approved these changes

@crenshaw-dev crenshaw-dev Awaiting requested review from crenshaw-dev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the ability to exclude files when using the git file generator #468
7 participants
@nitishfy @cardoe @rumstead @blakepettersson @crenshaw-dev @todaywasawesome @agaudreault
0