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: implement source refs for helm set-file (Beta) (#13220) #17941

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: implement source refs for helm set-file (Beta) (#13220) #17941

wants to merge 1 commit into from

Conversation

cardoe
Copy link
Contributor

@cardoe cardoe commented Apr 23, 2024 •
edited

In multi-source applications helm values files can come from other repos if you use the ref syntax but it is not possible for the --set-file param to take files from other repos via the ref syntax. This extends the behavior to cover set-file parameters as well as the values files. fixes #13220, refs #17822

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

@cardoe cardoe requested a review from a team as a code owner April 23, 2024 18:45
@cardoe
Copy link
Contributor Author

cardoe commented Apr 23, 2024

This still needs testing added. The documentation pieces are in #17822, which I will rebase this PR on top once that lands. I'll readily admit I'm not a Go programmer so probably did some things wrong from an idiomatic standpoint. Hoping another contributor is willing to help me get this over the finish line.

@cardoe cardoe mentioned this pull request Apr 23, 2024
Open
3 tasks
@cardoe cardoe changed the title feat: implement source refs for helm set-file (#13220) feat: implement source refs for helm set-file (Beta) (#13220) Apr 25, 2024
@cardoe
Copy link
Contributor Author

cardoe commented Apr 25, 2024

I've labeled this as beta since it's for multi-source applications which themselves are beta.

@cardoe cardoe mentioned this pull request Apr 25, 2024
Merged
14 tasks
cardoe added a commit to cardoe/argo-cd that referenced this pull request Apr 25, 2024
@cardoe
docs: document helm fileParameters / helm-set-file
e44e9dc 
Added documentation for fileParameters in a helm source Application or
the --helm-set-file cli option from argoproj#2751. Added a note about argoproj#13220 so
users are aware. argoproj#17941 aims to remove this limitation so the warning
will be adjusted to mention the version that lands that support.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
cardoe added a commit to cardoe/argo-cd that referenced this pull request Apr 25, 2024
@cardoe
docs: document helm fileParameters / helm-set-file
eb5c364 
Added documentation for fileParameters in a helm source Application or
the --helm-set-file cli option from argoproj#2751. Added a note about argoproj#13220 so
users are aware. argoproj#17941 aims to remove this limitation so the warning
will be adjusted to mention the version that lands that support.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
crenshaw-dev pushed a commit that referenced this pull request Apr 25, 2024
@cardoe
docs: document helm fileParameters / helm-set-file (#17822)
0f11dfb 
Added documentation for fileParameters in a helm source Application or
the --helm-set-file cli option from #2751. Added a note about #13220 so
users are aware. #17941 aims to remove this limitation so the warning
will be adjusted to mention the version that lands that support.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@cardoe cardoe requested a review from a team as a code owner April 27, 2024 00:37
@cardoe cardoe force-pushed the ref-helm-set-file branch 2 times, most recently from 87e6981 to 837f4df Compare May 4, 2024 17:43
@cardoe
Copy link
Contributor Author

cardoe commented May 4, 2024

I've set the documentation update to say the feature is available from ArgoCD 2.12+ because I'm assuming this won't make it into 2.11 since that's already in RC status.

@cardoe
Copy link
Contributor Author

cardoe commented May 4, 2024

So I pushed once to confirm the test failed. And the most recent test confirms the test passes.

@cardoe cardoe requested a review from morremeyer May 4, 2024 18:42
@morremeyer
Copy link
Contributor

@cardoe I can't open a PR against your branch, not sure why.

I added one commit at morremeyer@aa090e2 that uses slices.IndexFunc to check against all manifests, not just one.

Did not run the full test suite against it yet, feel free to add it to this PR.

@cardoe
feat: implement source refs for helm set-file
13a7943 
In multi-source applications helm values files can come from other repos
if you use the ref syntax but it is not possible for the --set-file
param to take files from other repos via the ref syntax. This extends
the behavior to cover set-file parameters as well as the values files.
fixes argoproj#13220, refs argoproj#17822

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
@cardoe
Copy link
Contributor Author

cardoe commented May 7, 2024

Thanks @morremeyer. I've squashed your change in since you prefixed it with fixup! and rebased the branch. Thanks for doing that!

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

@morremeyer morremeyer Awaiting requested review from morremeyer

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.

Cannot use file from other source as Helm --set-file param
2 participants
@cardoe @morremeyer