Param sets override persisted --param values on upgrade#3600
Merged
Conversation
When upgrading, parameters previously set via --param were persisted on the installation and incorrectly suppressed resolution of values from a named parameter set. Parameter sets should take precedence over old persisted values, with only current-invocation --param flags able to override them. Fixes getporter#3599 Signed-off-by: Kim Christensen <kimworking@gmail.com>
In `installation apply`, parameters from the YAML are the user's current explicit desired state. The existing fix for getporter#3599 only treated CLI `--param` flags as current overrides, so param set values were incorrectly winning over YAML-specified parameters. Add value-source parameters from the installation YAML to lifecycleOpts.Params in ReconcileInstallation, so they are treated as current overrides in applyActionOptionsToInstallation. Signed-off-by: Kim Christensen <kimworking@gmail.com>
Replace string round-trip serialization in ReconcileInstallation with a CurrentParamOverrides field on BundleExecutionOptions. All parameter strategies (SourceValue, SourceSecret, etc.) are now handled correctly, and TrimSpace corruption of whitespace in values is eliminated. Signed-off-by: Kim Christensen <kimworking@gmail.com>
There was a problem hiding this comment.
Pull request overview
Fixes a parameter-resolution precedence bug where named parameter sets on porter upgrade were silently ignored for any parameter that had previously been set via --param. The fix distinguishes "current invocation overrides" (CLI --param flags and inline YAML params from the apply path) from "old persisted values" so that parameter sets correctly take precedence over the latter.
Changes:
- In
applyActionOptionsToInstallation, buildcliOverridesListcontaining only current-invocation overrides (passed toloadParameterSets), and update the final merge loop so old persisted values only fill gaps not covered by a parameter set. - Add
CurrentParamOverrides secrets.StrategyListtoBundleExecutionOptions, and haveReconcileInstallationpopulate it withopts.Installation.Parameters.Parametersinstead of round-tripping through string serialization. - Add a unit regression test plus two integration tests (with a small new test bundle) covering: param set overrides previous
--param, and--paramoverrides param set.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/porter/parameters.go | Reworks parameter set resolution and final merge to distinguish current overrides from old persisted values. |
| pkg/porter/lifecycle.go | Adds CurrentParamOverrides field to BundleExecutionOptions. |
| pkg/porter/reconcile.go | Passes inline YAML installation parameters through CurrentParamOverrides instead of serializing them. |
| pkg/porter/lifecycle_test.go | Unit regression test for the precedence fix. |
| tests/integration/upgrade_test.go | Integration tests covering both precedence directions. |
| tests/integration/testdata/bundles/bundle-with-param-set-upgrade/porter.yaml | New tiny test bundle echoing the name parameter for the integration tests. |
| tests/integration/testdata/bundles/bundle-with-param-set-upgrade/helpers.sh | Helper script invoked by the test bundle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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 does this change
When running
porter upgradewith a parameter set, Porter was ignoringthe parameter set values for any parameter that had been previously set
via
--paramin an earlier invocation. The old persisted value wouldsilently win, making it impossible to update a parameter's value by
switching to a parameter set.
The fix corrects the precedence order so that:
--paramCLI flags win over everythingThe same precedence is applied in the
porter apply(reconcile) path: parameters specified inline in the installation YAML are treated as current overrides and win over parameter sets.What issue does it fix
Closes #3599
Notes for the reviewer
Root cause was in
applyActionOptionsToInstallation(pkg/porter/parameters.go):loadParameterSets()receivedinst.Parameters.Parameters(all persisted params from previous runs) as itsoverridenParametersargument, causing it to skip resolving parameter set values for any param that had ever been set via--param.resolvedOverrides(resolvedinst.Parameters) on top of parameter set values, so old persisted values always won.The fix builds
cliOverridesListcontaining only params from the current invocation's--paramflags (step 3), and changes the merge loop (step 5) to only letresolvedOverrideswin when the key is a current CLI override or not already provided by a parameter set.For the
porter applypath,ReconcileInstallationnow setsCurrentParamOverridesonBundleExecutionOptionsdirectly (asecrets.StrategyList) instead of serializing inline params toname=valuestrings. This avoids a string round-trip throughParseVariableAssignmentsthat would have (a) skipped sanitizedSourceSecretparams on subsequent reconciles, (b) silently stripped leading/trailing whitespace from values, and (c) re-injected params removed from the bundle definition.Checklist