Merged
Conversation
carolynvs
added
the
pep003-advanced-dependencies
carolynvs
force-pushed
the
refactor-param-resolution
branch
from
12076a1 to
44e12c6
Compare
This was referenced Dec 5, 2022
carolynvs
force-pushed
the
refactor-param-resolution
branch
from
44e12c6 to
6166db0
Compare
carolynvs
force-pushed
the
refactor-param-resolution
branch
from
6166db0 to
22a8440
Compare
4 tasks
carolynvs
force-pushed
the
refactor-param-resolution
branch
from
e002990 to
c960ea4
Compare
The sanitizer assumed that all param overrides on an installation were hard coded values because that's what the CLI allows. With recent changes to support dependencies and workflows, internal parameter sets can now resolve secrets from other sources such as env, file, etc. Update the sanitizer to not assume they are all values that should be sanitized or edited. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Do not re-parse parameters set on an installation resource as --param flags and instead apply any --param flag overrides on top of parameters defined on the installation. This changes the current behavior which would remove any parameter overrides set on an installation resource when the overrides were not re-specified with --param. Example: ``` porter install mybuns --param logLevel=10 porter installation show mybuns -o yaml > mybuns.yaml porter installation apply mybuns.yaml ``` A few other improvements are: * We only resolve parameters once, regardless of code path. * We persist the installation from a dry-run after comparing state now and only persist if modified. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
carolynvs
force-pushed
the
refactor-param-resolution
branch
from
c960ea4 to
113a0e5
Compare
Avoid null reference exceptions by resolving and caching the bundle reference on the options, and not letting code directly access the "private" field that stored the resolved reference. Now we will resolve JIT if needed and cache the result for later use. Ideally we would never have put state on the options in the first place but here we are. At least this will avoid accidental null references. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
I've combined applying options to an installation with calling sanitize so that it's easier to remember and never get accidentally skipped. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
sgettys
approved these changes
Feb 1, 2023
VinozzZ
reviewed
Feb 1, 2023
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
…OptionsToInstallation Instead of setting the modified timestamp, tracking the bundle reference, etc individually in the install/upgrade/uninstall, etc actions, I've consolidated them into applyActionOptionsToInstallation, and ensure that we validate the installation after modifying it. Now we only need to validate in this function before saving the record. The unit tests for IsInstallationInSync made a lot of simplifying assumptions that let it do less test setup that I have addressed so that it's safe to call the function under test without calling it from inside the install/upgade/uninstall/invoke actions which handle prepping the installation and options first. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
bdegeeter
pushed a commit
to bdegeeter/porter
that referenced
this pull request
May 11, 2023
* Update sanitizer logic to allow for non-value based parameter overrides The sanitizer assumed that all param overrides on an installation were hard coded values because that's what the CLI allows. With recent changes to support dependencies and workflows, internal parameter sets can now resolve secrets from other sources such as env, file, etc. Update the sanitizer to not assume they are all values that should be sanitized or edited. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Improve applying installation parameters without reparsing as flags Do not re-parse parameters set on an installation resource as --param flags and instead apply any --param flag overrides on top of parameters defined on the installation. This changes the current behavior which would remove any parameter overrides set on an installation resource when the overrides were not re-specified with --param. Example: ``` porter install mybuns --param logLevel=10 porter installation show mybuns -o yaml > mybuns.yaml porter installation apply mybuns.yaml ``` A few other improvements are: * We only resolve parameters once, regardless of code path. * We persist the installation from a dry-run after comparing state now and only persist if modified. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Add guard rails to resolving the bundle reference Avoid null reference exceptions by resolving and caching the bundle reference on the options, and not letting code directly access the "private" field that stored the resolved reference. Now we will resolve JIT if needed and cache the result for later use. Ideally we would never have put state on the options in the first place but here we are. At least this will avoid accidental null references. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Always sanitize the installation file I've combined applying options to an installation with calling sanitize so that it's easier to remember and never get accidentally skipped. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Do not modify the param varible in the sanitizer for loop Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> * Consolidate modifying the installation based on opts into applyActionOptionsToInstallation Instead of setting the modified timestamp, tracking the bundle reference, etc individually in the install/upgrade/uninstall, etc actions, I've consolidated them into applyActionOptionsToInstallation, and ensure that we validate the installation after modifying it. Now we only need to validate in this function before saving the record. The unit tests for IsInstallationInSync made a lot of simplifying assumptions that let it do less test setup that I have addressed so that it's safe to call the function under test without calling it from inside the install/upgade/uninstall/invoke actions which handle prepping the installation and options first. Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com> --------- Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
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
4 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 does this change
Refactors how we resolve parameters so that it's easier to think about and maintain. Previously the logic for parsing and resolving parameters was spread out all over the place and was composed of many (unfortunately stateful) functions.
I've combined all that logic into a single function that resolves all user-defined parameters (defaults and parameter sources are still done separately since we need to be able to distinguish between the two, and the params are represented in each with different data types).
I had to cheat and use a bit of state to identify parameters that were set on the CLI or through a parameter set that really were parameters for a dependency of the bundle being executed. Dependencies v1 (the current implementation) allows the user to directly set a parameter for a dependency. The new implementation doesn't work that way (the parent bundle always manages passing data to the dependencies instead). Since eventually that code will go away and I really don't want to touch it, I went with the smallest possible fix in that area of the code.
This is refactoring for a) my happiness and that of future maintainers and b) to enable setting parameters differently for dependencies v2.
What issue does it fix
N/A, this is refactoring work to make PEP003 possible
Notes for the reviewer
N/A
Checklist
Reviewer Checklist