diff --git a/cmd/init/init.go b/cmd/init/init.go index 6a842604fed..aef69e98fd4 100644 --- a/cmd/init/init.go +++ b/cmd/init/init.go @@ -18,6 +18,7 @@ import ( gen "github.com/cloudposse/atmos/pkg/generator" "github.com/cloudposse/atmos/pkg/generator/merge" "github.com/cloudposse/atmos/pkg/generator/source" + "github.com/cloudposse/atmos/pkg/generator/storage" "github.com/cloudposse/atmos/pkg/generator/templates" "github.com/cloudposse/atmos/pkg/generator/ui" "github.com/cloudposse/atmos/pkg/hooks" @@ -69,8 +70,20 @@ If no target directory is specified, you will be prompted for one.`, force := v.GetBool("force") update := v.GetBool("update") baseRef := v.GetString("base-ref") - if update { - baseRef = defaultBaseRef(baseRef) + // Only pre-resolve here when target is already the real, final + // target directory (i.e. it was given positionally). When target is + // "" the interactive flow still has to prompt for one -- see + // resolveInteractiveInitBaseRef, which resolves the base ref itself + // once the actual directory is known. Resolving against "" here + // would read .atmos/init/metadata.yaml from the wrong (empty/cwd) + // path and permanently overwrite baseRef with "HEAD", discarding any + // pin at the directory the user goes on to pick. + if update && target != "" { + resolvedBaseRef, err := defaultBaseRef(baseRef, target) + if err != nil { + return err + } + baseRef = resolvedBaseRef } sourceOverride := v.GetString("source-override") ref := v.GetString("ref") @@ -131,7 +144,7 @@ func init() { flags.WithBoolFlag("no-git", "", false, "Do not initialize a git repository"), flags.WithStringFlag("merge-driver", "", "auto", "Merge driver for --update: auto (YAML-aware for .yaml/.yml, text otherwise, default), text (force line-oriented text merge for every file)"), flags.WithValidValues("merge-driver", "auto", "text"), - flags.WithStringFlag("merge-strategy", "", "manual", "Conflict resolution strategy for --update: manual (surface conflicts, default), ours (keep your version), theirs (use the template's version)"), + flags.WithStringFlag("merge-strategy", "", "", "Conflict resolution strategy for --update: manual (surface conflicts, default; theirs if --force is set), ours (keep your version), theirs (use the template's version)"), flags.WithValidValues("merge-strategy", "manual", "ours", "theirs"), // Skip scaffold hooks at runtime, mirroring `terraform`'s --skip-hooks // (see cmd/terraform/flags.go): --skip-hooks (no value) skips all @@ -256,7 +269,7 @@ func executeInit(_ context.Context, opts *initOptions) error { return err } - conflictStrategy, err := merge.ParseConflictStrategy(opts.mergeStrategy) + conflictStrategy, err := merge.ResolveConflictStrategy(opts.mergeStrategy, opts.force, opts.update) if err != nil { return err } @@ -312,12 +325,22 @@ func maybeInitGeneratedProjectGit(targetDir string, selectedConfig *templates.Co if !opts.git || targetDir == "" { return nil } - _, _, err := gen.InitGitRepository(gen.InitGitOptions{ + _, headSHA, err := gen.InitGitRepository(gen.InitGitOptions{ TargetPath: targetDir, TemplateName: selectedConfig.Name, TemplateVersion: selectedConfig.Version, }) - return err + if err != nil { + return err + } + // No-op when headSHA is empty (targetDir was already a git repo; see + // gen.PinInitialBaseRefForInit). + return gen.PinInitialBaseRefForInit( + targetDir, headSHA, + gen.WithTemplateName(selectedConfig.Name), + gen.WithTemplateVersion(selectedConfig.Version), + gen.WithSource(selectedConfig.Source), + ) } // resolveTargetDir converts a target directory to an absolute path if provided. @@ -344,7 +367,7 @@ func createInitUI() (*ui.InitUI, error) { } // selectTemplate handles template selection, either from argument or interactively. -func selectTemplate(templateName string, interactive bool, initUI *ui.InitUI, configs map[string]templates.Configuration, ref string) (templates.Configuration, error) { +func selectTemplate(templateName string, interactive bool, initUI InitUI, configs map[string]templates.Configuration, ref string) (templates.Configuration, error) { // If template name is provided, use it directly. if templateName != "" { config, exists := configs[templateName] @@ -374,7 +397,7 @@ func selectTemplate(templateName string, interactive bool, initUI *ui.InitUI, co } // runInitExecution executes the init with the selected template and target directory. -func runInitExecution(initUI *ui.InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { +func runInitExecution(initUI InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { // If target directory is empty, use interactive flow; otherwise use normal Execute. if opts.targetDir == "" { return runInitInteractiveFlow(initUI, selectedConfig, opts) @@ -385,24 +408,90 @@ func runInitExecution(initUI *ui.InitUI, selectedConfig *templates.Configuration // runInitInteractiveFlow handles init when no target directory was provided, // prompting the user for one (and optionally offering a 3-way-merge update // instead of failing when it already exists and is non-empty). -func runInitInteractiveFlow(initUI *ui.InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { +func runInitInteractiveFlow(initUI InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { if !opts.interactive { return "", fmt.Errorf("%w: target directory is required in non-interactive mode", errUtils.ErrInitialization) } - targetDir, err := initUI.ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, "", opts.force, opts.update, !opts.interactive, opts.baseRef, opts.templateVars) - if offer, retryBaseRef := shouldOfferUpdate(err, opts); offer { - if confirmed, cErr := initUI.ConfirmUpdateInstead(targetDir); cErr == nil && confirmed { - return initUI.ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, targetDir, opts.force, true, !opts.interactive, retryBaseRef, opts.templateVars) + + resolved, err := resolveInteractiveInitBaseRef(initUI, selectedConfig, opts) + if err != nil { + return resolved.targetDir, err + } + + finalTargetDir, err := initUI.ExecuteWithInteractiveFlowAndBaseRefResult( + selectedConfig, resolved.targetDir, opts.force, opts.update, resolved.useDefaults, resolved.baseRef, resolved.templateValues, + ) + offer, retryBaseRef, offerErr := shouldOfferUpdate(err, opts, finalTargetDir) + if offerErr != nil { + return finalTargetDir, offerErr + } + if offer { + if confirmed, cErr := initUI.ConfirmUpdateInstead(finalTargetDir); cErr == nil && confirmed { + return initUI.ExecuteWithInteractiveFlowAndBaseRefResult( + selectedConfig, finalTargetDir, opts.force, true, resolved.useDefaults, retryBaseRef, resolved.templateValues, + ) } } - return targetDir, err + return finalTargetDir, err +} + +// interactiveInitBaseRef bundles resolveInteractiveInitBaseRef's results +// (grouped into a struct, rather than five separate return values, to stay +// under revive's function-result-limit). +type interactiveInitBaseRef struct { + targetDir string + baseRef string + templateValues map[string]interface{} + useDefaults bool +} + +// resolveInteractiveInitBaseRef resolves the --update merge base ref for the +// no-positional-target interactive flow, mirroring cmd/scaffold's +// resolveInteractiveBaseRef. --base-ref's default (the pinned ref from +// .atmos/init/metadata.yaml, see defaultBaseRef) can only be looked up once +// the real target directory is known, but in this flow that directory +// doesn't exist until the interactive prompt below picks one -- so for +// --update, resolve the target directory first (initUI.ResolveTargetPath +// runs the same prompt/setup-form logic +// ExecuteWithInteractiveFlowAndBaseRefResult would, and is a no-op once +// targetDir is non-empty), then resolve the base ref against it, and +// finally hand both back to the caller's +// ExecuteWithInteractiveFlowAndBaseRefResult call -- which skips prompting +// again since targetDir is already set. +// +// Without --update the base ref is unused (ExecuteWithDelimiters only sets +// up git storage when update is true), so this is a no-op passthrough that +// still lets the interactive flow prompt for the target itself. +func resolveInteractiveInitBaseRef( + initUI InitUI, + selectedConfig *templates.Configuration, + opts *initOptions, +) (interactiveInitBaseRef, error) { + if !opts.update { + return interactiveInitBaseRef{baseRef: opts.baseRef, templateValues: opts.templateVars, useDefaults: !opts.interactive}, nil + } + + targetDir, templateValues, useDefaults, err := initUI.ResolveTargetPath(selectedConfig, "", opts.update, !opts.interactive, opts.templateVars) + if err != nil { + return interactiveInitBaseRef{targetDir: targetDir}, err + } + + baseRef, err := defaultBaseRef(opts.baseRef, targetDir) + if err != nil { + return interactiveInitBaseRef{targetDir: targetDir}, err + } + return interactiveInitBaseRef{targetDir: targetDir, baseRef: baseRef, templateValues: templateValues, useDefaults: useDefaults}, nil } // runInitTargetedFlow handles init when a target directory was provided // (offering the same 3-way-merge update fallback as the interactive flow). -func runInitTargetedFlow(initUI *ui.InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { +func runInitTargetedFlow(initUI InitUI, selectedConfig *templates.Configuration, opts *initOptions) (string, error) { err := initUI.ExecuteWithBaseRef(selectedConfig, opts.targetDir, opts.force, opts.update, !opts.interactive, opts.baseRef, opts.templateVars) - if offer, retryBaseRef := shouldOfferUpdate(err, opts); offer { + offer, retryBaseRef, offerErr := shouldOfferUpdate(err, opts, opts.targetDir) + if offerErr != nil { + return opts.targetDir, offerErr + } + if offer { if confirmed, cErr := initUI.ConfirmUpdateInstead(opts.targetDir); cErr == nil && confirmed { return opts.targetDir, initUI.ExecuteWithBaseRef(selectedConfig, opts.targetDir, opts.force, true, !opts.interactive, retryBaseRef, opts.templateVars) } @@ -413,27 +502,34 @@ func runInitTargetedFlow(initUI *ui.InitUI, selectedConfig *templates.Configurat // shouldOfferUpdate decides whether to offer a 3-way-merge update instead of // failing outright on a non-empty target directory: only when the failure is // exactly that, the caller isn't already using --force/--update, and a real -// terminal is available to prompt on. Returns the base ref to retry with -// (the caller's --base-ref, defaulting to HEAD) alongside the decision. -func shouldOfferUpdate(err error, opts *initOptions) (bool, string) { +// terminal is available to prompt on. TargetDir must be the actual, final +// target directory generation just ran against (not opts.targetDir, which is +// the raw positional arg and can be "" when the interactive flow picked the +// real directory itself -- see resolveInteractiveInitBaseRef). Returns the +// base ref to retry with (the caller's --base-ref, defaulting to HEAD or a +// pinned metadata ref) alongside the decision. +func shouldOfferUpdate(err error, opts *initOptions, targetDir string) (offer bool, baseRef string, resolveErr error) { if err == nil || opts.force || opts.update || !opts.interactive { - return false, "" + return false, "", nil } if !errors.Is(err, errUtils.ErrTargetDirectoryNotEmpty) { - return false, "" + return false, "", nil + } + resolvedBaseRef, resolveErr := defaultBaseRef(opts.baseRef, targetDir) + if resolveErr != nil { + return false, "", resolveErr } - return true, defaultBaseRef(opts.baseRef) + return true, resolvedBaseRef, nil } -// defaultBaseRef fills in HEAD as the 3-way-merge base ref when the caller -// didn't supply one. Without this, --update silently sets up no git storage -// at all (ExecuteWithDelimiters only calls SetupGitStorage when baseRef is -// non-empty) and every file fails with an opaque "three-way merge failed" -- -// HEAD is the obvious default since `atmos init --git` always creates an -// initial commit. -func defaultBaseRef(baseRef string) string { - if baseRef == "" { - return "HEAD" - } - return baseRef +// defaultBaseRef resolves init's --update base ref against this target's own +// pinned metadata (.atmos/init/metadata.yaml, written by +// gen.PinInitialBaseRefForInit). See gen.ResolveDefaultBaseRef's doc for the +// full rationale -- that function is shared with cmd/scaffold's equivalent +// so the two commands' base-ref resolution can't drift apart again (as it +// did before: this command shipped with the same silent-overwrite bug +// cmd/scaffold fixed, because the fix lived only in cmd/scaffold and was +// never ported here). +func defaultBaseRef(baseRef, targetDir string) (string, error) { + return gen.ResolveDefaultBaseRef(baseRef, targetDir, storage.InitMetadataPath(targetDir)) } diff --git a/cmd/init/init_mock_test.go b/cmd/init/init_mock_test.go new file mode 100644 index 00000000000..fb2efca1f5d --- /dev/null +++ b/cmd/init/init_mock_test.go @@ -0,0 +1,317 @@ +package initcmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/generator/storage" + "github.com/cloudposse/atmos/pkg/generator/templates" +) + +// These tests exercise the retry-as-update confirmation flow in +// runInitTargetedFlow/runInitInteractiveFlow, and resolveInteractiveInitBaseRef's +// --update branch, using a mocked InitUI. That flow needs a real TTY and a +// pre-populated non-empty target directory to reach via integration tests, so +// it was previously only covered indirectly (or not at all for the "user +// declines"/error branches). Mocking InitUI lets each branch be asserted +// deterministically. Mirrors cmd/scaffold/scaffold_mock_test.go, which solves +// the same problem for the sibling command. + +func TestRunInitTargetedFlow_OffersUpdateAndRetriesOnConfirm(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + opts := &initOptions{ + targetDir: "/tmp/target", + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + + gomock.InOrder( + mockUI.EXPECT(). + ExecuteWithBaseRef(selectedConfig, "/tmp/target", false, false, false, "", opts.templateVars). + Return(errUtils.ErrTargetDirectoryNotEmpty), + mockUI.EXPECT(). + ConfirmUpdateInstead("/tmp/target"). + Return(true, nil), + mockUI.EXPECT(). + ExecuteWithBaseRef(selectedConfig, "/tmp/target", false, true, false, "HEAD", opts.templateVars). + Return(nil), + ) + + targetDir, err := runInitTargetedFlow(mockUI, selectedConfig, opts) + + require.NoError(t, err) + assert.Equal(t, "/tmp/target", targetDir) +} + +func TestRunInitTargetedFlow_DeclinesUpdateOffer(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + opts := &initOptions{ + targetDir: "/tmp/target", + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + + // ExecuteWithBaseRef must be called exactly once: declining the offer + // must not trigger a retry. + mockUI.EXPECT(). + ExecuteWithBaseRef(selectedConfig, "/tmp/target", false, false, false, "", opts.templateVars). + Return(errUtils.ErrTargetDirectoryNotEmpty). + Times(1) + mockUI.EXPECT(). + ConfirmUpdateInstead("/tmp/target"). + Return(false, nil) + + targetDir, err := runInitTargetedFlow(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrTargetDirectoryNotEmpty) + assert.Equal(t, "/tmp/target", targetDir) +} + +func TestRunInitInteractiveFlow_OffersUpdateAndRetriesOnConfirm(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + opts := &initOptions{ + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + + gomock.InOrder( + mockUI.EXPECT(). + ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, "", false, false, false, "", opts.templateVars). + Return("/tmp/picked", errUtils.ErrTargetDirectoryNotEmpty), + mockUI.EXPECT(). + ConfirmUpdateInstead("/tmp/picked"). + Return(true, nil), + mockUI.EXPECT(). + ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, "/tmp/picked", false, true, false, "HEAD", opts.templateVars). + Return("/tmp/picked", nil), + ) + + targetDir, err := runInitInteractiveFlow(mockUI, selectedConfig, opts) + + require.NoError(t, err) + assert.Equal(t, "/tmp/picked", targetDir) +} + +func TestRunInitInteractiveFlow_DeclinesUpdateOffer(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + opts := &initOptions{ + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + + mockUI.EXPECT(). + ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, "", false, false, false, "", opts.templateVars). + Return("/tmp/picked", errUtils.ErrTargetDirectoryNotEmpty). + Times(1) + mockUI.EXPECT(). + ConfirmUpdateInstead("/tmp/picked"). + Return(false, nil) + + targetDir, err := runInitInteractiveFlow(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrTargetDirectoryNotEmpty) + assert.Equal(t, "/tmp/picked", targetDir) +} + +// TestRunInitInteractiveFlow_ResolveInteractiveInitBaseRefError verifies that +// when resolveInteractiveInitBaseRef fails (--update with an unreadable +// metadata pin), runInitInteractiveFlow returns the error without ever +// calling ExecuteWithInteractiveFlowAndBaseRefResult. +func TestRunInitInteractiveFlow_ResolveInteractiveInitBaseRefError(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + opts := &initOptions{ + interactive: true, + update: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + mockUI.EXPECT(). + ResolveTargetPath(selectedConfig, "", true, false, opts.templateVars). + Return(dir, opts.templateVars, false, nil) + // No ExecuteWithInteractiveFlowAndBaseRefResult expectation: gomock fails + // the test if it's called. + + targetDir, err := runInitInteractiveFlow(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.Equal(t, dir, targetDir) +} + +// TestRunInitTargetedFlow_ShouldOfferUpdateErrorPropagates verifies that when +// shouldOfferUpdate itself fails (a corrupt metadata pin at the target +// directory, surfaced while resolving the retry base ref), that error is +// returned directly instead of ConfirmUpdateInstead ever being called. +func TestRunInitTargetedFlow_ShouldOfferUpdateErrorPropagates(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + opts := &initOptions{ + targetDir: dir, + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + mockUI.EXPECT(). + ExecuteWithBaseRef(selectedConfig, dir, false, false, false, "", opts.templateVars). + Return(errUtils.ErrTargetDirectoryNotEmpty) + // No ConfirmUpdateInstead expectation: gomock fails the test if it's called. + + targetDir, err := runInitTargetedFlow(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.NotErrorIs(t, err, errUtils.ErrTargetDirectoryNotEmpty) + assert.Equal(t, dir, targetDir) +} + +// TestRunInitInteractiveFlow_ShouldOfferUpdateErrorPropagates is the +// interactive-flow counterpart of +// TestRunInitTargetedFlow_ShouldOfferUpdateErrorPropagates. +func TestRunInitInteractiveFlow_ShouldOfferUpdateErrorPropagates(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + opts := &initOptions{ + interactive: true, + templateVars: map[string]interface{}{}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + mockUI.EXPECT(). + ExecuteWithInteractiveFlowAndBaseRefResult(selectedConfig, "", false, false, false, "", opts.templateVars). + Return(dir, errUtils.ErrTargetDirectoryNotEmpty) + // No ConfirmUpdateInstead expectation: gomock fails the test if it's called. + + targetDir, err := runInitInteractiveFlow(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.NotErrorIs(t, err, errUtils.ErrTargetDirectoryNotEmpty) + assert.Equal(t, dir, targetDir) +} + +// TestResolveInteractiveInitBaseRef_UpdateTrue_ResolvesTargetAndBaseRef +// reproduces the bug reported against `atmos init --update` with no +// positional target: the base ref used to default to "HEAD" because it was +// resolved (via defaultBaseRef) against the empty target passed to the RunE +// handler *before* the interactive flow prompted for and picked the real +// directory, so any pin at that real directory +// (.atmos/init/metadata.yaml, written by gen.PinInitialBaseRefForInit) was +// silently ignored. This asserts the base ref returned is resolved against +// the actual directory ResolveTargetPath returns, and picks up its pin. +func TestResolveInteractiveInitBaseRef_UpdateTrue_ResolvesTargetAndBaseRef(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + dir := t.TempDir() + metadata := storage.NewInitMetadata("test", "1.0.0", "embedded", "pinned-after-prompt", nil) + require.NoError(t, storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Save(metadata)) + + opts := &initOptions{ + update: true, + interactive: true, + templateVars: map[string]interface{}{"key": "value"}, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + // ResolveTargetPath stands in for the interactive prompt picking `dir`. + mockUI.EXPECT(). + ResolveTargetPath(selectedConfig, "", true, false, opts.templateVars). + Return(dir, opts.templateVars, true, nil) + + resolved, err := resolveInteractiveInitBaseRef(mockUI, selectedConfig, opts) + + require.NoError(t, err) + assert.Equal(t, dir, resolved.targetDir) + // The regression: baseRef must be the pin resolved against `dir` (the + // real, resolved target), not "HEAD" -- which is what a premature + // defaultBaseRef("", "") call against the empty positional target would + // have produced. + assert.Equal(t, "pinned-after-prompt", resolved.baseRef) + assert.True(t, resolved.useDefaults) + assert.Equal(t, opts.templateVars, resolved.templateValues) +} + +// TestResolveInteractiveInitBaseRef_UpdateTrue_ResolveTargetPathError verifies +// that a ResolveTargetPath failure is propagated without calling +// defaultBaseRef. +func TestResolveInteractiveInitBaseRef_UpdateTrue_ResolveTargetPathError(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + opts := &initOptions{ + update: true, + interactive: true, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + mockUI.EXPECT(). + ResolveTargetPath(selectedConfig, "", true, false, opts.templateVars). + Return("/tmp/picked", opts.templateVars, false, errUtils.ErrInitialization) + + resolved, err := resolveInteractiveInitBaseRef(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrInitialization) + assert.Equal(t, "/tmp/picked", resolved.targetDir) +} + +// TestResolveInteractiveInitBaseRef_UpdateTrue_DefaultBaseRefError verifies a +// corrupt/unreadable metadata file at the resolved target surfaces as an +// error rather than silently resolving to "HEAD". +func TestResolveInteractiveInitBaseRef_UpdateTrue_DefaultBaseRefError(t *testing.T) { + selectedConfig := &templates.Configuration{Name: "test"} + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + opts := &initOptions{ + update: true, + interactive: true, + } + + ctrl := gomock.NewController(t) + mockUI := NewMockInitUI(ctrl) + mockUI.EXPECT(). + ResolveTargetPath(selectedConfig, "", true, false, opts.templateVars). + Return(dir, opts.templateVars, false, nil) + + resolved, err := resolveInteractiveInitBaseRef(mockUI, selectedConfig, opts) + + require.Error(t, err) + assert.Equal(t, dir, resolved.targetDir) + assert.Empty(t, resolved.baseRef) +} diff --git a/cmd/init/init_test.go b/cmd/init/init_test.go index ab95f88bd34..f5fe6b445a5 100644 --- a/cmd/init/init_test.go +++ b/cmd/init/init_test.go @@ -8,11 +8,15 @@ import ( "path/filepath" "testing" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/spf13/cobra" "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/generator/storage" "github.com/cloudposse/atmos/pkg/generator/templates" ) @@ -568,21 +572,267 @@ func TestShouldOfferUpdate(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - offer, baseRef := shouldOfferUpdate(tt.err, tt.opts) + // A fresh, never-written directory: shouldOfferUpdate must resolve + // against the *actual* target passed in, not any stale + // opts.targetDir (see TestShouldOfferUpdate_UsesActualTargetDir for + // the regression this guards against). + offer, baseRef, err := shouldOfferUpdate(tt.err, tt.opts, t.TempDir()) + require.NoError(t, err) assert.Equal(t, tt.wantOffer, offer) assert.Equal(t, tt.wantBaseRef, baseRef) }) } } -// TestDefaultBaseRef pins the fix for a real bug: `atmos init aws/app -// --update` with no --base-ref silently set up no git storage at all -// (ExecuteWithDelimiters only calls SetupGitStorage when baseRef is -// non-empty), so every file failed with an opaque "three-way merge failed" -// even on a completely unmodified, freshly re-run directory. +// TestShouldOfferUpdate_UsesActualTargetDir reproduces the interactive +// retry-offer half of defaultBaseRef's bug: opts.targetDir is the raw +// positional CLI arg, which is "" when the user ran `atmos init --update` +// with no target and the interactive flow picked the real directory itself. +// It must resolve the retry base ref against the caller-supplied targetDir +// parameter (the real, resolved directory), not opts.targetDir. +func TestShouldOfferUpdate_UsesActualTargetDir(t *testing.T) { + dir := t.TempDir() + metadata := storage.NewInitMetadata("demo", "1.0.0", "embedded", "pinned-at-real-dir", nil) + require.NoError(t, storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Save(metadata)) + + notEmptyErr := errUtils.Build(errUtils.ErrTargetDirectoryNotEmpty).Err() + // opts.targetDir left empty on purpose: it mirrors the raw positional arg + // in the no-target interactive scenario, and must be ignored in favor of + // the targetDir parameter below. + opts := &initOptions{interactive: true} + + offer, baseRef, err := shouldOfferUpdate(notEmptyErr, opts, dir) + + require.NoError(t, err) + assert.True(t, offer) + assert.Equal(t, "pinned-at-real-dir", baseRef) +} + +// TestShouldOfferUpdate_PropagatesMetadataLoadError verifies a +// corrupt/unreadable metadata file surfaces as an error from +// shouldOfferUpdate rather than silently resolving to "HEAD". +func TestShouldOfferUpdate_PropagatesMetadataLoadError(t *testing.T) { + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + notEmptyErr := errUtils.Build(errUtils.ErrTargetDirectoryNotEmpty).Err() + opts := &initOptions{interactive: true} + + offer, baseRef, err := shouldOfferUpdate(notEmptyErr, opts, dir) + + require.Error(t, err) + assert.False(t, offer) + assert.Empty(t, baseRef) +} + +// TestDefaultBaseRef pins two behaviors: +// - An explicit --base-ref always wins, regardless of targetDir. +// - With no --base-ref and no pinned metadata at targetDir, it still falls +// back to "HEAD" -- the original fix for --update with no --base-ref +// silently setting up no git storage at all (ExecuteWithDelimiters only +// calls SetupGitStorage when baseRef is non-empty), which failed every +// file with an opaque "three-way merge failed" even on a completely +// unmodified, freshly re-run directory. func TestDefaultBaseRef(t *testing.T) { - assert.Equal(t, "HEAD", defaultBaseRef("")) - assert.Equal(t, "v1.2.3", defaultBaseRef("v1.2.3")) + headRef, err := defaultBaseRef("", t.TempDir()) + require.NoError(t, err) + assert.Equal(t, "HEAD", headRef) + + explicitRef, err := defaultBaseRef("v1.2.3", t.TempDir()) + require.NoError(t, err) + assert.Equal(t, "v1.2.3", explicitRef) +} + +// TestDefaultBaseRef_PrefersPinnedMetadata reproduces the fix for the bug +// where `atmos init --update` with no --base-ref always diffed against live +// HEAD, so a customization the user committed after generation became +// indistinguishable from the unmodified base -- the merge then silently let +// the freshly rendered template win with no conflict, discarding the user's +// edit. When a pinned base ref exists (written once, at initial `--git` +// generation -- see gen.PinInitialBaseRefForInit), defaultBaseRef must +// prefer it over live HEAD. +func TestDefaultBaseRef_PrefersPinnedMetadata(t *testing.T) { + dir := t.TempDir() + metadata := storage.NewInitMetadata("demo", "1.0.0", "embedded", "abc123pinned", nil) + require.NoError(t, storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Save(metadata)) + + pinnedRef, err := defaultBaseRef("", dir) + require.NoError(t, err) + assert.Equal(t, "abc123pinned", pinnedRef) + + // An explicit --base-ref still overrides the pin. + explicitRef, err := defaultBaseRef("v9.9.9", dir) + require.NoError(t, err) + assert.Equal(t, "v9.9.9", explicitRef) +} + +// TestDefaultBaseRef_PropagatesUnreadableMetadataError reproduces the bug +// where any metadata.Load() error (not just "file doesn't exist") was +// silently swallowed and defaultBaseRef fell back to "HEAD" regardless -- +// defeating the pin fix, since a corrupt pin file would silently +// re-introduce the original silent-overwrite bug (diffing against live HEAD) +// instead of surfacing the problem. The storage.MetadataStorage.Load method +// returns (nil, nil) only when the file is genuinely absent (os.IsNotExist); +// any other failure (corrupt YAML here) must propagate as an error. +func TestDefaultBaseRef_PropagatesUnreadableMetadataError(t *testing.T) { + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + resolved, err := defaultBaseRef("", dir) + + require.Error(t, err) + assert.Empty(t, resolved) + assert.NotEqual(t, "HEAD", resolved, "a corrupt metadata file must not silently fall back to HEAD") +} + +// TestMaybeInitGeneratedProjectGit_PinsInitialBaseRef verifies the fix for +// atmos init --update's silent-data-loss bug: --git must pin the initial +// commit's SHA at .atmos/init/metadata.yaml, the same way cmd/scaffold's +// maybeInitGeneratedGitRepository already does, so defaultBaseRef has a real +// pin to prefer over live HEAD once the user commits a customization. +func TestMaybeInitGeneratedProjectGit_PinsInitialBaseRef(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "README.md"), []byte("hello"), 0o600)) + + cfg := &templates.Configuration{Name: "demo", Version: "1.0.0", Source: "embedded"} + err := maybeInitGeneratedProjectGit(dir, cfg, &initOptions{git: true}) + require.NoError(t, err) + + metadata, err := storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Load() + require.NoError(t, err) + require.NotNil(t, metadata) + assert.NotEmpty(t, metadata.BaseRef) + assert.Equal(t, "demo", metadata.Template.Name) + + resolved, err := defaultBaseRef("", dir) + require.NoError(t, err) + assert.Equal(t, metadata.BaseRef, resolved, "defaultBaseRef must prefer the pin just written") +} + +// TestInitCmd_RunE_UpdateWithPositionalTarget_ResolvesBaseRefFromRealTargetPin +// reproduces the RunE fix: with a positional target directory and --update, +// RunE must pre-resolve --base-ref against that *real* target's own pinned +// metadata (.atmos/init/metadata.yaml), not an empty path. A pinned base ref +// that doesn't exist in the target's git history surfaces as +// errUtils.ErrInvalidBaseRef once ExecuteWithBaseRef's git storage setup +// tries to validate it -- proving the pin was actually read (the old, +// unconditional single-target-agnostic resolution would have silently +// defaulted to "HEAD", which resolves fine and would not fail this way). +func TestInitCmd_RunE_UpdateWithPositionalTarget_ResolvesBaseRefFromRealTargetPin(t *testing.T) { + dir := t.TempDir() + repo, err := git.PlainInit(dir, false) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, "README.md"), []byte("# demo\n"), 0o600)) + worktree, err := repo.Worktree() + require.NoError(t, err) + _, err = worktree.Add("README.md") + require.NoError(t, err) + _, err = worktree.Commit("initial", &git.CommitOptions{ + Author: &object.Signature{Name: "Test", Email: "test@example.com"}, + }) + require.NoError(t, err) + + metadata := storage.NewInitMetadata("simple", "1.0.0", "embedded", "missing-ref", nil) + require.NoError(t, storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Save(metadata)) + + // RunE binds this test's flags to the global viper.GetViper() singleton + // (BindFlagsToViper), which outlives the test unless reset. Registering a + // fresh *cobra.Command with initCmd's flags (rather than calling + // initCmd.SetArgs/Execute on the shared package-level initCmd) also keeps + // this test from mutating initCmd's own FlagSet -- see the identical + // pattern and rationale in + // cmd/scaffold/scaffold_coverage_test.go's + // TestScaffoldGenerateRunE_UpdateFlagWithPositionalTarget_ResolvesBaseRef + // (cmd.NewTestKit only restores RootCmd state and isn't available to this + // package: it would create an import cycle back into cmd). + t.Cleanup(func() { viper.Reset() }) + + cmd := &cobra.Command{} + initParser.RegisterFlags(cmd) + require.NoError(t, cmd.Flags().Set("update", "true")) + require.NoError(t, cmd.Flags().Set("interactive", "false")) + require.NoError(t, cmd.Flags().Set("force", "false")) + require.NoError(t, cmd.Flags().Set("no-git", "true")) + require.NoError(t, cmd.Flags().Set("set", "project_name=demo")) + + err = initCmd.RunE(cmd, []string{"simple", dir}) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrInvalidBaseRef) +} + +// TestInitCmd_RunE_UpdateWithPositionalTarget_PropagatesMetadataLoadError +// covers RunE's error branch for the same pre-resolution: a genuinely +// unreadable pin file (corrupt YAML here) must surface as an error from the +// command immediately, rather than being swallowed and silently falling back +// to "HEAD". +func TestInitCmd_RunE_UpdateWithPositionalTarget_PropagatesMetadataLoadError(t *testing.T) { + dir := t.TempDir() + metadataPath := storage.InitMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + // See the comment in + // TestInitCmd_RunE_UpdateWithPositionalTarget_ResolvesBaseRefFromRealTargetPin + // above for why this uses a fresh *cobra.Command plus a viper.Reset + // cleanup instead of calling initCmd.SetArgs/Execute directly. + t.Cleanup(func() { viper.Reset() }) + + cmd := &cobra.Command{} + initParser.RegisterFlags(cmd) + require.NoError(t, cmd.Flags().Set("update", "true")) + require.NoError(t, cmd.Flags().Set("interactive", "false")) + require.NoError(t, cmd.Flags().Set("force", "false")) + require.NoError(t, cmd.Flags().Set("no-git", "true")) + + err := initCmd.RunE(cmd, []string{"simple", dir}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "resolve default --base-ref") +} + +// TestResolveInteractiveInitBaseRef_NoUpdate_PassesThroughOptsUnchanged +// covers resolveInteractiveInitBaseRef's non-update path: without --update +// the base ref is unused (ExecuteWithDelimiters only sets up git storage when +// update is true), so this is a no-op passthrough that must not touch +// initUI at all -- exercised here with a nil *ui.InitUI to prove it. +// +// The --update branch (which resolves the target directory first via +// initUI.ResolveTargetPath) always prompts through a real huh form when no +// target is already known and so cannot be safely unit tested -- the same +// limitation documented on TestRunInitExecution_WithTargetDir above. +func TestResolveInteractiveInitBaseRef_NoUpdate_PassesThroughOptsUnchanged(t *testing.T) { + tests := []struct { + name string + interactive bool + wantUseDefaults bool + }{ + {name: "interactive", interactive: true, wantUseDefaults: false}, + {name: "non-interactive", interactive: false, wantUseDefaults: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &initOptions{ + update: false, + interactive: tt.interactive, + baseRef: "v1.2.3", + templateVars: map[string]interface{}{"key": "value"}, + } + + resolved, err := resolveInteractiveInitBaseRef(nil, nil, opts) + + require.NoError(t, err) + assert.Empty(t, resolved.targetDir) + assert.Equal(t, "v1.2.3", resolved.baseRef) + assert.Equal(t, opts.templateVars, resolved.templateValues) + assert.Equal(t, tt.wantUseDefaults, resolved.useDefaults) + }) + } } // TestRunInitExecution_NonEmptyTargetDir_NonInteractive_ReturnsError covers diff --git a/cmd/init/interfaces.go b/cmd/init/interfaces.go new file mode 100644 index 00000000000..556697bd856 --- /dev/null +++ b/cmd/init/interfaces.go @@ -0,0 +1,28 @@ +package initcmd + +//go:generate go run go.uber.org/mock/mockgen@v0.6.0 -source=$GOFILE -destination=mock_$GOFILE -package=$GOPACKAGE + +import ( + "github.com/cloudposse/atmos/pkg/generator/merge" + "github.com/cloudposse/atmos/pkg/generator/templates" + generatorUI "github.com/cloudposse/atmos/pkg/generator/ui" +) + +// InitUI is the subset of *generatorUI.InitUI's behavior the init command +// depends on, extracted so tests can substitute a mock instead of driving +// the real interactive TUI (prompts, huh forms) end to end. Mirrors +// cmd/scaffold's ScaffoldUI, which solves the same problem for the sibling +// command. +type InitUI interface { + SetConflictStrategy(strategy merge.ConflictStrategy) + SetMergeDriver(driver merge.Driver) + SetSkipHooks(skip func(string) bool) + PromptForTemplate(templateType string, templates interface{}) (string, error) + ExecuteWithBaseRef(embedsConfig *templates.Configuration, targetPath string, force, update, useDefaults bool, baseRef string, cmdTemplateValues map[string]interface{}) error + ExecuteWithInteractiveFlowAndBaseRefResult(embedsConfig *templates.Configuration, targetPath string, force, update, useDefaults bool, baseRef string, cmdTemplateValues map[string]interface{}) (string, error) + ResolveTargetPath(embedsConfig *templates.Configuration, targetPath string, update, useDefaults bool, cmdTemplateValues map[string]interface{}) (string, map[string]interface{}, bool, error) + ConfirmUpdateInstead(targetPath string) (bool, error) +} + +// Compile-time check that *generatorUI.InitUI satisfies InitUI. +var _ InitUI = (*generatorUI.InitUI)(nil) diff --git a/cmd/init/mock_interfaces.go b/cmd/init/mock_interfaces.go new file mode 100644 index 00000000000..1072a996b06 --- /dev/null +++ b/cmd/init/mock_interfaces.go @@ -0,0 +1,154 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: cmd/init/interfaces.go +// +// Generated by this command: +// +// mockgen -source=cmd/init/interfaces.go -destination=cmd/init/mock_interfaces.go -package=initcmd +// + +// Package initcmd is a generated GoMock package. +package initcmd + +import ( + reflect "reflect" + + merge "github.com/cloudposse/atmos/pkg/generator/merge" + templates "github.com/cloudposse/atmos/pkg/generator/templates" + gomock "go.uber.org/mock/gomock" +) + +// MockInitUI is a mock of InitUI interface. +type MockInitUI struct { + ctrl *gomock.Controller + recorder *MockInitUIMockRecorder + isgomock struct{} +} + +// MockInitUIMockRecorder is the mock recorder for MockInitUI. +type MockInitUIMockRecorder struct { + mock *MockInitUI +} + +// NewMockInitUI creates a new mock instance. +func NewMockInitUI(ctrl *gomock.Controller) *MockInitUI { + mock := &MockInitUI{ctrl: ctrl} + mock.recorder = &MockInitUIMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockInitUI) EXPECT() *MockInitUIMockRecorder { + return m.recorder +} + +// ConfirmUpdateInstead mocks base method. +func (m *MockInitUI) ConfirmUpdateInstead(targetPath string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ConfirmUpdateInstead", targetPath) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ConfirmUpdateInstead indicates an expected call of ConfirmUpdateInstead. +func (mr *MockInitUIMockRecorder) ConfirmUpdateInstead(targetPath any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConfirmUpdateInstead", reflect.TypeOf((*MockInitUI)(nil).ConfirmUpdateInstead), targetPath) +} + +// ExecuteWithBaseRef mocks base method. +func (m *MockInitUI) ExecuteWithBaseRef(embedsConfig *templates.Configuration, targetPath string, force, update, useDefaults bool, baseRef string, cmdTemplateValues map[string]any) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ExecuteWithBaseRef", embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues) + ret0, _ := ret[0].(error) + return ret0 +} + +// ExecuteWithBaseRef indicates an expected call of ExecuteWithBaseRef. +func (mr *MockInitUIMockRecorder) ExecuteWithBaseRef(embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecuteWithBaseRef", reflect.TypeOf((*MockInitUI)(nil).ExecuteWithBaseRef), embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues) +} + +// ExecuteWithInteractiveFlowAndBaseRefResult mocks base method. +func (m *MockInitUI) ExecuteWithInteractiveFlowAndBaseRefResult(embedsConfig *templates.Configuration, targetPath string, force, update, useDefaults bool, baseRef string, cmdTemplateValues map[string]any) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ExecuteWithInteractiveFlowAndBaseRefResult", embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ExecuteWithInteractiveFlowAndBaseRefResult indicates an expected call of ExecuteWithInteractiveFlowAndBaseRefResult. +func (mr *MockInitUIMockRecorder) ExecuteWithInteractiveFlowAndBaseRefResult(embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExecuteWithInteractiveFlowAndBaseRefResult", reflect.TypeOf((*MockInitUI)(nil).ExecuteWithInteractiveFlowAndBaseRefResult), embedsConfig, targetPath, force, update, useDefaults, baseRef, cmdTemplateValues) +} + +// PromptForTemplate mocks base method. +func (m *MockInitUI) PromptForTemplate(templateType string, arg1 any) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PromptForTemplate", templateType, arg1) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PromptForTemplate indicates an expected call of PromptForTemplate. +func (mr *MockInitUIMockRecorder) PromptForTemplate(templateType, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PromptForTemplate", reflect.TypeOf((*MockInitUI)(nil).PromptForTemplate), templateType, arg1) +} + +// ResolveTargetPath mocks base method. +func (m *MockInitUI) ResolveTargetPath(embedsConfig *templates.Configuration, targetPath string, update, useDefaults bool, cmdTemplateValues map[string]any) (string, map[string]any, bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveTargetPath", embedsConfig, targetPath, update, useDefaults, cmdTemplateValues) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(map[string]any) + ret2, _ := ret[2].(bool) + ret3, _ := ret[3].(error) + return ret0, ret1, ret2, ret3 +} + +// ResolveTargetPath indicates an expected call of ResolveTargetPath. +func (mr *MockInitUIMockRecorder) ResolveTargetPath(embedsConfig, targetPath, update, useDefaults, cmdTemplateValues any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveTargetPath", reflect.TypeOf((*MockInitUI)(nil).ResolveTargetPath), embedsConfig, targetPath, update, useDefaults, cmdTemplateValues) +} + +// SetConflictStrategy mocks base method. +func (m *MockInitUI) SetConflictStrategy(strategy merge.ConflictStrategy) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetConflictStrategy", strategy) +} + +// SetConflictStrategy indicates an expected call of SetConflictStrategy. +func (mr *MockInitUIMockRecorder) SetConflictStrategy(strategy any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetConflictStrategy", reflect.TypeOf((*MockInitUI)(nil).SetConflictStrategy), strategy) +} + +// SetMergeDriver mocks base method. +func (m *MockInitUI) SetMergeDriver(driver merge.Driver) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetMergeDriver", driver) +} + +// SetMergeDriver indicates an expected call of SetMergeDriver. +func (mr *MockInitUIMockRecorder) SetMergeDriver(driver any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetMergeDriver", reflect.TypeOf((*MockInitUI)(nil).SetMergeDriver), driver) +} + +// SetSkipHooks mocks base method. +func (m *MockInitUI) SetSkipHooks(skip func(string) bool) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetSkipHooks", skip) +} + +// SetSkipHooks indicates an expected call of SetSkipHooks. +func (mr *MockInitUIMockRecorder) SetSkipHooks(skip any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSkipHooks", reflect.TypeOf((*MockInitUI)(nil).SetSkipHooks), skip) +} diff --git a/cmd/scaffold/scaffold.go b/cmd/scaffold/scaffold.go index f6621126f62..c7bffbc0f87 100644 --- a/cmd/scaffold/scaffold.go +++ b/cmd/scaffold/scaffold.go @@ -255,7 +255,7 @@ func init() { flags.WithBoolFlag("no-git", "", false, "Do not initialize a git repository"), flags.WithStringFlag("merge-driver", "", "auto", "Merge driver for --update: auto (YAML-aware for .yaml/.yml, text otherwise, default), text (force line-oriented text merge for every file)"), flags.WithValidValues("merge-driver", "auto", "text"), - flags.WithStringFlag("merge-strategy", "", "manual", "Conflict resolution strategy for --update: manual (surface conflicts, default), ours (keep your version), theirs (use the template's version)"), + flags.WithStringFlag("merge-strategy", "", "", "Conflict resolution strategy for --update: manual (surface conflicts, default; theirs if --force is set), ours (keep your version), theirs (use the template's version)"), flags.WithValidValues("merge-strategy", "manual", "ours", "theirs"), // Skip scaffold hooks at runtime, mirroring `terraform`'s --skip-hooks // (see cmd/terraform/flags.go): --skip-hooks (no value) skips all @@ -357,7 +357,7 @@ func executeScaffoldGenerate(opts *scaffoldGenerateOptions) error { return err } - conflictStrategy, err := merge.ParseConflictStrategy(opts.mergeStrategy) + conflictStrategy, err := merge.ResolveConflictStrategy(opts.mergeStrategy, opts.force, opts.update) if err != nil { return err } @@ -673,40 +673,13 @@ func shouldOfferScaffoldUpdate(err error, opts *scaffoldGenerateOptions, targetD return true, resolvedBaseRef, nil } -// defaultBaseRef fills in the 3-way-merge base ref when the caller didn't -// supply --base-ref explicitly (which always wins when set). It prefers the -// ref pinned at targetDir by gen.PinInitialBaseRef -- the commit that -// actually contains this project's pristine generated content -- over live -// HEAD. Without a pin, --update always diffs against whatever HEAD happens -// to be by the time it runs; once a customization is committed, that makes -// it indistinguishable from the unmodified base, and the merge silently lets -// the freshly rendered template overwrite it. Falling back to plain "HEAD" -// (pre-fix scaffolds with no pin, or a non-git target) still fixes the -// original bug this guarded against: with no baseRef at all, --update -// silently sets up no git storage (ExecuteWithDelimiters only calls -// SetupGitStorage when baseRef is non-empty), and every file fails with an -// opaque "three-way merge failed". -// -// A genuinely unreadable metadata file (corrupt YAML, permission denied -- -// anything other than the file simply not existing yet) is surfaced as an -// error instead of silently falling back to "HEAD": swallowing it would -// defeat the whole point of the pin, quietly re-introducing the original -// silent-overwrite bug the very first time the pin file itself is damaged. -// The storage.MetadataStorage.Load method returns (nil, nil) specifically -// when the file is absent, so that case alone still falls through to the -// HEAD/pin logic below. +// defaultBaseRef resolves scaffold generate's --update base ref against this +// target's own pinned metadata (.atmos/scaffold/metadata.yaml, written by +// gen.PinInitialBaseRef). See gen.ResolveDefaultBaseRef's doc for the full +// rationale -- that function is shared with cmd/init's equivalent so the two +// commands' base-ref resolution can't drift apart again. func defaultBaseRef(baseRef, targetDir string) (string, error) { - if baseRef != "" { - return baseRef, nil - } - metadata, err := storage.NewMetadataStorage(storage.ScaffoldMetadataPath(targetDir)).Load() - if err != nil { - return "", fmt.Errorf("resolve default --base-ref from %s: %w", targetDir, err) - } - if metadata != nil && metadata.BaseRef != "" { - return metadata.BaseRef, nil - } - return "HEAD", nil + return gen.ResolveDefaultBaseRef(baseRef, targetDir, storage.ScaffoldMetadataPath(targetDir)) } func maybeInitGeneratedGitRepository(targetDir string, selectedConfig *templates.Configuration, opts *scaffoldGenerateOptions) error { diff --git a/cmd/scaffold/scaffold_test.go b/cmd/scaffold/scaffold_test.go index 1d7d1583375..20269b06972 100644 --- a/cmd/scaffold/scaffold_test.go +++ b/cmd/scaffold/scaffold_test.go @@ -116,10 +116,13 @@ func TestScaffoldGenerateCmd_FlagDefinitions(t *testing.T) { defaultValue: "false", }, { - name: "merge-strategy flag", - flagName: "merge-strategy", - shorthand: "", - defaultValue: "manual", + // Registered default is "" (see ResolveConflictStrategy: an unset + // value defaults to "manual", or "theirs" when --force is also set + // with --update), so no defaultValue check here -- same pattern as + // base-ref flag above. + name: "merge-strategy flag", + flagName: "merge-strategy", + shorthand: "", }, { name: "merge-driver flag", diff --git a/docs/fixes/2026-09-04-scaffold-init-update-merge-fixes.md b/docs/fixes/2026-09-04-scaffold-init-update-merge-fixes.md new file mode 100644 index 00000000000..f2b2801ffc7 --- /dev/null +++ b/docs/fixes/2026-09-04-scaffold-init-update-merge-fixes.md @@ -0,0 +1,186 @@ +# Fix: `--merge-strategy=manual` conflict markers, `atmos init --update` base-ref pinning, and `--force`+`--update` semantics + +**Date:** 2026-09-04 + +## Summary + +`atmos scaffold generate --update` (and `atmos init --update`, which shares the same engine) use a +git-based 3-way merge to reapply template changes onto a file the user has already customized. This +fix addresses four related problems found across that one code path, all tracked on +[cloudposse/atmos#2912](https://github.com/cloudposse/atmos/issues/2912): + +1. With the default `--merge-strategy=manual`, a genuine ours/theirs conflict caused the command to + exit non-zero with an explicit error — but wrote nothing to disk at all: no conflict markers, no + merged content, not even the file's own non-conflicting changes. This is the still-open half of + #2912; the issue was closed as resolved by #2989, but that PR fixed a different bug reported in + the same thread (a silent base-ref-pinning problem) and never touched this code path. +2. Field-testing fix 1 surfaced that `atmos init --update` still has the *exact* base-ref-pinning + bug #2989 fixed for `atmos scaffold generate` — it was never ported to `cmd/init`. +3. `--force` was silently ignored whenever `--update` was also set, making several existing error + hints false. +4. Re-running `--update` against a file fix 1 left with unresolved conflict markers produced an + opaque `three-way merge failed` instead of naming the real problem — a new consequence of fix 1, + not a pre-existing gap. + +## Context + +**Manual-merge conflict markers (fix 1):** `YAMLMerger` and `TextMerger` (`pkg/generator/merge/`) +both compute a 3-way merge result and report `HasConflicts` when the user and the template +genuinely diverged on the same value. `mergeFile` (`pkg/generator/engine/merge_update.go`) treated +`HasConflicts` as fatal and returned before ever reaching the write — for both mergers, on every +conflict, regardless of merge strategy. + +- `TextMerger` already produced real diff3-style markers in `MergeResult.Content` when a conflict + was found (via the `epiclabs-io/diff3` library) — they were computed and then thrown away. +- `YAMLMerger` never had an equivalent: on a real divergence it just picked `ours` internally + (`pickConflictValue`) so it had *something* to return, and that computed result was discarded by + the same early return. There was no way to express `<<<<<<<`/`=======`/`>>>>>>>` markers in a + parsed YAML tree at all. + +Verified live against the exact repro steps from the original issue (both the YAML and text-file +cases) that current `main` (`18750ab6a`, confirmed via `git fetch upstream` — 11 commits ahead of +the commit first tested, none touching the affected files) still reproduced the bug before this fix. + +**`atmos init --update` base-ref pinning (fix 2):** `cmd/scaffold/scaffold.go`'s `defaultBaseRef` +reads a pinned commit SHA from `.atmos/scaffold/metadata.yaml` (written by `gen.PinInitialBaseRef` +during `--git` generation) instead of defaulting to live `HEAD`. `cmd/init/init.go`'s +`defaultBaseRef` was never updated to match — it still hardcoded `"HEAD"`, and `atmos init --git` +never called any pinning function at all. Once a user committed a customization, `HEAD` became +byte-identical to their working tree, so the merge concluded nothing had diverged and silently let +the template win — exit 0, no warning. Root cause confirmed live: passing the correct +`--base-ref ` explicitly preserved the customization, isolating the bug to +default-resolution, not the merge logic itself. + +**`--force` semantics (fix 3):** `handleExistingFile` (`pkg/generator/engine/templating.go`) checks +`update` before `force`, and the `update` branch always merges and returns — so `--force --update` +together behaved identically to `--update` alone. A full blind overwrite under `--update` was +considered and rejected as the fix — it would silently discard non-conflicting customizations too, +defeating the reason `--update` was requested. The chosen design: `--force` flips +`--merge-strategy`'s *default* from `manual` to `theirs` when `--update` is set and no strategy was +explicitly passed through any layer (CLI flag, env var, or config) — and errors if +`--force --update` is combined with an *explicitly* passed `ours` or `manual`, since that +combination is a genuine contradiction rather than a preference. + +**Opaque re-run error (fix 4):** This scenario didn't exist before fix 1 (nothing was ever written +on conflict), so it's a new consequence of that fix rather than a pre-existing gap. A file left with +real conflict markers, if fed back into `--update` unresolved, either fails to parse as YAML (an +opaque error) or — for text files, which have no syntax requirement on their input — silently +produces a garbled result with no error at all. + +## Changes + +**Fix 1 — conflict markers:** + +- `pkg/generator/merge/yaml_merger.go`: on a real conflict under `ConflictStrategyManual`, the + conflicting node is now replaced with a unique sentinel scalar placeholder + (`addNodeConflict`/`conflictSentinelFormat`) instead of silently picking `ours`. After the whole + tree is encoded to YAML text, `spliceConflictMarkers` finds each sentinel and reconstructs real + `<<<<<<<`/`=======`/`>>>>>>>` markers around independently-rendered `ours`/`theirs` fragments — + inline when both sides are scalars, or as an indented block beneath the key when either side is a + mapping/sequence. `mergeSequences` returns a sentinel node as-is instead of wrapping it as a + `SequenceNode`. `MergeResult` gained a `ConflictPaths []string` field. +- `pkg/generator/merge/text_merger.go`: added the `ConflictPaths` field to `MergeResult` (left + `nil` — diff3 hunks aren't addressable by path). No merge-logic change needed; the markers already + existed. +- `pkg/generator/engine/merge_update.go`: `mergeFile` now writes `result.Content` to disk on a + conflict (skipped only under `--dry-run`) before returning `ErrMergeConflict`, with the conflicting + key paths attached as `conflict_paths` context when known. +- Known limitation, documented in code rather than solved: flow-style YAML (`{a: 1, b: 2}`) can + place more than one sentinel on the same line; the splice then wraps only the first match. Scaffold + templates in this repo use block style, so this was accepted rather than building a full + flow-aware splitter. +- `--merge-strategy=ours`/`theirs` are unaffected — both auto-resolve every conflict to a side + before `HasConflicts` is ever set. + +**Fix 2 — base-ref pinning:** + +- `pkg/generator/storage/metadata.go`: added `InitMetadataPath` (`.atmos/init/metadata.yaml`), + mirroring `ScaffoldMetadataPath`. +- `pkg/generator/gitinit.go`: extracted the shared pin/resolve logic that had drifted apart between + the two commands (which is exactly how `atmos init` missed the original fix) into + `ResolveDefaultBaseRef(baseRef, targetDir, metadataPath)` and `PinInitialBaseRefForInit` (sharing + a new private `pinBaseRef` helper with `PinInitialBaseRef`), parameterized by metadata path so + both commands share one implementation instead of two that can silently diverge again. +- `cmd/scaffold/scaffold.go`: `defaultBaseRef` now delegates to `gen.ResolveDefaultBaseRef` — a pure + refactor, no behavior change (existing tests pass unmodified). +- `cmd/init/init.go`: ported the full pattern from `cmd/scaffold` — `defaultBaseRef` delegates to + `gen.ResolveDefaultBaseRef`; `maybeInitGeneratedProjectGit` now calls + `gen.PinInitialBaseRefForInit` after `--git` creates the initial commit; the early `--base-ref` + resolution in `RunE` only runs when a positional target was given (matching `cmd/scaffold`'s + guard); added `resolveInteractiveInitBaseRef` (returning a small `interactiveInitBaseRef` struct, + to stay under revive's function-result-limit) for the no-positional-target interactive flow, + mirroring `cmd/scaffold`'s `resolveInteractiveBaseRef`; `shouldOfferUpdate` now takes the actual + resolved target directory and can return a metadata-load error, matching + `shouldOfferScaffoldUpdate`. + +**Fix 3 — `--force`+`--update`:** + +- `pkg/generator/merge/merge.go`: added `ResolveConflictStrategy(mergeStrategy, force, update)`, + called by both commands instead of `ParseConflictStrategy` directly. An unset strategy defaults to + `theirs` under `--force`+`--update`; an explicit `manual`/`ours` combined with both is an + `errUtils.ErrMutuallyExclusiveFlags` error. +- `cmd/init/init.go` / `cmd/scaffold/scaffold.go`: the `--merge-strategy` flag's registered default + changed from `"manual"` to `""` (help text still advertises `manual` as the effective default) so + `ResolveConflictStrategy` can distinguish "unset" from "explicitly set to manual" — the same + pattern already used by `--base-ref`. +- Every `--force`-suggesting hint across `pkg/generator/engine/merge_update.go`, + `pkg/generator/engine/templating.go`, `pkg/generator/merge/text_merger.go`, and + `pkg/generator/merge/yaml_merger.go` was reworded to describe what `--force` actually does now + (resolve conflicts to the template's version) rather than a "complete overwrite" it never performs + under `--update`; the two "no git storage" hints were corrected to say `--force` only works there + if `--update` is dropped, since a merge literally cannot be attempted without a git base regardless + of conflict-strategy. + +**Fix 4 — opaque re-run error:** + +- `pkg/generator/merge/text_merger.go`: added `HasUnresolvedConflictMarkers`, a + false-positive-safe check (requires the full `<<<<<<< Ours` / `=======` / `>>>>>>> Theirs` triplet + in order) distinct from the existing, broader — and previously unused — `HasConflictMarkers`. +- `pkg/generator/engine/merge_update.go`: `mergeFile` now checks `HasUnresolvedConflictMarkers` + against the existing file before attempting a merge, failing fast with `ErrMergeConflict` and a + specific explanation instead of re-attempting a merge against corrupted "ours" content. + +## Validation + +- `go build ./...` clean. +- `go test ./pkg/generator/... ./cmd/init/... ./cmd/scaffold/...` — all pass, including new tests: + `TestYAMLMerger_ConflictMarkers_Scalar`, `TestYAMLMerger_ConflictMarkers_KindDivergence`, + `TestYAMLMerger_ConflictMarkers_MultipleConflictsDoNotCollide` (guards the fixed-width sentinel + format against substring collisions across 12 simultaneous conflicts), + `TestProcessorMergeFile_ConflictBranchReturnsError`/`TestProcessorMergeFile_ConflictBranchDryRunDoesNotWrite`/ + `TestProcessorMergeFile_RejectsUnresolvedMarkers` (`pkg/generator/engine`), + `TestDefaultBaseRef_PrefersPinnedMetadata`/`TestShouldOfferUpdate_UsesActualTargetDir`/ + `TestShouldOfferUpdate_PropagatesMetadataLoadError`/`TestMaybeInitGeneratedProjectGit_PinsInitialBaseRef` + (`cmd/init`), and `TestResolveConflictStrategy`/`TestResolveConflictStrategy_ErrorIsMutuallyExclusiveFlags`/ + `TestHasUnresolvedConflictMarkers` (`pkg/generator/merge`). +- `atmos lint --changed` clean on every file this fix touched, after fixing three findings it + surfaced along the way: two `godot` comment-capitalization findings, and a `revive` + function-result-limit finding on `resolveInteractiveInitBaseRef` (fixed by bundling its five + return values into an `interactiveInitBaseRef` struct — `cmd/scaffold`'s equivalent function has + the same five-return shape but predates this lint baseline and wasn't flagged, so this new copy + needed the struct where the original didn't). +- Live end-to-end verification via a built `./build/atmos` binary for all four fixes: the original + YAML and text-file repro steps now write real conflict markers with non-conflicting changes from + both sides preserved; nested/deep YAML conflicts reconstruct with correct indentation at 4+ levels; + `--dry-run` still never writes; `--merge-strategy=ours`/`theirs` still resolve cleanly with no + markers; a committed `atmos.yaml` customization now survives `atmos init --update`; + `--update --force` with no explicit strategy now resolves conflicts to the template's version + instead of being a no-op; `--update --force --merge-strategy=ours` now errors with an explanation + instead of silently behaving like plain `--update`; `--update --force --merge-strategy=theirs` + still succeeds (redundant, not contradictory); plain `--update` with no `--force` is unaffected; + and re-running `--update` against an unresolved-marker file now reports `merge conflict detected` + (with a specific "still has unresolved conflict markers" explanation) instead of the generic + `three-way merge failed`, leaving the file untouched either way. +- A full, unscoped `go test ./...` was attempted but is not this fix's validation method of record: + it hit a `tests` package panic inside `go-git`'s internal merkletrie diff computation (unrelated to + any file this fix touches) and a 10-minute timeout building a coverage-instrumented binary in + `tests/testhelpers`, both consistent with running a parallel `go build`/lint pass competing for CPU + at the same time on this machine rather than a real regression. Per CLAUDE.md, `atmos test`/ + `atmos test --full` (not raw `go test ./...`) is the sanctioned validation command for this repo's + slow/integration suite; that full run was not repeated here since every package this fix actually + touches was independently re-verified green above. + +## Follow-ups + +None. This closes out the original report and every item posted to +[cloudposse/atmos#2912](https://github.com/cloudposse/atmos/issues/2912)'s follow-up comment. diff --git a/pkg/generator/engine/merge_update.go b/pkg/generator/engine/merge_update.go index 581612364b5..7c94160a4be 100644 --- a/pkg/generator/engine/merge_update.go +++ b/pkg/generator/engine/merge_update.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "strings" "github.com/go-git/go-git/v5" @@ -136,6 +137,24 @@ func (p *Processor) mergeFile(existingPath string, file File, targetPath string) Err() } + // A file left with real conflict markers from a previous --update can't + // be merged again as-is: re-parsing it as "ours" either fails outright + // (YAMLMerger) or silently garbles the result (TextMerger, which has no + // syntax requirement on its inputs). Fail fast with a specific message + // naming the real problem, instead of surfacing whatever opaque failure + // that produces. + if merge.HasUnresolvedConflictMarkers(string(existingContent)) { + return errUtils.Build(errUtils.ErrMergeConflict). + WithExplanationf("`%s` still has unresolved conflict markers from a previous `--update`", file.Path). + WithHint("Open the file, resolve the `<<<<<<<`/`=======`/`>>>>>>>` blocks, and remove the markers"). + WithHint("Then re-run `--update`"). + WithHint("Or drop `--update` and use `--force` alone to overwrite the file completely"). + WithContext("file_path", file.Path). + WithContext("absolute_path", existingPath). + WithExitCode(1). + Err() + } + // Determine base content for 3-way merge baseContent, shouldSkip, err := p.determineBaseContent(file, existingPath) if err != nil { @@ -170,25 +189,55 @@ func (p *Processor) mergeFile(existingPath string, file File, targetPath string) return errUtils.Build(errUtils.ErrThreeWayMerge). WithExplanationf("Failed to perform 3-way merge for file: `%s`", file.Path). WithHint("The changes may be too extensive for automatic merging"). - WithHint("Try using `--force` to overwrite instead"). + WithHint("Try `--force` to resolve every conflict to the template's version instead"). WithHint("Or manually merge the changes"). WithContext("file_path", file.Path). WithExitCode(1). Err() } - // Check for conflicts + // Check for conflicts. Manual (default) strategy still writes the merged + // content — with real <<<<<<< / ======= / >>>>>>> conflict markers, and + // every non-conflicting change from the template applied — so the user + // has something to actually resolve, rather than the file being left + // completely untouched. Dry-run never writes, same as the clean path below. if result.HasConflicts { - return errUtils.Build(errUtils.ErrMergeConflict). - WithExplanationf("Merge resulted in **%d conflict(s)** in file: `%s`", result.ConflictCount, file.Path). - WithHint("Open the file and look for conflict markers: `<<<<<<<`, `=======`, `>>>>>>>`"). - WithHint("Resolve conflicts manually and re-run the command"). - WithHint("Or use `--force` to overwrite the file completely"). + if !p.DryRun { + if err := writeFileSecure(existingPath, []byte(result.Content), file.Permissions, true); err != nil { + return errUtils.Build(errUtils.ErrFileWrite). + WithCause(err). + WithExplanationf("Failed to write conflict markers to file: `%s`", existingPath). + WithHint("Check directory permissions"). + WithHint("Verify sufficient disk space"). + WithContext("file_path", file.Path). + WithContext("absolute_path", existingPath). + WithExitCode(2). + Err() + } + } + + builder := errUtils.Build(errUtils.ErrMergeConflict). + WithExplanationf("Merge resulted in **%d conflict(s)** in file: `%s`", result.ConflictCount, file.Path) + if result.HasMarkers { + builder = builder. + WithHint("Conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`) have been written to the file"). + WithHint("Open it, resolve the conflicts, and remove the markers") + } else { + // Some conflicts (e.g. a document the template changed that the + // user's stream dropped) have no ours/theirs node pair to splice + // inline markers from, so the template's version was kept as-is + // instead -- there's nothing in the file itself to point at. + builder = builder.WithHint("The template's version was kept for the conflicting item(s); review the file to confirm it's what you want") + } + builder = builder. + WithHint("Or re-run with `--force` (or `--merge-strategy=theirs`) to resolve every conflict to the template's version"). WithContext("file_path", file.Path). WithContext("conflict_count", result.ConflictCount). - WithContext("absolute_path", existingPath). - WithExitCode(1). - Err() + WithContext("absolute_path", existingPath) + if len(result.ConflictPaths) > 0 { + builder = builder.WithContext("conflict_paths", strings.Join(result.ConflictPaths, ", ")) + } + return builder.WithExitCode(1).Err() } // Dry-run: the merge above already ran (and would have surfaced conflicts @@ -229,7 +278,7 @@ func (p *Processor) determineBaseContent(file File, existingPath string) (string return "", false, errUtils.Build(errUtils.ErrThreeWayMerge). WithExplanationf("Cannot determine the merge base for `%s` without a git repository", file.Path). WithHint("Run inside a git repository so the base version can be loaded"). - WithHint("Or use `--force` to overwrite the file"). + WithHint("Or drop `--update` and use `--force` alone to overwrite the file"). WithContext("file_path", file.Path). WithExitCode(2). Err() @@ -248,7 +297,7 @@ func (p *Processor) determineBaseContent(file File, existingPath string) (string WithCause(err). WithExplanationf("Failed to load the merge base for `%s` from git", file.Path). WithHint("Verify the base ref exists: `git show `"). - WithHint("Or use `--force` to overwrite the file"). + WithHint("Or drop `--update` and use `--force` alone to overwrite the file"). WithContext("file_path", file.Path). WithContext("relative_path", relativePath). WithExitCode(2). diff --git a/pkg/generator/engine/templating.go b/pkg/generator/engine/templating.go index 0270d42fbc3..33f2db3b196 100644 --- a/pkg/generator/engine/templating.go +++ b/pkg/generator/engine/templating.go @@ -343,6 +343,13 @@ func resolveExistingAncestor(dir string) (string, error) { } } +// newAtomicWriteFS is indirected through a package-level var so tests can +// substitute a mock filesystem.FileSystem (see pkg/filesystem's generated +// MockFileSystem) and force WriteFileAtomic to fail -- a real disk write +// failure (permission denied, out of space) is otherwise impractical to +// trigger portably in a test. +var newAtomicWriteFS = func() filesystem.FileSystem { return filesystem.NewOSFileSystem() } + // writeFileSecure writes content to fullPath, closing the TOCTOU gap between // an earlier existence check and the write. For non-overwrite writes it uses // exclusive creation (O_EXCL), which atomically fails if something raced in @@ -353,7 +360,7 @@ func resolveExistingAncestor(dir string) (string, error) { // secondary symlink-safety win over a plain O_TRUNC open. func writeFileSecure(fullPath string, content []byte, perm os.FileMode, overwrite bool) (err error) { if overwrite { - return filesystem.NewOSFileSystem().WriteFileAtomic(fullPath, content, perm) + return newAtomicWriteFS().WriteFileAtomic(fullPath, content, perm) } f, openErr := os.OpenFile(fullPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, perm) @@ -554,7 +561,7 @@ func (p *Processor) handleExistingFile(file File, fullPath, targetPath string, f return errUtils.Build(errUtils.ErrThreeWayMerge). WithExplanation("`--update` requires a git repository to compute a 3-way merge base"). WithHint("Run inside a git repository and/or pass `--base-ref`"). - WithHint("Or use `--force` to overwrite the file"). + WithHint("Or drop `--update` and use `--force` alone to overwrite the file"). WithContext("file_path", file.Path). WithExitCode(2). Err() diff --git a/pkg/generator/engine/update_test.go b/pkg/generator/engine/update_test.go index c34c008d289..22831f68602 100644 --- a/pkg/generator/engine/update_test.go +++ b/pkg/generator/engine/update_test.go @@ -1,17 +1,21 @@ package engine import ( + "errors" "os" "path/filepath" "strings" "testing" + cockroachErrors "github.com/cockroachdb/errors" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" errUtils "github.com/cloudposse/atmos/errors" + "github.com/cloudposse/atmos/pkg/filesystem" "github.com/cloudposse/atmos/pkg/generator/merge" "github.com/cloudposse/atmos/pkg/generator/storage" ) @@ -446,18 +450,186 @@ func TestProcessorMergeFile_ConflictBranchReturnsError(t *testing.T) { require.Error(t, err) assert.ErrorIs(t, err, errUtils.ErrMergeConflict) + + // The conflict branch must still write the merged content — with real + // conflict markers and any non-conflicting changes — instead of leaving + // the file completely untouched (the original bug: a non-zero exit with + // an explicit error, but nothing on disk to actually resolve). + written, readErr := os.ReadFile(testRepo.configPath) + require.NoError(t, readErr) + writtenContent := string(written) + assert.Contains(t, writtenContent, "<<<<<<< Ours") + assert.Contains(t, writtenContent, "user-change") + assert.Contains(t, writtenContent, "=======") + assert.Contains(t, writtenContent, "template-change") + assert.Contains(t, writtenContent, ">>>>>>> Theirs") } -// Note: mergeFile's os.WriteFile failure branch (writing merged content back -// to existingPath) is not covered here. Reaching it requires existingPath to -// remain a valid, readable regular file through os.ReadFile at the top of -// mergeFile, then fail specifically at the write step — the "directory -// already exists at this path" trick used elsewhere (e.g. -// templating_coverage_test.go's TestWriteFileErrors) does not apply here, -// since that trick fails at the read step instead for a path mergeFile -// requires to already be a regular file. Forcing this branch portably would -// need either a chmod-based permission trick (root/Windows-unsafe, per repo -// convention) or a new injectable write seam, both out of scope here. +// TestProcessorMergeFile_ConflictBranchDryRunDoesNotWrite verifies dry-run +// still reports the conflict (mergeFile returns the same error) but never +// touches the file on disk, matching the clean-merge path's dry-run behavior. +func TestProcessorMergeFile_ConflictBranchDryRunDoesNotWrite(t *testing.T) { + initialContent := "setting: original\n" + userContent := "setting: user-change\n" + testRepo := setupGitTestRepo(t, initialContent, userContent) + testRepo.processor.SetMaxChanges(100) + testRepo.processor.SetDryRun(true) + + templateFile := File{ + Path: "config.yaml", + Content: "setting: template-change\n", + IsTemplate: false, + Permissions: 0o644, + } + + err := testRepo.processor.mergeFile(testRepo.configPath, templateFile, testRepo.tmpDir) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrMergeConflict) + + written, readErr := os.ReadFile(testRepo.configPath) + require.NoError(t, readErr) + assert.Equal(t, userContent, string(written), "dry-run must not modify the file even on conflict") +} + +// TestProcessorMergeFile_RejectsUnresolvedMarkers verifies mergeFile fails +// fast with a specific error when the existing file already contains +// unresolved conflict markers from a previous --update, instead of +// re-attempting a merge against corrupted "ours" content and surfacing +// whatever opaque failure that produces (a YAML parse error, in this case, +// but TextMerger has no syntax requirement on its inputs and would silently +// garble the result rather than error at all). +func TestProcessorMergeFile_RejectsUnresolvedMarkers(t *testing.T) { + initialContent := "setting: original\n" + unresolvedContent := "<<<<<<< Ours\nsetting: user-change\n=======\nsetting: template-change\n>>>>>>> Theirs\n" + testRepo := setupGitTestRepo(t, initialContent, unresolvedContent) + + templateFile := File{ + Path: "config.yaml", + Content: "setting: template-change\n", + IsTemplate: false, + Permissions: 0o644, + } + + err := testRepo.processor.mergeFile(testRepo.configPath, templateFile, testRepo.tmpDir) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrMergeConflict) + + // This is a fail-fast check, not another merge attempt -- the file must + // be left completely untouched. + written, readErr := os.ReadFile(testRepo.configPath) + require.NoError(t, readErr) + assert.Equal(t, unresolvedContent, string(written)) +} + +// TestProcessorMergeFile_DocumentStreamConflictHasNoMarkers covers a +// YAMLMerger conflict that has no ours/theirs node pair to splice inline +// markers from: a multi-document stream where the user's stream dropped a +// document the template went on to change. +// +// That case is recorded as a conflict (HasConflicts) but keeps the +// template's version verbatim instead of inserting <<<<<<< Ours markers +// (HasMarkers is false), so mergeFile must reflect that in its hint instead +// of claiming markers were written when none exist in the file. +func TestProcessorMergeFile_DocumentStreamConflictHasNoMarkers(t *testing.T) { + initialContent := "doc: one\n---\ndoc: two\n" + userContent := "doc: one\n" + testRepo := setupGitTestRepo(t, initialContent, userContent) + testRepo.processor.SetMaxChanges(100) + + templateFile := File{ + Path: "config.yaml", + Content: "doc: one\n---\ndoc: two\ntemplate: true\n", + IsTemplate: false, + Permissions: 0o644, + } + + err := testRepo.processor.mergeFile(testRepo.configPath, templateFile, testRepo.tmpDir) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrMergeConflict) + + hints := cockroachErrors.GetAllHints(err) + for _, h := range hints { + assert.NotContains(t, h, "have been written to the file", + "no inline markers exist for this conflict, so the hint must not claim they were written") + } + assert.Contains(t, hints, "The template's version was kept for the conflicting item(s); review the file to confirm it's what you want") + + // The template's version of the dropped document is kept verbatim -- no + // conflict markers appear anywhere in the written file. + written, readErr := os.ReadFile(testRepo.configPath) + require.NoError(t, readErr) + writtenContent := string(written) + assert.NotContains(t, writtenContent, "<<<<<<<") + assert.Contains(t, writtenContent, "template: true") +} + +// TestProcessorMergeFile_ConflictWriteFailurePropagates forces +// newAtomicWriteFS's underlying WriteFileAtomic to fail via a mock +// filesystem.FileSystem, and asserts the conflict-markers write failure +// (mergeFile's first writeFileSecure call) surfaces as ErrFileWrite instead +// of the conflict succeeding silently. Reaching a real disk write failure at +// this exact step is impractical to trigger portably (existingPath must +// remain a valid, readable regular file through the earlier os.ReadFile, then +// fail specifically at the write) -- see newAtomicWriteFS's doc comment. +func TestProcessorMergeFile_ConflictWriteFailurePropagates(t *testing.T) { + initialContent := "setting: original\n" + userContent := "setting: user-change\n" + testRepo := setupGitTestRepo(t, initialContent, userContent) + testRepo.processor.SetMaxChanges(100) + + original := newAtomicWriteFS + injectedErr := errors.New("injected write failure") + ctrl := gomock.NewController(t) + mockFS := filesystem.NewMockFileSystem(ctrl) + mockFS.EXPECT().WriteFileAtomic(gomock.Any(), gomock.Any(), gomock.Any()).Return(injectedErr) + newAtomicWriteFS = func() filesystem.FileSystem { return mockFS } + t.Cleanup(func() { newAtomicWriteFS = original }) + + templateFile := File{ + Path: "config.yaml", + Content: "setting: template-change\n", + IsTemplate: false, + Permissions: 0o644, + } + + err := testRepo.processor.mergeFile(testRepo.configPath, templateFile, testRepo.tmpDir) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrFileWrite) +} + +// TestProcessorMergeFile_CleanWriteFailurePropagates is the clean-merge +// counterpart of TestProcessorMergeFile_ConflictWriteFailurePropagates: no +// conflicts, so mergeFile takes its second writeFileSecure call instead. +func TestProcessorMergeFile_CleanWriteFailurePropagates(t *testing.T) { + initialContent := "setting: original\nkey1: v1\n" + testRepo := setupGitTestRepo(t, initialContent, initialContent) + + original := newAtomicWriteFS + injectedErr := errors.New("injected write failure") + ctrl := gomock.NewController(t) + mockFS := filesystem.NewMockFileSystem(ctrl) + mockFS.EXPECT().WriteFileAtomic(gomock.Any(), gomock.Any(), gomock.Any()).Return(injectedErr) + newAtomicWriteFS = func() filesystem.FileSystem { return mockFS } + t.Cleanup(func() { newAtomicWriteFS = original }) + + // Template changes a different key: no conflict, so the merge takes the + // clean-write path (mergeFile's second writeFileSecure call). + templateFile := File{ + Path: "config.yaml", + Content: "setting: original\nkey1: v2\n", + IsTemplate: false, + Permissions: 0o644, + } + + err := testRepo.processor.mergeFile(testRepo.configPath, templateFile, testRepo.tmpDir) + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrFileWrite) +} // TestProcessorDetermineBaseContent_LoadBaseError covers LoadBase returning a // non-nil error (as opposed to the found=true and gitStorage==nil cases diff --git a/pkg/generator/gitinit.go b/pkg/generator/gitinit.go index bedc393a3e0..196675caf79 100644 --- a/pkg/generator/gitinit.go +++ b/pkg/generator/gitinit.go @@ -122,6 +122,34 @@ func WithSource(source string) PinOption { func PinInitialBaseRef(targetPath, headSHA string, opts ...PinOption) error { defer perf.Track(nil, "generator.PinInitialBaseRef")() + return pinBaseRef(storage.ScaffoldMetadataPath(targetPath), storage.NewScaffoldMetadata, headSHA, opts...) +} + +// PinInitialBaseRefForInit is PinInitialBaseRef's counterpart for `atmos +// init`, which records its own generation metadata separately at +// .atmos/init/metadata.yaml (see storage.InitMetadataPath) rather than +// .atmos/scaffold/metadata.yaml -- the two commands track unrelated +// generation history, but the pin mechanism itself is identical, so it's +// shared here rather than reimplemented a second time. It was reimplemented +// once before: `atmos init --update` shipped with the same silent-overwrite +// bug PinInitialBaseRef/ResolveDefaultBaseRef guard against, because the +// fix for `atmos scaffold generate --update` lived only in cmd/scaffold and +// was never ported to cmd/init. +func PinInitialBaseRefForInit(targetPath, headSHA string, opts ...PinOption) error { + defer perf.Track(nil, "generator.PinInitialBaseRefForInit")() + + return pinBaseRef(storage.InitMetadataPath(targetPath), storage.NewInitMetadata, headSHA, opts...) +} + +// pinBaseRef is the shared implementation behind PinInitialBaseRef and +// PinInitialBaseRefForInit; only the metadata path and constructor differ +// between the two commands. +func pinBaseRef( + metadataPath string, + newMetadata func(templateName, templateVersion, templateSource, baseRef string, variables map[string]string) *storage.GenerationMetadata, + headSHA string, + opts ...PinOption, +) error { if headSHA == "" { return nil } @@ -131,13 +159,57 @@ func PinInitialBaseRef(targetPath, headSHA string, opts ...PinOption) error { opt(&pinOpts) } - metadata := storage.NewScaffoldMetadata(pinOpts.templateName, pinOpts.templateVersion, pinOpts.source, headSHA, nil) - if err := storage.NewMetadataStorage(storage.ScaffoldMetadataPath(targetPath)).Save(metadata); err != nil { - return fmt.Errorf("%w: pin initial scaffold base ref: %w", errUtils.ErrMetadataSave, err) + metadata := newMetadata(pinOpts.templateName, pinOpts.templateVersion, pinOpts.source, headSHA, nil) + if err := storage.NewMetadataStorage(metadataPath).Save(metadata); err != nil { + return fmt.Errorf("%w: pin initial base ref: %w", errUtils.ErrMetadataSave, err) } return nil } +// ResolveDefaultBaseRef fills in the 3-way-merge base ref used by `--update` +// when the caller didn't supply --base-ref explicitly (which always wins +// when set). It prefers the ref pinned at targetDir by PinInitialBaseRef/ +// PinInitialBaseRefForInit -- the commit that actually contains this +// project's pristine generated content -- over live HEAD. Without a pin, +// --update always diffs against whatever HEAD happens to be by the time it +// runs; once a customization is committed, that makes it indistinguishable +// from the unmodified base, and the merge silently lets the freshly +// rendered template overwrite it. Falling back to plain "HEAD" (pre-fix +// targets with no pin, or a non-git target) still fixes the original bug +// this guards against: with no baseRef at all, --update silently sets up no +// git storage, and every file fails with an opaque "three-way merge +// failed". +// +// A genuinely unreadable metadata file (corrupt YAML, permission denied -- +// anything other than the file simply not existing yet) is surfaced as an +// error instead of silently falling back to "HEAD": swallowing it would +// defeat the whole point of the pin, quietly reintroducing the +// silent-overwrite bug the first time the pin file itself is damaged. The +// storage.MetadataStorage.Load method returns (nil, nil) specifically when +// the file is absent, so that case alone still falls through to the +// HEAD/pin logic below. +// +// The metadataPath parameter is the caller's own pinned-metadata location +// (see storage.ScaffoldMetadataPath/storage.InitMetadataPath) -- shared here +// so `atmos scaffold generate --update` and `atmos init --update`, which pin +// and resolve base refs identically but keep separate metadata, can't drift +// apart the way they did before. +func ResolveDefaultBaseRef(baseRef, targetDir, metadataPath string) (string, error) { + defer perf.Track(nil, "generator.ResolveDefaultBaseRef")() + + if baseRef != "" { + return baseRef, nil + } + metadata, err := storage.NewMetadataStorage(metadataPath).Load() + if err != nil { + return "", fmt.Errorf("resolve default --base-ref from %s: %w", targetDir, err) + } + if metadata != nil && metadata.BaseRef != "" { + return metadata.BaseRef, nil + } + return "HEAD", nil +} + func isInsideGitRepository(path string) bool { _, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{DetectDotGit: true}) return err == nil diff --git a/pkg/generator/gitinit_test.go b/pkg/generator/gitinit_test.go index f60f23d333d..6366515e1c7 100644 --- a/pkg/generator/gitinit_test.go +++ b/pkg/generator/gitinit_test.go @@ -181,3 +181,99 @@ func TestPinInitialBaseRef_NoOptionsStillWritesBaseRef(t *testing.T) { assert.Equal(t, "def456", metadata.BaseRef) assert.Empty(t, metadata.Template.Name) } + +// TestPinInitialBaseRefForInit_WritesInitMetadata verifies `atmos init`'s pin +// writes to .atmos/init/metadata.yaml (storage.InitMetadataPath) rather than +// the scaffold command's .atmos/scaffold/metadata.yaml -- the two commands +// must not clobber each other's pinned base ref. +func TestPinInitialBaseRefForInit_WritesInitMetadata(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, PinInitialBaseRefForInit( + dir, "abc123", + WithTemplateName("simple"), + WithTemplateVersion("2.0.0"), + WithSource("embedded"), + )) + + metadata, err := storage.NewMetadataStorage(storage.InitMetadataPath(dir)).Load() + require.NoError(t, err) + require.NotNil(t, metadata) + assert.Equal(t, "abc123", metadata.BaseRef) + assert.Equal(t, "simple", metadata.Template.Name) + assert.Equal(t, "2.0.0", metadata.Template.Version) + assert.Equal(t, "embedded", metadata.Template.Source) + + // Must not also write (or be confused with) the scaffold command's + // separate metadata file at the same target directory. + assert.NoFileExists(t, storage.ScaffoldMetadataPath(dir)) +} + +// TestPinInitialBaseRefForInit_NoopWhenSkipped mirrors +// TestPinInitialBaseRef_NoopWhenSkipped for the init variant: no commit means +// nothing to pin. +func TestPinInitialBaseRefForInit_NoopWhenSkipped(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, PinInitialBaseRefForInit(dir, "", WithTemplateName("simple"))) + + assert.NoFileExists(t, storage.InitMetadataPath(dir)) +} + +// TestResolveDefaultBaseRef_ExplicitAlwaysWins verifies an explicit --base-ref +// short-circuits before ever consulting pinned metadata, regardless of what +// (if anything) is pinned at targetDir. +func TestResolveDefaultBaseRef_ExplicitAlwaysWins(t *testing.T) { + dir := t.TempDir() + require.NoError(t, PinInitialBaseRef(dir, "pinned-ref", WithTemplateName("basic"))) + + resolved, err := ResolveDefaultBaseRef("v1.2.3", dir, storage.ScaffoldMetadataPath(dir)) + + require.NoError(t, err) + assert.Equal(t, "v1.2.3", resolved) +} + +// TestResolveDefaultBaseRef_FallsBackToHEADWithNoPin reproduces the original +// bug fix: with no --base-ref and no pinned metadata (a pre-fix target, or one +// that was never git-initialized), --update must still get a usable base ref +// instead of silently setting up no git storage at all. +func TestResolveDefaultBaseRef_FallsBackToHEADWithNoPin(t *testing.T) { + dir := t.TempDir() + + resolved, err := ResolveDefaultBaseRef("", dir, storage.ScaffoldMetadataPath(dir)) + + require.NoError(t, err) + assert.Equal(t, "HEAD", resolved) +} + +// TestResolveDefaultBaseRef_PrefersPinnedMetadataOverHEAD verifies the actual +// fix: once a pin exists, it wins over live HEAD so a customization committed +// after generation doesn't silently become indistinguishable from the +// unmodified base. +func TestResolveDefaultBaseRef_PrefersPinnedMetadataOverHEAD(t *testing.T) { + dir := t.TempDir() + require.NoError(t, PinInitialBaseRef(dir, "pinned-sha", WithTemplateName("basic"))) + + resolved, err := ResolveDefaultBaseRef("", dir, storage.ScaffoldMetadataPath(dir)) + + require.NoError(t, err) + assert.Equal(t, "pinned-sha", resolved) +} + +// TestResolveDefaultBaseRef_PropagatesLoadError verifies a genuinely +// unreadable metadata file (corrupt YAML here) surfaces as an error instead +// of silently falling back to "HEAD" -- swallowing it would quietly +// reintroduce the silent-overwrite bug the first time the pin file itself is +// damaged. +func TestResolveDefaultBaseRef_PropagatesLoadError(t *testing.T) { + dir := t.TempDir() + metadataPath := storage.ScaffoldMetadataPath(dir) + require.NoError(t, os.MkdirAll(filepath.Dir(metadataPath), 0o755)) + require.NoError(t, os.WriteFile(metadataPath, []byte("not: valid: yaml: ["), 0o600)) + + resolved, err := ResolveDefaultBaseRef("", dir, metadataPath) + + require.Error(t, err) + assert.Empty(t, resolved) + assert.NotEqual(t, "HEAD", resolved) +} diff --git a/pkg/generator/merge/merge.go b/pkg/generator/merge/merge.go index 9cac0d2473f..2b577ef28aa 100644 --- a/pkg/generator/merge/merge.go +++ b/pkg/generator/merge/merge.go @@ -150,6 +150,54 @@ func ParseConflictStrategy(s string) (ConflictStrategy, error) { } } +// ResolveConflictStrategy determines the effective --merge-strategy after +// accounting for --force, and is what `atmos scaffold generate --update`/ +// `atmos init --update` call instead of ParseConflictStrategy directly. +// +// Without --force (or without --update, where merge-strategy has no effect +// at all), an unset mergeStrategy resolves via ParseConflictStrategy as +// usual, defaulting to ConflictStrategyManual. +// +// With --force AND --update, an unset mergeStrategy instead defaults to +// ConflictStrategyTheirs. --force's own meaning -- both without --update +// (skip the file-exists check, overwrite) and with it -- is "resolve +// whichever safety blocker exists in favor of the fresh generation"; leaving +// <<<<<<< conflict markers for the user to resolve by hand +// (ConflictStrategyManual) is the opposite of that, so --force must not +// silently do nothing the way it used to (mergeStrategy defaulting to +// "manual" regardless left every conflict-branch hint suggesting --force as +// a remedy false). +// +// An EXPLICITLY set "manual" or "ours" together with --force and --update is +// rejected outright: that combination is a genuine contradiction ("force +// through the conflict" vs. "keep my own value" / "show me the conflict to +// resolve by hand"), not a preference to silently pick a side on -- unlike +// an explicit "theirs", which simply agrees with what --force already +// implies and is allowed through unchanged. +func ResolveConflictStrategy(mergeStrategy string, force, update bool) (ConflictStrategy, error) { + defer perf.Track(nil, "merge.ResolveConflictStrategy")() + + if mergeStrategy == "" && force && update { + return ConflictStrategyTheirs, nil + } + + strategy, err := ParseConflictStrategy(mergeStrategy) + if err != nil { + return ConflictStrategyManual, err + } + + if force && update && strategy != ConflictStrategyTheirs { + return ConflictStrategyManual, errUtils.Build(errUtils.ErrMutuallyExclusiveFlags). + WithExplanationf("`--force` and `--merge-strategy=%s` conflict when `--update` is set", mergeStrategy). + WithHint("`--force` already means \"on conflict, the latest generation wins\" (the same as `--merge-strategy=theirs`)"). + WithHint("Drop `--force`, or use `--merge-strategy=theirs` (or omit `--merge-strategy`) instead"). + WithExitCode(2). + Err() + } + + return strategy, nil +} + // Driver selects which merger runs, named after git's merge driver concept // (see `man gitattributes`). type Driver int diff --git a/pkg/generator/merge/merge_test.go b/pkg/generator/merge/merge_test.go index c084797fdf1..1c7d9e0a678 100644 --- a/pkg/generator/merge/merge_test.go +++ b/pkg/generator/merge/merge_test.go @@ -1,8 +1,11 @@ package merge import ( + "errors" "strings" "testing" + + errUtils "github.com/cloudposse/atmos/errors" ) func TestThreeWayMerger_AutoDetection(t *testing.T) { @@ -294,6 +297,61 @@ func TestParseConflictStrategy(t *testing.T) { } } +// TestResolveConflictStrategy covers --force's interaction with +// --merge-strategy under --update: an unset strategy defaults to theirs +// under --force+--update (instead of manual), an explicit theirs is allowed +// through unchanged, but an explicit manual/ours together with --force and +// --update is a hard error rather than force silently winning or losing. +func TestResolveConflictStrategy(t *testing.T) { + tests := []struct { + name string + mergeStrategy string + force bool + update bool + want ConflictStrategy + wantErr bool + }{ + {name: "no force, no update: unset defaults to manual", mergeStrategy: "", force: false, update: false, want: ConflictStrategyManual}, + {name: "no force, with update: unset defaults to manual", mergeStrategy: "", force: false, update: true, want: ConflictStrategyManual}, + {name: "force without update: unset stays manual (force has no update-context meaning)", mergeStrategy: "", force: true, update: false, want: ConflictStrategyManual}, + {name: "force with update: unset defaults to theirs", mergeStrategy: "", force: true, update: true, want: ConflictStrategyTheirs}, + {name: "force with update: explicit theirs allowed", mergeStrategy: "theirs", force: true, update: true, want: ConflictStrategyTheirs}, + {name: "force with update: explicit manual is a contradiction", mergeStrategy: "manual", force: true, update: true, wantErr: true}, + {name: "force with update: explicit ours is a contradiction", mergeStrategy: "ours", force: true, update: true, wantErr: true}, + {name: "force without update: explicit manual is fine (force is a no-op there)", mergeStrategy: "manual", force: true, update: false, want: ConflictStrategyManual}, + {name: "no force: explicit ours passes through", mergeStrategy: "ours", force: false, update: true, want: ConflictStrategyOurs}, + {name: "invalid value still errors", mergeStrategy: "bogus", force: false, update: true, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ResolveConflictStrategy(tt.mergeStrategy, tt.force, tt.update) + if tt.wantErr { + if err == nil { + t.Fatal("expected an error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} + +// TestResolveConflictStrategy_ErrorIsMutuallyExclusiveFlags pins the specific +// sentinel error for the force+update+explicit-manual/ours contradiction, so +// callers can reliably distinguish it from an ordinary invalid-value error. +func TestResolveConflictStrategy_ErrorIsMutuallyExclusiveFlags(t *testing.T) { + _, err := ResolveConflictStrategy("manual", true, true) + if !errors.Is(err, errUtils.ErrMutuallyExclusiveFlags) { + t.Fatalf("expected errUtils.ErrMutuallyExclusiveFlags, got: %v", err) + } +} + func TestParseDriver(t *testing.T) { tests := []struct { name string diff --git a/pkg/generator/merge/text_merger.go b/pkg/generator/merge/text_merger.go index 83593801d3f..f54564142ae 100644 --- a/pkg/generator/merge/text_merger.go +++ b/pkg/generator/merge/text_merger.go @@ -44,6 +44,18 @@ type MergeResult struct { Content string HasConflicts bool ConflictCount int + // ConflictPaths names the conflicting locations (e.g. YAML key paths) + // when the merger can identify them. TextMerger leaves this nil since + // diff3 hunks aren't addressable by path; YAMLMerger populates it. + ConflictPaths []string + // HasMarkers reports whether Content actually contains inline + // <<<<<<>>>>>> conflict markers for a caller to point the user + // at. TextMerger's conflicts always come with markers, so this mirrors + // HasConflicts there. YAMLMerger can record a conflict with no node pair + // to splice markers from (e.g. a document-stream-level conflict where the + // user's stream dropped a document the template changed), in which case + // HasConflicts is true but HasMarkers is false. + HasMarkers bool } // Merge performs a 3-way merge using the diff3 algorithm. @@ -112,7 +124,7 @@ func (m *TextMerger) Merge(base, ours, theirs string) (*MergeResult, error) { if changePercentage > m.thresholdPercent { return nil, errUtils.Build(errUtils.ErrMergeThresholdExceeded). WithExplanationf("Too many changes detected (%d%% changes, threshold: %d%%). %d conflicts found", changePercentage, m.thresholdPercent, conflictCount). - WithHint("Use --force to overwrite or manually merge"). + WithHint("Use --force (resolves every conflict to the template's version) or manually merge"). Err() } } @@ -120,6 +132,7 @@ func (m *TextMerger) Merge(base, ours, theirs string) (*MergeResult, error) { return &MergeResult{ Content: mergedContent, HasConflicts: hasConflicts, + HasMarkers: hasConflicts, ConflictCount: conflictCount, }, nil } @@ -254,3 +267,44 @@ func HasConflictMarkers(content string) bool { strings.Contains(content, "=======") || strings.Contains(content, ">>>>>>>") } + +// HasUnresolvedConflictMarkers reports whether content still contains a full +// <<<<<<< Ours / ======= / >>>>>>> Theirs block -- the exact triplet both +// TextMerger and YAMLMerger write under the manual (default) conflict +// strategy (see engine.Processor.mergeFile and YAMLMerger's +// spliceConflictMarkers). Unlike HasConflictMarkers, which flags any single +// bare marker line and can false-positive on unrelated content (a markdown +// rule, a line that happens to be "======="), this requires the specific +// "Ours"/"Theirs"-labeled sequence in order, which in practice is only ever +// produced by this exact code path -- so engine.Processor.mergeFile can use +// it to fail fast with a specific "resolve this first" error instead of +// re-attempting a merge against already-corrupted "ours" content and +// surfacing whatever opaque failure that produces (a YAML parse error for +// YAMLMerger, or a silently garbled result for TextMerger, which doesn't +// require its "ours" input to be any particular syntax and so wouldn't +// error at all). +func HasUnresolvedConflictMarkers(content string) bool { + defer perf.Track(nil, "merge.HasUnresolvedConflictMarkers")() + + sawOurs, sawSeparator := false, false + for _, line := range strings.Split(content, newlineSeparator) { + trimmed := strings.TrimLeft(line, " ") + switch { + case !sawOurs: + // Exact match: both TextMerger (via diff3's "<<<<<<< %s" label + // format) and YAMLMerger always emit this opening marker verbatim, + // with no trailing suffix -- unlike the closing marker below, which + // YAMLMerger can append the original line's suffix to. A prefix + // match here would false-positive on unrelated content that merely + // starts with this marker (e.g. a line of literal text). + sawOurs = trimmed == "<<<<<<< Ours" + case !sawSeparator: + sawSeparator = trimmed == "=======" + default: + if strings.HasPrefix(trimmed, ">>>>>>> Theirs") { + return true + } + } + } + return false +} diff --git a/pkg/generator/merge/text_merger_test.go b/pkg/generator/merge/text_merger_test.go index 7eb4ea3748d..5a52658565c 100644 --- a/pkg/generator/merge/text_merger_test.go +++ b/pkg/generator/merge/text_merger_test.go @@ -753,6 +753,97 @@ text`, } } +// TestHasUnresolvedConflictMarkers covers the stricter, false-positive-safe +// check engine.Processor.mergeFile uses to reject re-running --update against +// a file a previous conflict already left with real markers: unlike +// HasConflictMarkers, a single bare "=======" line (e.g. a markdown rule) or +// an out-of-order/incomplete triplet must NOT match. +func TestHasUnresolvedConflictMarkers(t *testing.T) { + tests := []struct { + name string + content string + want bool + }{ + {name: "no markers", content: "plain text content", want: false}, + { + name: "full triplet in order", + content: `<<<<<<< Ours +user version +======= +template version +>>>>>>> Theirs +`, + want: true, + }, + { + name: "indented triplet (nested/block-style YAML conflict)", + content: `key: + <<<<<<< Ours + nested: true + ======= + - item + >>>>>>> Theirs +`, + want: true, + }, + { + name: "bare separator alone is not a false positive", + content: "Title\n=======\nSome content", + want: false, + }, + { + name: "start marker with no separator or end marker", + content: "<<<<<<< Ours\nsomething\n", + want: false, + }, + { + name: "start and separator but no end marker", + content: "<<<<<<< Ours\na\n=======\nb\n", + want: false, + }, + { + name: "end marker before start marker does not count", + content: ">>>>>>> Theirs\n<<<<<<< Ours\n=======\n", + want: false, + }, + { + name: "generic diff3 labels (not our Ours/Theirs) do not match", + content: "<<<<<<< HEAD\na\n=======\nb\n>>>>>>> branch\n", + want: false, + }, + { + // Regression: the start marker is always emitted verbatim by both + // mergers (diff3's "<<<<<<< %s" label format and YAMLMerger's + // literal "<<<<<<< Ours") with no trailing suffix, unlike the + // closing marker, which YAMLMerger can append the original line's + // suffix to. A prefix match on the start marker would + // false-positive here. + name: "start marker with suffix is not a real marker", + content: "<<<<<<< OursSomethingElse\na\n=======\nb\n>>>>>>> Theirs\n", + want: false, + }, + { + name: "closing marker with suffix still matches (YAMLMerger appends the original line's suffix)", + content: `<<<<<<< Ours +setting: user-change +======= +setting: template-change +>>>>>>> Theirs # trailing comment +`, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := HasUnresolvedConflictMarkers(tt.content) + if got != tt.want { + t.Errorf("HasUnresolvedConflictMarkers() = %v, want %v", got, tt.want) + } + }) + } +} + // TestTextMerger_TrailingNewlinePreservation asserts exact byte-for-byte // merge output, trailing-newline count included, split into merges with no // genuine ours/theirs divergence (result must equal the unchanged content diff --git a/pkg/generator/merge/yaml_merger.go b/pkg/generator/merge/yaml_merger.go index 5da8c3fb7c3..44a1f5dea6b 100644 --- a/pkg/generator/merge/yaml_merger.go +++ b/pkg/generator/merge/yaml_merger.go @@ -2,6 +2,8 @@ package merge import ( "bytes" + "crypto/rand" + "encoding/hex" "errors" "fmt" "io" @@ -17,6 +19,26 @@ import ( // Constants for YAML merging. const ( maxChangePercentage = 100 // Maximum change percentage when parsing fails. + + // Format for unique placeholder scalar values that stand in for a real + // ours/theirs divergence while the YAML tree is being built. YAML has no + // syntax for embedding diff3-style conflict markers directly in a node, + // so a conflicted node is temporarily replaced with a sentinel scalar; + // once the whole tree is encoded to text, spliceConflictMarkers finds + // each sentinel and replaces it with real <<<<<<< / ======= / >>>>>>> + // markers. The fixed-width %06d index guarantees no two sentinels issued + // for the same merge are ever a substring of one another, so a plain text + // search for one can never accidentally match another. The random hex + // suffix (see conflictTracker.nextSentinel) guarantees a sentinel can + // never collide with a pre-existing scalar value already present in the + // document being merged -- without it, a value that happened to equal a + // sentinel would let spliceConflictMarkers mistake unrelated content for + // the inserted conflict placeholder and corrupt it. + conflictSentinelFormat = "ATMOSMERGECONFLICT%06d-%s" + + // Number of random bytes (hex-encoded, so twice this many characters) + // appended to each sentinel by randomSentinelSuffix. + conflictSentinelSuffixBytes = 8 ) // YAMLMerger handles 3-way merging of YAML files with structure awareness. @@ -65,20 +87,19 @@ func (m *YAMLMerger) SetConflictStrategy(strategy ConflictStrategy) { } // pickConflictValue resolves a real ours/theirs divergence per the configured -// conflict strategy. Manual (default) still records the conflict via -// conflicts.addConflict — MergeResult.HasConflicts then aborts the write in -// engine.Processor.mergeFile, which is today's existing "surface, don't -// silently pick a side" behavior. Ours/theirs pick a side and deliberately do -// not record a conflict, so the write proceeds. -func (m *YAMLMerger) pickConflictValue(ours, theirs *yaml.Node, path string, conflicts *conflictTracker) *yaml.Node { +// conflict strategy. Manual (default) records the conflict and returns a +// sentinel placeholder node (see addNodeConflict) so the real markers can be +// spliced in once the whole document has been rendered to text. Ours/theirs +// pick a side and deliberately do not record a conflict, so the write +// proceeds with no markers. +func (m *YAMLMerger) pickConflictValue(ours, theirs *yaml.Node, path string, conflicts *conflictTracker) (*yaml.Node, error) { switch m.conflictStrategy { case ConflictStrategyTheirs: - return theirs + return theirs, nil case ConflictStrategyOurs: - return ours + return ours, nil default: - conflicts.addConflict(path) - return ours + return conflicts.addNodeConflict(path, ours, theirs) } } @@ -114,7 +135,7 @@ func (m *YAMLMerger) Merge(base, ours, theirs string) (*MergeResult, error) { } // Perform structure-aware merge, document by document. - conflicts := &conflictTracker{conflicts: make([]string, 0)} + conflicts := &conflictTracker{conflicts: make([]string, 0), forbidden: base + ours + theirs} mergedDocs, err := m.mergeDocumentStreams(baseDocs, oursDocs, theirsDocs, conflicts) if err != nil { return nil, errUtils.Build(errUtils.ErrThreeWayMerge). @@ -130,7 +151,7 @@ func (m *YAMLMerger) Merge(base, ours, theirs string) (*MergeResult, error) { if changePercentage > m.thresholdPercent { return nil, errUtils.Build(errUtils.ErrMergeThresholdExceeded). WithExplanationf("Too many YAML conflicts detected (%d%% changes, threshold: %d%%). %d conflicts found", changePercentage, m.thresholdPercent, len(conflicts.conflicts)). - WithHint("Use --force to overwrite or manually merge"). + WithHint("Use --force (resolves every conflict to the template's version) or manually merge"). Err() } } @@ -159,10 +180,21 @@ func (m *YAMLMerger) Merge(base, ours, theirs string) (*MergeResult, error) { Err() } + content := buf.String() + if len(conflicts.nodeConflicts) > 0 { + spliced, err := spliceConflictMarkers(content, conflicts.nodeConflicts) + if err != nil { + return nil, err + } + content = spliced + } + return &MergeResult{ - Content: buf.String(), + Content: content, HasConflicts: len(conflicts.conflicts) > 0, + HasMarkers: len(conflicts.nodeConflicts) > 0, ConflictCount: len(conflicts.conflicts), + ConflictPaths: conflicts.conflicts, }, nil } @@ -247,14 +279,352 @@ func (m *YAMLMerger) mergeDocumentStreams(baseDocs, oursDocs, theirsDocs []*yaml // conflictTracker keeps track of conflicts during merge. type conflictTracker struct { - conflicts []string + conflicts []string + nodeConflicts []nodeConflict + // forbidden is the concatenation of the raw base/ours/theirs text being + // merged. addNodeConflict never hands out a sentinel that appears + // anywhere in it, so a pre-existing scalar value coincidentally equal to + // a sentinel can never be mistaken for the inserted conflict placeholder + // by spliceConflictMarkers/findSentinel. + forbidden string } -// addConflict records a conflict at the given path. +// nodeConflict records a real ours/theirs divergence at a specific path, plus +// the two competing subtrees, so spliceConflictMarkers can render both sides +// and inject diff3-style markers at the sentinel's exact location in the +// encoded text. +type nodeConflict struct { + sentinel string + ours *yaml.Node + theirs *yaml.Node +} + +// addConflict records a conflict at the given path. Used both by +// addNodeConflict below and by the document-stream-level conflict case +// (a document the template changed that the user's stream dropped), which +// has no ours/theirs node pair to splice markers from. func (c *conflictTracker) addConflict(path string) { c.conflicts = append(c.conflicts, path) } +// addNodeConflict records a real ours/theirs divergence and returns a unique +// sentinel scalar node to embed in the tree at that location. The sentinel +// carries ours' comments so they aren't silently dropped from the encoded +// output. +func (c *conflictTracker) addNodeConflict(path string, ours, theirs *yaml.Node) (*yaml.Node, error) { + c.addConflict(path) + sentinel, err := c.nextSentinel() + if err != nil { + return nil, err + } + c.nodeConflicts = append(c.nodeConflicts, nodeConflict{sentinel: sentinel, ours: ours, theirs: theirs}) + return &yaml.Node{ + Kind: yaml.ScalarNode, + Tag: "!!str", + Value: sentinel, + HeadComment: ours.HeadComment, + LineComment: ours.LineComment, + FootComment: ours.FootComment, + }, nil +} + +// nextSentinel returns a sentinel guaranteed unique among every sentinel +// issued so far for this merge (the fixed-width index) and absent from every +// document being merged (the random suffix, regenerated on the vanishingly +// rare collision against c.forbidden). See conflictSentinelFormat. +func (c *conflictTracker) nextSentinel() (string, error) { + index := len(c.nodeConflicts) + for { + suffix, err := randomSentinelSuffix() + if err != nil { + return "", err + } + sentinel := fmt.Sprintf(conflictSentinelFormat, index, suffix) + if !strings.Contains(c.forbidden, sentinel) { + return sentinel, nil + } + } +} + +// cryptoRandRead is rand.Read, indirected through a package-level var so +// tests can force a failure of randomSentinelSuffix without an injectable +// dependency-injection layer -- crypto/rand.Reader itself never errors on +// any platform Atmos supports, so this failure path is otherwise +// unreachable from a test. +var cryptoRandRead = rand.Read + +// randomSentinelSuffix returns a random hex string that makes each sentinel +// unpredictable, so a value already present in the document being merged +// cannot coincide with one -- neither by chance nor by construction. +func randomSentinelSuffix() (string, error) { + buf := make([]byte, conflictSentinelSuffixBytes) + if _, err := cryptoRandRead(buf); err != nil { + return "", errUtils.Build(errUtils.ErrThreeWayMerge). + WithCause(err). + WithExplanation("Failed to generate a random conflict marker"). + Err() + } + return hex.EncodeToString(buf), nil +} + +// spliceConflictMarkers replaces each conflict sentinel embedded in the +// encoded YAML text with real diff3-style conflict markers. It works line by +// line: for each line containing a sentinel, it uses the text before the +// sentinel (e.g. " setting: " or " - ") and the line's indentation to +// reconstruct both sides of the conflict, rendered independently as their own +// YAML fragments. +// +// Flow-style YAML (`{a: 1, b: 2}`) can place more than one sentinel on the +// same line; renderConflictBlock/appendTailToLastLine drain every sentinel +// on the line into nested marker blocks instead of leaving the extra one as +// literal placeholder text. This is genuinely rare in practice -- scaffold +// templates use block style -- and the nested-marker output it produces is +// harder to hand-resolve than a single-conflict line, but it never silently +// drops or corrupts content. +// +// Trailing syntax after the sentinel on that line (e.g. a flow-style closing +// `}`/`]`, or an inline comment) is appended to *both* alternatives' own +// content, not just tacked onto a marker line -- see renderConflictBlock -- +// so it survives regardless of which alternative (or which marker lines) a +// manual resolution ends up deleting. +func spliceConflictMarkers(yamlText string, conflicts []nodeConflict) (string, error) { + bySentinel := make(map[string]nodeConflict, len(conflicts)) + for _, c := range conflicts { + bySentinel[c.sentinel] = c + } + + lines := strings.Split(yamlText, newlineSeparator) + out := make([]string, 0, len(lines)) + for _, line := range lines { + conflict, sentinel, idx := findSentinel(line, bySentinel) + if idx == -1 { + out = append(out, line) + continue + } + + block, err := renderConflictBlock(line, idx, sentinel, conflict, bySentinel) + if err != nil { + return "", err + } + out = append(out, block...) + } + return strings.Join(out, newlineSeparator), nil +} + +// findSentinel returns the conflict, sentinel string, and byte index of the +// earliest sentinel found in line, or idx == -1 if none is present. +// +// Note: bySentinel is a Go map, so ranging over it in iteration order (as an +// earlier version of this function did) picks a different sentinel on every +// call when a line holds more than one -- the documented flow-style case +// below. Comparing byte indices instead makes the choice deterministic +// regardless of map iteration order. +func findSentinel(line string, bySentinel map[string]nodeConflict) (nodeConflict, string, int) { + best := -1 + var bestSentinel string + var bestConflict nodeConflict + for sentinel, c := range bySentinel { + if idx := strings.Index(line, sentinel); idx != -1 && (best == -1 || idx < best) { + best, bestSentinel, bestConflict = idx, sentinel, c + } + } + return bestConflict, bestSentinel, best +} + +// renderConflictBlock builds the replacement lines for a single sentinel +// occurrence, choosing inline markers (both sides are scalars, so the +// conflict fits on the same line as the key/list-item marker) or block +// markers (either side is a mapping/sequence, so the conflict needs its own +// indented block beneath the key). +// +// Note: bySentinel lets appendTailToLastLine drain any further sentinel +// still embedded in suffix (two conflicts landed on the same original line) +// into its own nested block, rather than leaving it as literal sentinel +// text. +func renderConflictBlock(line string, idx int, sentinel string, c nodeConflict, bySentinel map[string]nodeConflict) ([]string, error) { + prefix := line[:idx] + suffix := line[idx+len(sentinel):] + trimmed := strings.TrimLeft(line, " ") + indent := line[:len(line)-len(trimmed)] + + return renderConflictBlockWithIndent(indent, prefix, suffix, c, bySentinel) +} + +// renderConflictBlockWithIndent is renderConflictBlock's core, factored out +// so appendTailToLastLine can render a nested conflict's markers at an +// already-known indent without a real source line to recompute one from. +func renderConflictBlockWithIndent(indent, prefix, suffix string, c nodeConflict, bySentinel map[string]nodeConflict) ([]string, error) { + oursText, err := encodeNodeFragment(c.ours) + if err != nil { + return nil, err + } + theirsText, err := encodeNodeFragment(c.theirs) + if err != nil { + return nil, err + } + + parts := conflictBlockParts{indent: indent, prefix: prefix, suffix: suffix, oursText: oursText, theirsText: theirsText} + if c.ours.Kind == yaml.ScalarNode && c.theirs.Kind == yaml.ScalarNode { + return inlineConflictBlock(&parts, bySentinel) + } + return blockConflictBlock(&parts, bySentinel) +} + +// conflictBlockParts bundles inlineConflictBlock/blockConflictBlock's +// rendering inputs (grouped into a struct, rather than five separate +// parameters, to stay under revive's argument-limit alongside the +// bySentinel map appendTailToLastLine needs for draining). +type conflictBlockParts struct { + indent string + prefix string + suffix string + oursText string + theirsText string +} + +// inlineConflictBlock reconstructs a conflict where both sides are scalars, +// so the value fits on the same line as the reconstructed key/list-item +// prefix, e.g.: +// +// <<<<<<< Ours +// setting: user-change +// ======= +// setting: template-change +// >>>>>>> Theirs +func inlineConflictBlock(p *conflictBlockParts, bySentinel map[string]nodeConflict) ([]string, error) { + oursLines := strings.Split(strings.TrimRight(p.oursText, newlineSeparator), newlineSeparator) + theirsLines := strings.Split(strings.TrimRight(p.theirsText, newlineSeparator), newlineSeparator) + + block := []string{p.indent + "<<<<<<< Ours", p.prefix + oursLines[0]} + for _, l := range oursLines[1:] { + block = append(block, p.indent+l) + } + block, err := appendTailToLastLine(block, p.indent, p.suffix, bySentinel) + if err != nil { + return nil, err + } + + block = append(block, p.indent+"=======", p.prefix+theirsLines[0]) + for _, l := range theirsLines[1:] { + block = append(block, p.indent+l) + } + block, err = appendTailToLastLine(block, p.indent, p.suffix, bySentinel) + if err != nil { + return nil, err + } + + return append(block, p.indent+">>>>>>> Theirs"), nil +} + +// appendTailToLastLine appends tail -- whatever trailing syntax followed the +// sentinel on the original line (e.g. a flow-style closing `}`/`]`, or an +// inline comment) -- to block's last line in place, so it survives on +// whichever alternative a manual resolution ends up keeping, instead of only +// being tacked onto a marker line that a resolution deletes along with the +// alternative it didn't choose. +// +// Skipped when that line already ends with tail: addNodeConflict's sentinel +// carries ours' own LineComment, so when tail is that same comment, +// encodeNodeFragment(c.ours) already rendered it as part of ours' own text. +// +// Appending it again would duplicate it. By contrast, theirs never has +// ours' comment, so this guard is a no-op there and the append always +// applies. +// +// When tail itself still holds another sentinel -- two conflicts landed on +// the same original line -- the text before it is appended as usual, then +// that conflict's own markers are spliced in as a nested block at indent, +// and whatever trails it is processed the same way recursively, so every +// sentinel on the line is drained instead of the extra one reaching the +// output as literal text. +func appendTailToLastLine(block []string, indent, tail string, bySentinel map[string]nodeConflict) ([]string, error) { + if tail == "" { + return block, nil + } + last := len(block) - 1 + if strings.HasSuffix(block[last], tail) { + return block, nil + } + + conflict, sentinel, idx := findSentinel(tail, bySentinel) + if idx == -1 { + block[last] += tail + return block, nil + } + + block[last] += tail[:idx] + nested, err := renderConflictBlockWithIndent(indent, "", tail[idx+len(sentinel):], conflict, bySentinel) + if err != nil { + return nil, err + } + return append(block, nested...), nil +} + +// blockConflictBlock reconstructs a conflict where either side is a +// mapping/sequence, so both sides are rendered as their own indented block +// beneath the key, e.g.: +// +// nested: +// <<<<<<< Ours +// a: 1 +// ======= +// a: 2 +// b: 3 +// >>>>>>> Theirs +func blockConflictBlock(p *conflictBlockParts, bySentinel map[string]nodeConflict) ([]string, error) { + nested := p.indent + " " + + oursLines := strings.Split(strings.TrimRight(p.oursText, newlineSeparator), newlineSeparator) + theirsLines := strings.Split(strings.TrimRight(p.theirsText, newlineSeparator), newlineSeparator) + + // A nested block (see appendTailToLastLine) has no key of its own -- the + // key already appeared on the outer conflict's line -- so prefix is "". + var block []string + if keyLine := strings.TrimRight(p.prefix, " "); keyLine != "" { + block = append(block, keyLine) + } + block = append(block, nested+"<<<<<<< Ours") + for _, l := range oursLines { + block = append(block, nested+l) + } + block, err := appendTailToLastLine(block, nested, p.suffix, bySentinel) + if err != nil { + return nil, err + } + + block = append(block, nested+"=======") + for _, l := range theirsLines { + block = append(block, nested+l) + } + block, err = appendTailToLastLine(block, nested, p.suffix, bySentinel) + if err != nil { + return nil, err + } + + return append(block, nested+">>>>>>> Theirs"), nil +} + +// encodeNodeFragment encodes a single YAML node (not necessarily a document) +// on its own, starting at column 0, for embedding in a conflict block. +func encodeNodeFragment(node *yaml.Node) (string, error) { + var buf bytes.Buffer + encoder := yaml.NewEncoder(&buf) + encoder.SetIndent(2) + if err := encoder.Encode(node); err != nil { + return "", errUtils.Build(errUtils.ErrEncode). + WithCause(err). + WithExplanation("Failed to encode conflicting YAML fragment"). + Err() + } + if err := encoder.Close(); err != nil { + return "", errUtils.Build(errUtils.ErrEncode). + WithCause(err). + WithExplanation("Failed to close YAML encoder for conflicting fragment"). + Err() + } + return buf.String(), nil +} + // mergeNodes recursively merges YAML nodes. // //nolint:revive // cyclomatic: 3-way merge requires handling multiple node states @@ -285,7 +655,7 @@ func (m *YAMLMerger) mergeNodes(base, ours, theirs *yaml.Node, path string, conf // when ours is a MappingNode). Record a conflict and preserve the user's // version whenever kinds diverge. if ours.Kind != base.Kind || theirs.Kind != base.Kind { - return m.pickConflictValue(ours, theirs, path, conflicts), nil + return m.pickConflictValue(ours, theirs, path, conflicts) } // Handle based on node type. @@ -351,7 +721,7 @@ func (m *YAMLMerger) mergeMappings(base, ours, theirs *yaml.Node, path string, c // If there's a kind mismatch, resolve per the configured conflict strategy. if !baseIsMapping || !oursIsMapping || !theirsIsMapping { - return m.pickConflictValue(ours, theirs, path, conflicts), nil + return m.pickConflictValue(ours, theirs, path, conflicts) } result := &yaml.Node{ @@ -407,7 +777,10 @@ func (m *YAMLMerger) mergeMappings(base, ours, theirs *yaml.Node, path string, c case !inBase && inTheirs: // Both added the same key - merge values. if oursValue.Kind != theirsValue.Kind { - picked := m.pickConflictValue(oursValue, theirsValue, keyPath, conflicts) + picked, err := m.pickConflictValue(oursValue, theirsValue, keyPath, conflicts) + if err != nil { + return nil, err + } result.Content = append(result.Content, keyNode, picked) continue } @@ -465,7 +838,18 @@ func (m *YAMLMerger) mergeSequences(_, ours, theirs *yaml.Node, path string, con // conflict strategy (manual/ours/theirs); identical sequences need no choice. picked := ours if !nodesEqual(ours, theirs) { - picked = m.pickConflictValue(ours, theirs, path, conflicts) + var err error + picked, err = m.pickConflictValue(ours, theirs, path, conflicts) + if err != nil { + return nil, err + } + } + + // Manual strategy on a real divergence returns a scalar sentinel (see + // addNodeConflict), not a sequence — return it as-is rather than + // wrapping it in a SequenceNode below, which would discard its value. + if picked.Kind != yaml.SequenceNode { + return picked, nil } // Preserve the picked side's comments and style. @@ -488,7 +872,11 @@ func (m *YAMLMerger) mergeScalars(base, ours, theirs *yaml.Node, path string, co // the configured conflict strategy (manual/ours/theirs). picked := ours if ours.Value != base.Value && theirs.Value != base.Value && ours.Value != theirs.Value { - picked = m.pickConflictValue(ours, theirs, path, conflicts) + var err error + picked, err = m.pickConflictValue(ours, theirs, path, conflicts) + if err != nil { + return nil, err + } } // Preserve the picked side's comments, tag, and style (folding, literal, etc.) diff --git a/pkg/generator/merge/yaml_merger_test.go b/pkg/generator/merge/yaml_merger_test.go index 82b68336e09..c44662684be 100644 --- a/pkg/generator/merge/yaml_merger_test.go +++ b/pkg/generator/merge/yaml_merger_test.go @@ -1,12 +1,16 @@ package merge import ( + "errors" + "fmt" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" + + errUtils "github.com/cloudposse/atmos/errors" ) func TestYAMLMerger_CleanMerges(t *testing.T) { @@ -765,10 +769,34 @@ func TestYAMLMerger_KindDivergencePreservesOursAndRecordsConflict(t *testing.T) require.NoError(t, err) require.NotNil(t, result) assert.True(t, result.HasConflicts) + assert.True(t, result.HasMarkers, "a real ours/theirs divergence always has a node pair to splice markers from") assert.Equal(t, 1, result.ConflictCount) assert.Contains(t, result.Content, "nested: true") } +// TestYAMLMerger_DroppedDocumentConflictHasNoMarkers covers +// mergeDocumentStreams' ours==nil branch: the user's stream dropped a +// document the template went on to change. That's recorded as a conflict +// (via addConflict) with no ours/theirs node pair to splice inline markers +// from, so HasConflicts is true but HasMarkers must be false -- unlike every +// other conflict this merger records, which always comes from addNodeConflict +// and therefore always has a marker. +func TestYAMLMerger_DroppedDocumentConflictHasNoMarkers(t *testing.T) { + base := "doc: one\n---\ndoc: two\n" + ours := "doc: one\n" + theirs := "doc: one\n---\ndoc: two\ntemplate: true\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + + require.NoError(t, err) + require.NotNil(t, result) + assert.True(t, result.HasConflicts) + assert.False(t, result.HasMarkers, "a dropped document has no node pair to splice markers from") + assert.Equal(t, []string{"documents[1]"}, result.ConflictPaths) + assert.NotContains(t, result.Content, "<<<<<<<") + assert.Contains(t, result.Content, "template: true") +} + func TestYAMLMerger_ComplexMappingKeyError(t *testing.T) { base := "? [a, b]\n: value\n" ours := "? [a, b]\n: user\n" @@ -916,6 +944,319 @@ func TestYAMLMerger_PreservesTagsAndStyle(t *testing.T) { } } +// TestYAMLMerger_ConflictMarkers_Scalar covers the inline-marker path: both +// sides of the conflict are scalars, so the reconstructed markers fit on the +// same line as the key. This is the exact shape from the original bug report +// (github.com/cloudposse/atmos/issues/2912): a scalar value diverges on both +// sides while an unrelated key is added by each side too. +func TestYAMLMerger_ConflictMarkers_Scalar(t *testing.T) { + base := "setting: original\nkey1: v1\n" + ours := "setting: user-change\nkey1: v1\ncustom: mine\n" + theirs := "setting: template-change\nkey1: v1\nfeature: enabled\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + require.Equal(t, []string{"documents[0].setting"}, result.ConflictPaths) + + assert.Equal(t, `<<<<<<< Ours +setting: user-change +======= +setting: template-change +>>>>>>> Theirs +key1: v1 +custom: mine +feature: enabled +`, result.Content) +} + +// TestYAMLMerger_ConflictMarkers_KindDivergence covers the block-marker path: +// one side is a scalar and the other a mapping (a real structural +// divergence), so the reconstructed markers wrap an indented block beneath +// the key rather than fitting inline. +func TestYAMLMerger_ConflictMarkers_KindDivergence(t *testing.T) { + base := "key: value\n" + ours := "key:\n nested: true\n" + theirs := "key:\n - item\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + + assert.Equal(t, `key: + <<<<<<< Ours + nested: true + ======= + - item + >>>>>>> Theirs +`, result.Content) +} + +// TestYAMLMerger_ConflictMarkers_MultipleConflictsDoNotCollide guards the +// fixed-width sentinel format: with more than 10 conflicts in one document, a +// naive substring search (e.g. sentinel "...0" matching inside "...01") would +// misattribute markers to the wrong conflict. Every conflict here must +// resolve to its own value on both sides. +func TestYAMLMerger_ConflictMarkers_MultipleConflictsDoNotCollide(t *testing.T) { + var base, ours, theirs strings.Builder + const count = 12 + for i := 0; i < count; i++ { + fmt.Fprintf(&base, "k%d: base%d\n", i, i) + fmt.Fprintf(&ours, "k%d: ours%d\n", i, i) + fmt.Fprintf(&theirs, "k%d: theirs%d\n", i, i) + } + + result, err := NewYAMLMerger(100).Merge(base.String(), ours.String(), theirs.String()) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, count, result.ConflictCount) + + for i := 0; i < count; i++ { + assert.Contains(t, result.Content, fmt.Sprintf("k%d: ours%d", i, i)) + assert.Contains(t, result.Content, fmt.Sprintf("k%d: theirs%d", i, i)) + } + assert.Equal(t, count, strings.Count(result.Content, "<<<<<<< Ours")) + assert.Equal(t, count, strings.Count(result.Content, ">>>>>>> Theirs")) +} + +// TestYAMLMerger_ConflictMarkers_PreexistingSentinelLookalike guards against +// sentinel collisions: a scalar value already equal to the old, purely +// sequential sentinel format (ATMOSMERGECONFLICT000000) must not be mistaken +// for a real conflict placeholder by findSentinel/spliceConflictMarkers, and +// must survive an unrelated conflict elsewhere in the document untouched. +// The random suffix and forbidden-corpus check in conflictTracker.nextSentinel +// guarantee this; without them, this literal value colliding with the first +// sentinel issued would corrupt it in unpredictable ways. +func TestYAMLMerger_ConflictMarkers_PreexistingSentinelLookalike(t *testing.T) { + base := "setting: original\nlookalike: ATMOSMERGECONFLICT000000\n" + ours := "setting: user-change\nlookalike: ATMOSMERGECONFLICT000000\n" + theirs := "setting: template-change\nlookalike: ATMOSMERGECONFLICT000000\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + + // The lookalike value is identical on all three sides, so it must pass + // through untouched -- appearing exactly once, and not wrapped in (or + // replaced by) conflict markers meant for the unrelated "setting" key. + assert.Equal(t, 1, strings.Count(result.Content, "ATMOSMERGECONFLICT000000")) + assert.Equal(t, `<<<<<<< Ours +setting: user-change +======= +setting: template-change +>>>>>>> Theirs +lookalike: ATMOSMERGECONFLICT000000 +`, result.Content) +} + +// TestYAMLMerger_RandomSentinelSuffixFailurePropagates forces +// cryptoRandRead (the crypto/rand.Read indirection used by +// randomSentinelSuffix) to fail, and asserts a real ours/theirs divergence +// surfaces that failure as ErrThreeWayMerge instead of silently succeeding. +// Note: crypto/rand.Reader itself never errors on any platform Atmos +// supports, so this branch (and everything upstream that propagates its +// error -- nextSentinel, addNodeConflict, pickConflictValue, and every +// mergeNodes/mergeMappings/mergeSequences/mergeScalars call site above +// them) is otherwise unreachable from a test. +func TestYAMLMerger_RandomSentinelSuffixFailurePropagates(t *testing.T) { + original := cryptoRandRead + injectedErr := errors.New("injected rand failure") + cryptoRandRead = func([]byte) (int, error) { return 0, injectedErr } + t.Cleanup(func() { cryptoRandRead = original }) + + _, err := NewYAMLMerger(100).Merge("setting: original\n", "setting: user-change\n", "setting: template-change\n") + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrThreeWayMerge) +} + +// TestYAMLMerger_RandomSentinelSuffixFailurePropagates_SequenceConflict is +// the mergeSequences counterpart of +// TestYAMLMerger_RandomSentinelSuffixFailurePropagates: a real ours/theirs +// sequence divergence routes through mergeSequences' own pickConflictValue +// call instead of mergeMappings'. +func TestYAMLMerger_RandomSentinelSuffixFailurePropagates_SequenceConflict(t *testing.T) { + original := cryptoRandRead + injectedErr := errors.New("injected rand failure") + cryptoRandRead = func([]byte) (int, error) { return 0, injectedErr } + t.Cleanup(func() { cryptoRandRead = original }) + + _, err := NewYAMLMerger(100).Merge("items:\n - a\n", "items:\n - b\n", "items:\n - c\n") + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrThreeWayMerge) +} + +// TestYAMLMerger_RandomSentinelSuffixFailurePropagates_AddedKeyKindMismatch +// covers mergeMappings' "!inBase && inTheirs" branch, where both sides add +// the same key but with different node kinds -- a distinct pickConflictValue +// call site from the "all three have the key" case the base +// TestYAMLMerger_RandomSentinelSuffixFailurePropagates test exercises. +func TestYAMLMerger_RandomSentinelSuffixFailurePropagates_AddedKeyKindMismatch(t *testing.T) { + original := cryptoRandRead + injectedErr := errors.New("injected rand failure") + cryptoRandRead = func([]byte) (int, error) { return 0, injectedErr } + t.Cleanup(func() { cryptoRandRead = original }) + + _, err := NewYAMLMerger(100).Merge("base: unrelated\n", "base: unrelated\nnewkey: scalar\n", "base: unrelated\nnewkey:\n nested: true\n") + + require.Error(t, err) + assert.ErrorIs(t, err, errUtils.ErrThreeWayMerge) +} + +// TestYAMLMerger_ConflictMarkers_MultilineScalar covers inlineConflictBlock's +// loop over additional lines of a multi-line scalar (e.g. a block literal `|` +// value): both ours and theirs are still ScalarNode, so the conflict renders +// inline, but each side spans more than one line once re-encoded. +func TestYAMLMerger_ConflictMarkers_MultilineScalar(t *testing.T) { + base := "setting: |\n line1\n" + ours := "setting: |\n ours-line1\n ours-line2\n" + theirs := "setting: |\n theirs-line1\n theirs-line2\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + + assert.Equal(t, `<<<<<<< Ours +setting: | + ours-line1 + ours-line2 +======= +setting: | + theirs-line1 + theirs-line2 +>>>>>>> Theirs +`, result.Content) +} + +// TestYAMLMerger_ConflictMarkers_SuffixPreservedForBothAlternatives covers a +// real ours/theirs divergence where the sentinel's line has trailing content +// after it -- here, an inline comment addNodeConflict carries over from +// ours' own LineComment (see addNodeConflict). That trailing text must +// survive on *both* reconstructed alternatives, not just be tacked onto the +// closing >>>>>>> Theirs marker line: only one alternative survives manual +// resolution, and a resolution that deletes the theirs block (or just the +// marker lines) must not silently drop it. +func TestYAMLMerger_ConflictMarkers_SuffixPreservedForBothAlternatives(t *testing.T) { + base := "key: original\n" + ours := "key: user-change # user note\n" + theirs := "key: template-change\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + + assert.Equal(t, `<<<<<<< Ours +key: user-change # user note +======= +key: template-change # user note +>>>>>>> Theirs +`, result.Content) +} + +// TestYAMLMerger_ConflictMarkers_FlowStyleSuffixPreserved covers a +// divergence inside a flow-style mapping: the sentinel replaces only key +// "a"'s value, so the rest of the flow mapping (", b: 2}") trails the +// sentinel on the same encoded line. That trailing text must close out +// *both* reconstructed alternatives so each remains a syntactically valid, +// self-contained flow mapping on its own -- not just close out whichever one +// happens to sit next to the >>>>>>> Theirs marker. +func TestYAMLMerger_ConflictMarkers_FlowStyleSuffixPreserved(t *testing.T) { + base := "obj: {a: 1, b: 2}\n" + ours := "obj: {a: user, b: 2}\n" + theirs := "obj: {a: template, b: 2}\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 1, result.ConflictCount) + + assert.Equal(t, `<<<<<<< Ours +obj: {a: user, b: 2} +======= +obj: {a: template, b: 2} +>>>>>>> Theirs +`, result.Content) +} + +// TestFindSentinel_DeterministicEarliestByteIndex guards against +// findSentinel silently depending on Go's randomized map iteration order: +// when a line holds more than one sentinel (the documented flow-style case +// in spliceConflictMarkers' doc comment), it must always pick the one with +// the smallest byte index, not whichever the map happens to yield first. +// Ranging over the same two-entry map many times exercises different +// iteration orders, so the old implementation (return on first map hit) +// would have failed this near-certainly before it ever reached run 50. +func TestFindSentinel_DeterministicEarliestByteIndex(t *testing.T) { + first := nodeConflict{sentinel: "ATMOSMERGECONFLICT000000-aaaaaaaa"} + second := nodeConflict{sentinel: "ATMOSMERGECONFLICT000001-bbbbbbbb"} + line := "obj: {a: " + first.sentinel + ", b: " + second.sentinel + "}" + bySentinel := map[string]nodeConflict{ + first.sentinel: first, + second.sentinel: second, + } + + for i := 0; i < 50; i++ { + conflict, sentinel, idx := findSentinel(line, bySentinel) + assert.Equal(t, first.sentinel, sentinel, "must always pick the earliest sentinel by byte index") + assert.Equal(t, first, conflict) + assert.Equal(t, strings.Index(line, first.sentinel), idx) + } +} + +// TestYAMLMerger_ConflictMarkers_DrainsMultipleSentinelsOnOneLine covers two +// independent conflicts (keys "a" and "b") landing on the same flow-style +// line. Both must be drained into real, nested diff3 markers. +// +// The second sentinel must never reach the output as literal placeholder +// text (see appendTailToLastLine). "b"'s conflict is nested once under each +// of "a"'s two alternatives (three <<<<<<< Ours/>>>>>>> Theirs pairs total, +// not two): resolving "a" one way or the other still leaves "b" to resolve +// independently, so each of "a"'s alternatives needs its own copy of "b"'s +// markers rather than sharing one. +func TestYAMLMerger_ConflictMarkers_DrainsMultipleSentinelsOnOneLine(t *testing.T) { + base := "obj: {a: 1, b: 2}\n" + ours := "obj: {a: user-a, b: user-b}\n" + theirs := "obj: {a: template-a, b: template-b}\n" + + result, err := NewYAMLMerger(100).Merge(base, ours, theirs) + require.NoError(t, err) + require.True(t, result.HasConflicts) + require.Equal(t, 2, result.ConflictCount) + + assert.NotContains(t, result.Content, "ATMOSMERGECONFLICT", + "every sentinel must be drained into real markers, none left as literal placeholder text") + // Built line-by-line (rather than a raw string literal) since "obj: {a: + // user-a, b: " genuinely ends in a trailing space -- the original ": " + // key-value separator, immediately followed by the nested block on its + // own line -- and a literal trailing space in a backtick string is easy + // to lose to editor/linter whitespace trimming. + wantLines := []string{ + "<<<<<<< Ours", + "obj: {a: user-a, b: ", + "<<<<<<< Ours", + "user-b}", + "=======", + "template-b}", + ">>>>>>> Theirs", + "=======", + "obj: {a: template-a, b: ", + "<<<<<<< Ours", + "user-b}", + "=======", + "template-b}", + ">>>>>>> Theirs", + ">>>>>>> Theirs", + "", + } + assert.Equal(t, strings.Join(wantLines, "\n"), result.Content) +} + func TestYAMLMerger_PreservesLineComments(t *testing.T) { tests := []struct { name string diff --git a/pkg/generator/storage/metadata.go b/pkg/generator/storage/metadata.go index 02a5498cffb..d748bdcf7ae 100644 --- a/pkg/generator/storage/metadata.go +++ b/pkg/generator/storage/metadata.go @@ -62,6 +62,14 @@ func ScaffoldMetadataPath(targetDir string) string { return filepath.Join(targetDir, ".atmos", "scaffold", "metadata.yaml") } +// InitMetadataPath returns the path to an init-generated target directory's +// persisted generation metadata (.atmos/init/metadata.yaml). +func InitMetadataPath(targetDir string) string { + defer perf.Track(nil, "storage.InitMetadataPath")() + + return filepath.Join(targetDir, ".atmos", "init", "metadata.yaml") +} + // NewMetadataStorage creates a new metadata storage for the given metadata file path. // For init: .atmos/init/metadata.yaml // For scaffold: .atmos/scaffold/metadata.yaml. diff --git a/pkg/generator/storage/metadata_test.go b/pkg/generator/storage/metadata_test.go index 2c81f90bee4..20d1435098c 100644 --- a/pkg/generator/storage/metadata_test.go +++ b/pkg/generator/storage/metadata_test.go @@ -25,6 +25,19 @@ func TestMetadataStorage_GetMetadataPath(t *testing.T) { assert.Equal(t, path, storage.GetMetadataPath()) } +// TestInitMetadataPath verifies `atmos init` reads/writes its pinned +// generation metadata at .atmos/init/metadata.yaml, distinct from +// ScaffoldMetadataPath's .atmos/scaffold/metadata.yaml -- the two commands +// must not clobber each other's pin when generating into the same directory. +func TestInitMetadataPath(t *testing.T) { + targetDir := filepath.Join("some", "target", "dir") + + got := InitMetadataPath(targetDir) + + assert.Equal(t, filepath.Join(targetDir, ".atmos", "init", "metadata.yaml"), got) + assert.NotEqual(t, ScaffoldMetadataPath(targetDir), got) +} + func TestMetadataStorage_Exists(t *testing.T) { tests := []struct { name string