feat(loader): made interpolation after merge#644
feat(loader): made interpolation after merge#644idsulik wants to merge 7 commits intocompose-spec:mainfrom
Conversation
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
|
This was my initial intent during compose-go/v2 refactoring, but this breaks when |
@ndeloof could you please provide an example? |
|
A typical use: services:
prod:
image: a
dev:
image: a-dev
app:
extends: ${BASE:-prod} |
|
@ndeloof pushed changes to interpolate |
| return err | ||
| } | ||
|
|
||
| if !opts.SkipValidation { |
There was a problem hiding this comment.
while remove per-file validation ?
There was a problem hiding this comment.
accidentally removed, reverted it back
loader/loader.go
Outdated
|
|
||
| if opts.Interpolate != nil && !opts.SkipInterpolation { | ||
| interpolateOptsBeforeMerge = opts.Interpolate.Clone() | ||
| interpolateOptsBeforeMerge.KeysToInterpolate = []string{"include", "extends"} |
There was a problem hiding this comment.
AFAICT this would interpolate include but will skip include.path, won't it?
This deserves a dedicated test as there's a high risk to introduce a regression with such a change
I also think it would be safer to filter by path (services.*.extends.**) vs attribute name
There was a problem hiding this comment.
Yes, it will. Fixed it, now it checks if the Path contains a given part.
Before writing tests I want to know do we agree to do it this way?
tree/path.go
Outdated
| return strings.Split(string(p), pathSeparator) | ||
| } | ||
|
|
||
| func (p Path) ContainsPart(part string) bool { |
There was a problem hiding this comment.
This could work for this use case but is a bit fragile.
IMHO we could make this more deterministic if Matches adds support for "anything and child" ** syntax, so that you can use :
KeysToInterpolate := []{
"services.*.extends.**"
"includes.**"
}
There was a problem hiding this comment.
pushed changes, wrote tests, if you know any other cases that can be useful, let me know
|
|
||
| if opts.Interpolate != nil && !opts.SkipInterpolation { | ||
| cfg, err = interp.Interpolate(cfg, *opts.Interpolate) | ||
| cfg, err = interp.Interpolate(cfg, interpolateOptsBeforeMerge) |
There was a problem hiding this comment.
I wonder:
as extends will always require interpolation, wouldn't it be simpler to just force interpolation in ApplyExtends and ApplyInclude?
There was a problem hiding this comment.
- We call
ApplyExtendsonly ifSkipExtendsis nottruewhile interpolation happens always(the same for Include) - We call
ApplyIncludeafter interpolation andprocessors, if we start interpolate insideApplyIncludein can have consequences, I wanted to keep current logic as much as I can
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
|
Checking this PR again I went with an alternate implementation, reusing your test case: #651 wdyt ? Anyway, the current compose-spec schema declare types for many attributes which can actually be set by a variable, and validation fails. Would need to first fix the schema |
|
Closing as #651 has been merged |
What this PR does / why we need it:
Changed interpolation order, it used to interpolate for each config file separately, but now it interpolates after merge.
To be honest, I'm not sure it's a good idea, maybe it'll be better to add an option to control it, like
--interpolate-after-mergeWhich issue(s) this PR fixes:
Fixes docker/compose#11925