More expanded parameter overrides#8604
Conversation
There was a problem hiding this comment.
Thanks for the contribution @sigJoe! This is a nice follow-up to #7876. The relative path resolution and $include support are both sensible additions that improve the ergonomics of nested config files.
Implementation looks good:
- Clean use of
current_fileparameter to track context for relative path resolution file_path.resolve()properly normalizes paths- Reuses existing
seen_filesmechanism for recursion detection
Good error message for invalid$include` types
A few suggestions:
- Add a test for
$includecircular references - The existingtest_infinite_recursion_protectionuses list syntax. Would be good to add a similar test using$include:
def test_include_key_infinite_recursion(self):
mock_files = {
"A.yaml": "$include: file://B.yaml",
"B.yaml": "$include: file://A.yaml",
}
# ... should raise BadParameter with "Infinite recursion detected"- Consider adding type hint for
current_file:
def convert(self, values, param, ctx, seen_files=None, current_file: Optional[Path] = None):-
Edge case question: What happens if someone writes
$include: "NotAFileReference"(withoutfile://prefix)? It looks like it would fall through to the legacy parameter matching and fail with a potentially confusing error. Might be worth either:- Adding validation that
$includevalues must start withfile:// - Or just documenting the expected format
- Adding validation that
Overall this is solid work. Once the circular reference test is added, this should be good to go.
|
I added a test for
You are correct it would fall through to legacy parameter matching. It would throw an error Right now env: prod
$include:
- file://morestuff.yaml
- foo: baror in TOML: env = "prod"
"$include" = [
"file://morestuff.yaml",
{ foo = "bar" }
]It's very non-opinionated and flexible rather than being limited to file includes, although I could understand if you want this to be a bit more limited. |
|
Thanks for the follow-up @sigJoe! I think keeping $include generic makes sense here. The flexibility to merge inline parameters alongside file references is actually a nice feature, and restricting it to only file:// would be an artificial limitation. The current error message when someone passes an invalid value is clear enough - they'll see the list of valid formats. We can always tighten the validation later if users find it confusing, but I'd rather start permissive. One small suggestion for the docs/PR description: maybe clarify that $include is really "merge these parameters" rather than strictly "include these files" - that way users understand they can mix file refs with inline params if needed. Otherwise this looks good to merge. |
|
@bnusunny I updated the documentation in my first post to reflect the generic nature of Also I'd love to merge this, but as an external contributor I don't have write access to the repository. If you're happy with the current state, could you please handle the merge on your end? Thanks! |
Which issue(s) does this change fix?
This PR adds two enhancements to the parameter override file functionality introduced in #7876:
$includekey support - Dict-based parameter files (YAML/TOML) can now include other files using a special$includekey, providing the same composition capabilities that list-based files already hadWhy is this change necessary?
./config/a.yamland./config/b.yaml), it's unintuitive fora.yamlto referenceb.yamlasfile://config/b.yamlinstead of simplyfile://b.yaml. Relative path resolution makes file organization more natural and maintainable.How does it address the issue?
current_fileparameter to track the file being processed. When afile://reference is encountered, relative paths are resolved againstcurrent_file.parentinstead of the project root.$includekey: Added support for a special$includekey in dict-based parameter files that accepts either a single file path or a list of file paths. I picked this key name because it is not a valid cloudformation parameter so it should be fully backwards compatible. Another option was__includewhich didn't need to be quote-escaped in TOML but overall didn't look as pretty.What side effects does this change have?
This changes how
file://references work for parameter overrides in a way that is not backwards compatible. However, impact is limited since:Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Documentation
Old and busted
When using
file://to include external parameter overrides, there were limitations to include further levels of nested files. YAML had to be a list:TOML just didn't support it:
New hotness
The root object can now be a dict if you want using
$include:And TOML can now join the party too:
It's worth nothing that
$includeis a generic parameter inclusion that supports ALL parameter formats. It is not specifically limited tofile://references. For example, the following snippets are also valid: