PoC: Global/Manifest-level Parameters #1538
Draft
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
Unit Test Results1 895 tests +3 1 881 ✅ - 10 27s ⏱️ ±0s For more details on these failures, see this check. Results for commit 11f54b8. ± Comparison against base commit bf613e7. ♻️ This comment has been updated with latest results. |
…e as a global parameter As configs are already aware of which environment they are for, this is easy to just make available like other global parameters. A dedicated parameter type would simplify access even further, as it can simply return the config's 'environment' value based on it's resolve context directly
|
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.
This is a PoC testing if it's possible to define parameters in the manifest and make them available to all configs - it is.
The middle commit making global parameters actually accessible is where concerns are cut heavily for the PoC:
This is memory inefficient and hacky.
In the spirit of making things explicit when it comes to Config YAMLs I'd suggest replacing the hack with a dedicated parameter type for accessing global parameters - and making these read from a single copy of the global param map when their ResolveValue is called.
If they are explicitly separated from general Parameters, there is also no risk of overlapping parameter names by accident - and no need to implement extra validation.
To cover #731 the environment a config is for, can similarly be made available as a global parameter available by default.
A dedicated parameter type for accessing global params, would make this even easier, as it can just return the string value - instead of wrapping it in an extra parameter as this PoC does.
Note on test failures:
Several tests assert that configs are created with expected parameters - the addition of the environment name parameter breaks these asserts. As that implementation should IMO not make it out of the PoC phase, the tests are not adapted.