Repository navigation
Conversation
NewApp assigned the Package's spec to the App through a shallow copy, so the App
spec kept pointers into the Package: AppTemplate.Ytt is a pointer, and
AppSpec.Template shares its backing array. Appending the PackageInstall's values
into templateStep.Ytt/HelmTemplate.ValuesFrom therefore wrote back into the
Package that was passed in.
reconcileAppWithPackage calls NewApp twice with the same Package - once to detect
whether the App needs updating (packageinstall.go:225), and once inside the
update closure (:234). Because the first call had already mutated the Package,
the second call appended pkgInstall.spec.values a second time, so the stored App
listed every PackageInstall values secret twice.
Signed-off-by: Sonny Yang <klzsysy@gmail.com>
klzsysy
force-pushed
the
fix/duplicate-values-from-secret
branch
from
September 16, 2026 12:07
4afd23d to
5b05de5
Compare
joaopapereira
requested changes
Sep 23, 2026
joaopapereira
left a comment
joaopapereira
left a comment
Member
There was a problem hiding this comment.
Some of this already has been addressed and merge in #1862, do you mind rebasing your PR?
Some other changes:
- Dedup is incomplete: annotation-driven paths (ExtHelmPathsFromSecretNameAnnKey, ytt PathsFrom / ExtYttDataValuesOverlaysAnnKey) still append without checking existing names. Either extend or document as out of scope.
- We should add a line on the Release Note section of the PR saying that we are not allowing duplicated secrets and that we are only taking into account the first occurrence of each secret
- Prefer aligning with #1862’s test style rather than duplicating fixtures.
Author
|
#1862 has already addressed the core issue I was concerned about，this PR can be closed for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
If the
secretReffield exists in thepackageinstallobject, the childappobject will redundantly reference these secrets.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
It also fixed an issue where duplicate references occurred in the
appobject whenpackageandpackageinstallobjects referenced the samesecret.Added relevant unit test cases.
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: