feat: implement import logic for Zarf Values #4427
Conversation
✅ Deploy Preview for zarf-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Wayne Starr <[email protected]>
Racer159
force-pushed
the
feat/add-values-import
branch
from
7f28fcf to
68adc37
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Wayne Starr <[email protected]>
|
Overall logic looks good, but worth noting that this PR depends on #4403 as without that change imports will not work since Zarf will look for a values directory. |
| |----------------------------|--------------------------|-------------| | ||
| | 'name'd Globals | `constants`, `variables` | These fields will match on `name` building up a map as components are processed (starting with the importing package definition). Any names that already exist will be skipped so the parent or an earlier component import will take precedence. | | ||
| | Un'name'd Primitive Arrays | `files.values` | The importing package definition's array will be appended to the end of the array from the imported component's package definition. Elements will be reappended regardless of whether they already exist in the array (since merge order impacts the final values). | | ||
| | Global Behavior | `files.schema` | This field will always take the value of the importing component's package definition (even if it is empty) | |
There was a problem hiding this comment.
I think there is an important decision point here around merging or not merging schemas. It seems like the allOf key could be used here to manage this.
There are a few non-ideal aspects of this strategy:
- There wouldn't be precedence, the parent wouldn't take priority.
- The value would have to be valid against every schema. This could lead to values that are impossible to define.
AnyOf, won't work since if any schemas don't define a certain property then it will pass.
Still AllOf could be the way to go. It's a more intuitive experience. Curious if you considered schema merging and ran into any hard blockers
There was a problem hiding this comment.
I did briefly consider schema merging - my thoughts though were to basically follow a similar tact to how values files are merged where parent keys would override child keys within the JSON.
This would need to be handled specific to the jsonschema format but for example:
$defs, patternProperties and properties would replace by key so if a parent redefined Constant here it would replace the whole definition under that key:
Line 3 in 092b721
Strings in the schema (like $id, additionalProperties, or description) could just take whatever is in the parent.
There could be some other things to think about in there too (like what happens if someone mixed jsonschema versions) but that shouldn't happen since there is really only one non-draft currently published and folks should just use 2020-12 - Zarf maybe could also just hard-require a specific version.
There was a problem hiding this comment.
considering the component-to-package relationship - have we considered namespacing values and nesting the schema?
There is a lot to consider here - so if it has been covered then the current discussion should continue. Otherwise I am curious if it could be a valid alternative to isolate merge collision issues.
Co-authored-by: Austin Abro <[email protected]> Signed-off-by: Wayne Starr <[email protected]>
brandtkeller
left a comment
brandtkeller
left a comment
There was a problem hiding this comment.
Testdata looks incomplete - or I am missing context.
There was a problem hiding this comment.
I believe we'll also want sourcePath/targetPath to work with imports with the parents sourcePath/targetPath taking priority
3 participants
Description
This PR implements import logic for Zarf Values.
Related Issue
Fixes #N/A
Checklist before merging