bake: ignore unrelated fields when parsing compose files#3292
bake: ignore unrelated fields when parsing compose files#3292tonistiigi merged 2 commits intodocker:masterfrom
Conversation
Signed-off-by: CrazyMax <[email protected]>
github-actions
bot
added
area/bake
area/bake/compose
area/dependencies
area/tests
labels
| filteredServices[svcName] = map[string]any{} | ||
| } else if svcMap, ok := svc.(map[string]any); ok { | ||
| filteredService := make(map[string]any) | ||
| for _, svcField := range []string{"image", "build", "environment", "env_file"} { |
There was a problem hiding this comment.
AFAICT you don't need environment or env_file as those won't have any impact on the build context (they only apply on runtime environment)
There was a problem hiding this comment.
We need them to evaluate build arguments:
Lines 396 to 424 in a007368
If not set then this test fails:
Error: Not equal:
expected: map[string]*string{"CT_ECR":(*string)(0xc00060a510), "FOO":(*string)(0xc00060a520), "NODE_ENV":(*string)(0xc00060a530)}
actual : map[string]*string{"CT_ECR":(*string)(0xc00060a4a0)}
Diff:
--- Expected
+++ Actual
@@ -1,5 +1,3 @@
-(map[string]*string) (len=3) {
- (string) (len=6) "CT_ECR": (*string)((len=3) "foo"),
- (string) (len=3) "FOO": (*string)((len=10) "bsdf -csdf"),
- (string) (len=8) "NODE_ENV": (*string)((len=4) "test")
+(map[string]*string) (len=1) {
+ (string) (len=6) "CT_ECR": (*string)((len=3) "foo")
}
Test: TestEnv
--- FAIL: TestEnv (0.00s)
There was a problem hiding this comment.
There's a bug then. Runtime environment MUST not impact build environment; Sounds there's a confusion here with client environment (which is defined by .env file and --env-file flag in Compose) ... which is unfortunately a pretty common source of confusion 😭
There was a problem hiding this comment.
Argh yes seems it was a mistake, this was introduced in #905. What about .env then?
There was a problem hiding this comment.
For parity with Compose, .env should be loaded (from compose file's parent folder), and interpolated using os.Env
|
|
||
| return loader.ModelToProject(filtered, loader.ToOptions(&cfgDetails, append([]func(*loader.Options){func(options *loader.Options) { | ||
| options.SkipNormalization = true | ||
| options.Profiles = []string{"*"} |
There was a problem hiding this comment.
building with all profiles enabled is not consistent with Compose. Is this expected ?
There was a problem hiding this comment.
We allow all profiles because Bake doesn't support them: #1903. If we add something similar in our spec I think we could reconsider that. cc @colinhemmings
There was a problem hiding this comment.
In follow-up we could consider mapping compose profiles to bake groups and maybe defining a default profile that would be used if one is defined.
crazy-max
force-pushed
the
compose-filtered-spec
branch
from
05451de to
0ce459f
Compare
| Environment: envs, | ||
| } | ||
|
|
||
| raw, err := loader.LoadModelWithContext(context.Background(), cfgDetails, append([]func(*loader.Options){func(opts *loader.Options) { |
There was a problem hiding this comment.
We can do this as follow-up as not directly related but seems fine to pass in the right context from the command.
|
|
||
| return loader.ModelToProject(filtered, loader.ToOptions(&cfgDetails, append([]func(*loader.Options){func(options *loader.Options) { | ||
| options.SkipNormalization = true | ||
| options.Profiles = []string{"*"} |
There was a problem hiding this comment.
In follow-up we could consider mapping compose profiles to bake groups and maybe defining a default profile that would be used if one is defined.
Signed-off-by: CrazyMax <[email protected]>
crazy-max
force-pushed
the
compose-filtered-spec
branch
from
0ce459f to
c124b14
Compare
3 participants
fixes #3288