This repository has been archived by the owner on Dec 21, 2019. It is now read-only.
Refactor, document and test value merging logic #147
Merged
Conversation
This file contains 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
Changes the logic for merging context values to be unambiguous and easy to follow. * loadDefaultVars returns the default vars directly instead of performing merging in addition * all merging is performed in `mergeContextValues` using explicit explanations for every step of the merge. After this commit the order of merging goes from least to most "specific", please read the explanatory comments for more information. This relates to #142.
Introduces a test which will merge variables defined at every possible
layer together and ensure that the loaded context configuration is as
expected.
The test data provides an actual resource set template that can be
tested locally from a kontemplate source checkout:
kontemplate template context/testdata/merging/context.yaml --var cliVar=cliVar
|
I tested the branch. Its very understandable now from the comments, and the behavior is the one we were aiming for. Thanks for the fix!!! |
|
@Arttii Thank you! I'm going to wait for feedback from @merlineus, too. |
|
Yeah! |
|
@tazjin do you plan to release a minor version with this change? Maybe 1.6.1? |
|
@merlineus No, but I'm planning to release a major version with it (1.7.0) ;-) Waiting to see if @tommyJimmy87 (who filed the original issue) has more input before moving forward. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Please see the individual commit messages in this pull request for additional information.
This relates to the discussion in issue #142. I've published a pre-release build of this version here (tarball includes builds for Linux, Darwin, FreeBSD and Windows).
cc: @merlineus @Arttii @tommyJimmy87