Skip to content

Commit a7b992b

Browse files
ostermanclaude
osterman
and
claude
committed
fix(schema): decode with: into Build/Run/Push/Inspect for custom commands
Fixes #2876. A custom command's `type: container` step with a `with:` block (engine, driver, cache, tags, etc.) silently dropped everything, falling back to a bare `docker build -f Dockerfile .`, when loaded from a commands.yaml merged into atmos.yaml's Viper config tree. Root cause: `with:` is polymorphic -- decoded into Build/Run/Push/Inspect for `type: container` steps, or the generic With map otherwise -- but that promotion lives entirely in Task.UnmarshalYAML/WorkflowStep.UnmarshalYAML (go-yaml's yaml.Unmarshaler interface), invoked only when something calls yaml.Node.Decode directly (e.g. standalone workflows/*.yaml files via pkg/utils.UnmarshalYAMLFromFile). Custom commands merged into atmos.yaml decode via Viper's mapstructure pipeline (TasksDecodeHook -> decodeTaskFromMap), which never invokes yaml.Unmarshaler and had no equivalent promotion, so `with:` only ever reached the raw generic map. decodeTaskFromMap now pulls `with:` out before the mapstructure decode and replays the same polymorphic decode via decodeStepWith, round-tripping the value through YAML so both code paths share one implementation and can't drift apart. Reproduced through the real production paths per the bug report's request: config loaded via InitCliConfig (pkg/config), and the full custom command executed via RootCmd through a fake logging docker executable (cmd/) -- not by manually constructing schema.Task/WorkflowStep/ContainerBuildStep literals, which would have bypassed the actual decode bug. Added a complementary test proving workflow-file and custom-command steps decode with: identically, per the report's public-contract requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 14c9abc commit a7b992b

4 files changed

Lines changed: 342 additions & 0 deletions

File tree

‎cmd/custom_command_container_build_test.go‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
package cmd
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
12+
cfg "github.com/cloudposse/atmos/pkg/config"
13+
"github.com/cloudposse/atmos/pkg/schema"
14+
"github.com/cloudposse/atmos/tests/testhelpers"
15+
)
16+
17+
// TestCustomCommandContainerBuildPassesWithBlockToDocker reproduces
18+
// https://github.com/cloudposse/atmos/issues/2876 end to end: a
19+
// `.atmos.d/commands.yaml`-style custom command with a `type: container,
20+
// action: build` step and a `with:` block must invoke docker with the full
21+
// configured Buildx engine, builder, cache, tags, dockerfile, and context --
22+
// not silently fall back to `docker build -f Dockerfile .`.
23+
//
24+
// This writes real config to disk, loads it through cfg.InitCliConfig (the
25+
// same production config-loading path `atmos` itself uses), registers it via
26+
// processCustomCommands, and invokes the resulting custom command
27+
// through RootCmd.Execute() exactly as a user would from the shell. A fake,
28+
// logging `docker` executable on PATH (testhelpers.InstallFakeContainerRuntime)
29+
// captures the real argv Atmos emits, so this exercises the real command
30+
// executor rather than manually constructing schema.Task/WorkflowStep/
31+
// ContainerBuildStep values in Go.
32+
func TestCustomCommandContainerBuildPassesWithBlockToDocker(t *testing.T) {
33+
_ = NewTestKit(t)
34+
35+
tempDir := t.TempDir()
36+
appDir := filepath.Join(tempDir, "app")
37+
require.NoError(t, os.MkdirAll(appDir, 0o755))
38+
require.NoError(t, os.WriteFile(filepath.Join(appDir, "Dockerfile"), []byte("FROM scratch\n"), 0o644))
39+
40+
atmosYAML := `
41+
base_path: "."
42+
commands:
43+
- name: test-container-build-with-block
44+
description: Build the application image
45+
steps:
46+
- name: build
47+
type: container
48+
action: build
49+
provider: docker
50+
with:
51+
engine: buildx
52+
context: app
53+
dockerfile: Dockerfile
54+
tags:
55+
- "example.invalid/demo:sha-test"
56+
driver:
57+
name: atmos-native-ci
58+
provider: docker-container
59+
opts:
60+
image: mirror.gcr.io/moby/buildkit:buildx-stable-1
61+
cache:
62+
from:
63+
- type: registry
64+
ref: "example.invalid/demo:buildcache"
65+
to:
66+
- type: registry
67+
ref: "example.invalid/demo:buildcache"
68+
mode: max
69+
`
70+
require.NoError(t, os.WriteFile(filepath.Join(tempDir, "atmos.yaml"), []byte(atmosYAML), 0o644))
71+
72+
t.Setenv("ATMOS_CLI_CONFIG_PATH", tempDir)
73+
t.Setenv("ATMOS_BASE_PATH", tempDir)
74+
t.Chdir(tempDir)
75+
76+
argsPath := filepath.Join(t.TempDir(), "docker-args.log")
77+
t.Setenv("ATMOS_FAKE_RUNTIME_ARGS_FILE", argsPath)
78+
testhelpers.InstallFakeContainerRuntime(t, testhelpers.FakeContainerRuntimeSpec{
79+
Name: "docker",
80+
Mode: testhelpers.FakeContainerRuntimeStep,
81+
})
82+
83+
atmosConfig, err := cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false)
84+
require.NoError(t, err)
85+
86+
require.NoError(t, processCustomCommands(atmosConfig, atmosConfig.Commands, RootCmd))
87+
88+
RootCmd.SetArgs([]string{"test-container-build-with-block"})
89+
require.NoError(t, RootCmd.Execute())
90+
91+
content, err := os.ReadFile(argsPath)
92+
require.NoError(t, err, "the fake docker executable must have been invoked at least once")
93+
lines := strings.Split(strings.TrimSpace(string(content)), "\n")
94+
95+
var buildLine string
96+
for _, line := range lines {
97+
fields := strings.Split(line, "\t")
98+
if len(fields) > 1 && fields[0] == "buildx" && fields[1] == "build" {
99+
buildLine = line
100+
break
101+
}
102+
}
103+
require.NotEmpty(t, buildLine,
104+
"expected a `docker buildx build ...` invocation; got invocations: %v", lines)
105+
106+
fields := strings.Split(buildLine, "\t")
107+
assert.Contains(t, fields, "--builder", "configured Buildx driver must be applied")
108+
assert.Contains(t, fields, "atmos-native-ci")
109+
assert.Contains(t, fields, "--cache-from", "configured registry cache-from must be applied")
110+
assert.Contains(t, fields, "--cache-to", "configured registry cache-to must be applied")
111+
assert.Contains(t, fields, "-t", "configured tag must be applied")
112+
assert.Contains(t, fields, "example.invalid/demo:sha-test")
113+
assert.Contains(t, fields, "-f", "configured Dockerfile must be applied")
114+
assert.Contains(t, fields, "Dockerfile")
115+
assert.Contains(t, fields, "app", "configured context must be applied")
116+
117+
// The exact bug report's symptom: Atmos must not fall back to a bare,
118+
// unconfigured `docker build -f Dockerfile .`.
119+
for _, line := range lines {
120+
assert.NotEqual(t, "build\t-f\tDockerfile\t.", line,
121+
"must not silently fall back to a bare, unconfigured docker build")
122+
}
123+
}

‎pkg/config/custom_command_container_with_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package config
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
11+
"github.com/cloudposse/atmos/pkg/schema"
12+
)
13+
14+
// TestCustomCommandContainerBuildStepDecodesWithBlock reproduces
15+
// https://github.com/cloudposse/atmos/issues/2876: a custom command's
16+
// `type: container, action: build` step with a `with:` block, defined in
17+
// commands.yaml exactly as a user would write it, must decode `with:` into
18+
// the typed `Build` struct -- not silently drop it.
19+
//
20+
// This loads the config through the real production path (InitCliConfig,
21+
// which drives Viper's merge + mapstructure decode via atmosDecodeHook/
22+
// TasksDecodeHook), not by constructing a schema.Task/ContainerBuildStep
23+
// literal in Go. Workflow files loaded via pkg/utils.UnmarshalYAMLFromFile
24+
// go through go-yaml's node.Decode, which invokes Task.UnmarshalYAML and
25+
// correctly promotes `with:` into Build/Run/Push/Inspect. Custom commands
26+
// merged into atmos.yaml's Viper config tree never hit that yaml.Unmarshaler
27+
// method -- they decode via TasksDecodeHook -> decodeTaskFromMap, a separate
28+
// mapstructure-based path that (before this fix) had no equivalent
29+
// promotion, leaving Build nil regardless of what `with:` contained.
30+
func TestCustomCommandContainerBuildStepDecodesWithBlock(t *testing.T) {
31+
setupTestAdapters()
32+
33+
tempDir := t.TempDir()
34+
atmosYAML := `
35+
base_path: "."
36+
commands:
37+
- name: build
38+
description: Build the application image
39+
steps:
40+
- name: build
41+
type: container
42+
action: build
43+
provider: docker
44+
with:
45+
engine: buildx
46+
context: app
47+
dockerfile: Dockerfile
48+
tags:
49+
- "example.invalid/demo:sha-test"
50+
driver:
51+
name: atmos-native-ci
52+
provider: docker-container
53+
opts:
54+
image: mirror.gcr.io/moby/buildkit:buildx-stable-1
55+
cache:
56+
from:
57+
- type: registry
58+
ref: "example.invalid/demo:buildcache"
59+
to:
60+
- type: registry
61+
ref: "example.invalid/demo:buildcache"
62+
mode: max
63+
`
64+
require.NoError(t, os.WriteFile(filepath.Join(tempDir, "atmos.yaml"), []byte(atmosYAML), 0o644))
65+
66+
t.Chdir(tempDir)
67+
configInfo := schema.ConfigAndStacksInfo{
68+
AtmosBasePath: tempDir,
69+
AtmosCliConfigPath: filepath.Join(tempDir, "atmos.yaml"),
70+
}
71+
cfg, err := InitCliConfig(configInfo, false)
72+
require.NoError(t, err)
73+
74+
require.Len(t, cfg.Commands, 1)
75+
require.Len(t, cfg.Commands[0].Steps, 1)
76+
step := cfg.Commands[0].Steps[0]
77+
78+
require.NotNil(t, step.Build,
79+
"with: block must decode into the typed Build struct, not be silently dropped")
80+
assert.Equal(t, "buildx", step.Build.Engine)
81+
assert.Equal(t, "app", step.Build.Context)
82+
assert.Equal(t, "Dockerfile", step.Build.Dockerfile)
83+
assert.Equal(t, []string{"example.invalid/demo:sha-test"}, step.Build.Tags)
84+
require.NotNil(t, step.Build.Driver)
85+
assert.Equal(t, "atmos-native-ci", step.Build.Driver.Name)
86+
assert.Equal(t, "docker-container", step.Build.Driver.Provider)
87+
assert.Equal(t, "mirror.gcr.io/moby/buildkit:buildx-stable-1", step.Build.Driver.Opts["image"])
88+
require.NotNil(t, step.Build.Cache)
89+
require.Len(t, step.Build.Cache.From, 1)
90+
assert.Equal(t, "example.invalid/demo:buildcache", step.Build.Cache.From[0]["ref"])
91+
require.Len(t, step.Build.Cache.To, 1)
92+
assert.Equal(t, "max", step.Build.Cache.To[0]["mode"])
93+
}

‎pkg/schema/task.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -782,6 +782,22 @@ func decodeTaskFromMap(m map[string]any, index int) (Task, error) {
782782
if err != nil {
783783
return Task{}, fmt.Errorf("failed to decode task steps at index %d: %w", index, err)
784784
}
785+
786+
// `with:` is polymorphic -- decoded into Build/Run/Push/Inspect for
787+
// `type: container` steps, or the generic With map otherwise -- exactly
788+
// as Task.UnmarshalYAML's applyStepPolymorphicNodes handles it for a step
789+
// loaded directly from a workflow YAML file. This mapstructure-based
790+
// decode path (used for custom commands merged into atmos.yaml's Viper
791+
// config tree) never invokes that yaml.Unmarshaler method, so it must
792+
// pull `with:` out and replay the same polymorphic decode explicitly.
793+
// Without this, `with:` only reaches the plain mapstructure struct
794+
// decode below (which has no notion of the polymorphism) and
795+
// Build/Run/Push/Inspect stay nil regardless of what `with:` contains.
796+
withValue, hasWith := m["with"]
797+
if hasWith {
798+
delete(m, "with")
799+
}
800+
785801
decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
786802
Result: &task,
787803
TagName: "mapstructure",
@@ -803,9 +819,42 @@ func decodeTaskFromMap(m map[string]any, index int) (Task, error) {
803819
if task.Type == "" {
804820
task.Type = TaskTypeShell
805821
}
822+
823+
if hasWith {
824+
if err := decodeStepWithFromMapValue(withValue, task.Type, task.Action, &stepPolyTargets{
825+
generic: &task.With,
826+
container: containerActionTargets{Build: &task.Build, Run: &task.Run, Push: &task.Push, Inspect: &task.Inspect},
827+
}); err != nil {
828+
return Task{}, fmt.Errorf("failed to decode task with-block at index %d: %w", index, err)
829+
}
830+
}
831+
806832
return task, nil
807833
}
808834

835+
// decodeStepWithFromMapValue applies the same `with:` polymorphic decode as
836+
// decodeStepWith (see workflow.go), but starting from a plain Go value
837+
// (as produced by mapstructure/Viper's merged config tree) instead of a
838+
// *yaml.Node. It round-trips the value through YAML so both the direct
839+
// workflow-file path (yaml.Node.Decode -> UnmarshalYAML) and this
840+
// mapstructure-based path share one implementation of the with:->Build/
841+
// Run/Push/Inspect promotion and can't drift apart.
842+
func decodeStepWithFromMapValue(withValue any, stepType, action string, t *stepPolyTargets) error {
843+
withBytes, err := yaml.Marshal(withValue)
844+
if err != nil {
845+
return err
846+
}
847+
var doc yaml.Node
848+
if err := yaml.Unmarshal(withBytes, &doc); err != nil {
849+
return err
850+
}
851+
node := &doc
852+
if doc.Kind == yaml.DocumentNode && len(doc.Content) == 1 {
853+
node = doc.Content[0]
854+
}
855+
return decodeStepWith(node, stepType, action, t)
856+
}
857+
809858
func normalizeTaskStepsMap(m map[string]any) (map[string]any, error) {
810859
steps, ok := m[taskMapKeySteps]
811860
if !ok {

‎pkg/schema/task_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,6 +1193,83 @@ func TestSliceToAny_ConvertsTypedSlice(t *testing.T) {
11931193
assert.Equal(t, typed[1], slice[1])
11941194
}
11951195

1196+
// containerStepWithBlockYAML is a single `type: container, action: build`
1197+
// step with a full `with:` block, in the exact shape a user writes it in
1198+
// either a workflow file or a commands.yaml custom command.
1199+
const containerStepWithBlockYAML = `
1200+
- name: build
1201+
type: container
1202+
action: build
1203+
provider: docker
1204+
with:
1205+
engine: buildx
1206+
context: app
1207+
dockerfile: Dockerfile
1208+
tags:
1209+
- "example.invalid/demo:sha-test"
1210+
driver:
1211+
name: atmos-native-ci
1212+
provider: docker-container
1213+
cache:
1214+
from:
1215+
- type: registry
1216+
ref: "example.invalid/demo:buildcache"
1217+
to:
1218+
- type: registry
1219+
ref: "example.invalid/demo:buildcache"
1220+
mode: max
1221+
`
1222+
1223+
// TestContainerStepWithBlock_WorkflowAndCustomCommandDecodeIdentically confirms
1224+
// the public contract cited by https://github.com/cloudposse/atmos/issues/2876:
1225+
// a `type: container` step's `with:` block must decode into the same Build
1226+
// struct whether it's parsed as a standalone workflow file (direct
1227+
// yaml.Node.Decode -> Task.UnmarshalYAML) or as a commands.yaml custom
1228+
// command merged into Viper's config tree (mapstructure -> TasksDecodeHook ->
1229+
// decodeTaskFromMap). Both call paths are exercised here exactly as their
1230+
// real callers do: yaml.Unmarshal for the workflow-file path (see
1231+
// pkg/utils.UnmarshalYAMLFromFile, used to load workflows/*.yaml), and
1232+
// mapstructure.NewDecoder with TasksDecodeHook for the Viper path (see
1233+
// pkg/config's atmosDecodeHook, used to decode atmos.yaml's `commands:`).
1234+
func TestContainerStepWithBlock_WorkflowAndCustomCommandDecodeIdentically(t *testing.T) {
1235+
// Workflow-file path: direct YAML decode, invoking Task.UnmarshalYAML.
1236+
var fromYAML Tasks
1237+
require.NoError(t, yaml.Unmarshal([]byte(containerStepWithBlockYAML), &fromYAML))
1238+
require.Len(t, fromYAML, 1)
1239+
1240+
// Custom-command / Viper path: decode into a generic tree first (as Viper
1241+
// does when it reads the YAML file), then mapstructure-decode that tree
1242+
// into Tasks via the real TasksDecodeHook, mirroring atmosDecodeHook.
1243+
var generic []any
1244+
require.NoError(t, yaml.Unmarshal([]byte(containerStepWithBlockYAML), &generic))
1245+
1246+
var fromMapstructure Tasks
1247+
decoder, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
1248+
Result: &fromMapstructure,
1249+
TagName: "mapstructure",
1250+
WeaklyTypedInput: true,
1251+
DecodeHook: mapstructure.ComposeDecodeHookFunc(
1252+
mapstructure.StringToTimeDurationHookFunc(),
1253+
ConditionDecodeHook(),
1254+
WorkflowStepDecodeHook(),
1255+
TasksDecodeHook(),
1256+
),
1257+
})
1258+
require.NoError(t, err)
1259+
require.NoError(t, decoder.Decode(generic))
1260+
require.Len(t, fromMapstructure, 1)
1261+
1262+
workflowStep := fromYAML[0]
1263+
commandStep := fromMapstructure[0]
1264+
1265+
require.NotNil(t, workflowStep.Build, "workflow-file path must decode with: into Build")
1266+
require.NotNil(t, commandStep.Build, "custom-command path must decode with: into Build")
1267+
assert.Equal(t, workflowStep.Build, commandStep.Build,
1268+
"a type: container step's with: block must decode identically for workflow files and custom commands")
1269+
assert.Nil(t, workflowStep.With, "with: must not also leak into the generic With map for a container step")
1270+
assert.Nil(t, commandStep.With, "with: must not also leak into the generic With map for a container step")
1271+
}
1272+
11961273
// TestDecodeTaskItem_MapAnyAny verifies the default branch of decodeTaskItem that
11971274
// stringifies a map[any]any item before decoding it as a task map.
11981275
func TestDecodeTaskItem_MapAnyAny(t *testing.T) {

0 commit comments

Comments
 (0)