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.

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

feat(appset): Provide additional parameter files to git generator - Fixes #20593 #20653

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

feat(appset): Provide additional parameter files to git generator - Fixes #20593 #20653

wants to merge 2 commits into from
+2,639 −786

Conversation

OpenGuidou
Copy link
Contributor

@OpenGuidou OpenGuidou commented Nov 4, 2024 •
edited
Loading

Fixes #20593

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the 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).

@OpenGuidou OpenGuidou requested review from a team as code owners November 4, 2024 17:00
@bunnyshell Bunnyshell
Copy link

bunnyshell bot commented Nov 4, 2024 •
edited
Loading

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • ❌ /bns:delete to remove the environment

@codecov Codecov
Copy link

codecov bot commented Nov 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.99083% with 12 lines in your changes missing coverage. Please review.

Project coverage is 55.63%. Comparing base (fa747f9) to head (2c332c7).

Files with missing lines Patch % Lines
applicationset/generators/git.go 87.36% 8 Missing and 4 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #20653      +/-   ##
==========================================
- Coverage   55.67%   55.63%   -0.05%     
==========================================
  Files         339      339              
  Lines       56814    56903      +89     
==========================================
+ Hits        31634    31660      +26     
- Misses      22535    22590      +55     
- Partials     2645     2653       +8

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

@OpenGuidou OpenGuidou force-pushed the 20593-gitgen branch 4 times, most recently from 332728c to f8f5a0f Compare November 5, 2024 10:22
speedfl
speedfl reviewed Nov 8, 2024
View reviewed changes
docs/operator-manual/applicationset/Generators-Git.md
A hierarchy occurs in case of collision betweens the parameters, as follows:

* Parameters are merged and overriden file by file, from top to bottom of the list provided
* When a file provided is a glob pattern, files are sorted by path and overrides occur in the sorted order.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure it's a good idea to provide globs here.
Forcing people to sort alphabetically can lead to error. I would rather enforce explicit path

What do you think @crenshaw-dev @ishitasequeira ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm partial to leaving the glob supported as long as we document it properly people can use it and it's convenient.

@OpenGuidou OpenGuidou force-pushed the 20593-gitgen branch 3 times, most recently from 30e93ef to c436323 Compare November 18, 2024 08:51
crenshaw-dev
crenshaw-dev reviewed Dec 19, 2024
View reviewed changes
docs/operator-manual/applicationset/Generators-Git.md Outdated
Comment on lines 423 to 426
- path: "applicationset/examples/git-generator-files-discovery/cluster-config/**/config.json"
extraParameterFiles:
- "mypath/common-parameters/**/*.json
- "myotherpath/specific-params/params-{{.cluster.name}}.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- path: "applicationset/examples/git-generator-files-discovery/cluster-config/**/config.json"
extraParameterFiles:
- "mypath/common-parameters/**/*.json
- "myotherpath/specific-params/params-{{.cluster.name}}.yaml"
- path: "applicationset/examples/git-generator-files-discovery/cluster-config/**/config.json"
- path: "mypath/common-parameters/**/*.json
prefix: extraParams
- path: "myotherpath/specific-params/params-{{.cluster.name}}.yaml"
prefix: extraExtraParams

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice ! It just complexifies a bit the whole thing as I was implementing a basic yaml merge as helm is doing with values files.
But we can just add the extra top level from the prefix and do the merge anyway, so it should work !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with that change

@todaywasawesome todaywasawesome mentioned this pull request Dec 19, 2024
Open
3 tasks
@OpenGuidou
Copy link
Contributor Author

Same flakky test failure as master

@crenshaw-dev , @todaywasawesome , @speedfl , @andrii-korotkov-verkada : Everything is ready now. Could you perform a final check ?

@speedfl
Copy link
Contributor

speedfl commented Dec 20, 2024

Added one last comment. Otherwise LGTM 👍

@tal-perchuk-biocatch
Copy link

tal-perchuk-biocatch commented Jan 7, 2025 •
edited
Loading

Hi @OpenGuidou ,
i'd kindly like to ask if there is any update on this PR, and whether it is going to be merged soon?

@OpenGuidou
Copy link
Contributor Author

Hi @OpenGuidou , i'd kindly like to ask if there is any update on this PR, and whether it is going to be merged soon?

Hi, I just rebased the branch and for me everything is ready and all comments were addressed.
I'm pending the maintainers approval.

@argoproj/argocd-approvers could you give a final stamp ?

@tal-perchuk-biocatch
Copy link

tal-perchuk-biocatch commented Feb 10, 2025 •
edited
Loading

Hi @crenshaw-dev @argoproj/argocd-approvers,

Is there any timeline for merging this PR?

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@speedfl speedfl speedfl left review comments

@andrii-korotkov-verkada andrii-korotkov-verkada Awaiting requested review from andrii-korotkov-verkada

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

@todaywasawesome todaywasawesome Awaiting requested review from todaywasawesome

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

Enhance git generator with parameter files
6 participants
@OpenGuidou @speedfl @tal-perchuk-biocatch @crenshaw-dev @todaywasawesome @andrii-korotkov-verkada