From fba25240814399948165d2457bf886e5872a741e Mon Sep 17 00:00:00 2001 From: Erik Rasmussen Date: Mon, 19 Aug 2024 00:13:53 -0500 Subject: [PATCH] Fix pointer diffs always triggering (#155) * Fix the mktemp diff * More better diffs --- provider/pkg/provider/cmd/diff.go | 12 +++++++ provider/pkg/provider/coreutils/mktemp.go | 20 +++++------ provider/pkg/provider/coreutils/mv.go | 20 +++++------ provider/pkg/provider/coreutils/rm.go | 8 ++--- provider/pkg/provider/coreutils/tee.go | 4 +-- tests/lifecycle/mktemp_test.go | 43 +++++++++++++++++++++-- 6 files changed, 77 insertions(+), 30 deletions(-) diff --git a/provider/pkg/provider/cmd/diff.go b/provider/pkg/provider/cmd/diff.go index 03904f33..d35757fa 100644 --- a/provider/pkg/provider/cmd/diff.go +++ b/provider/pkg/provider/cmd/diff.go @@ -23,3 +23,15 @@ func (s *State[T]) Diff(ctx context.Context, inputs CommandArgs[T]) (map[string] return diff, nil } + +func Changed[T comparable](a *T, b *T) bool { + if a == b { + return false + } + + if a == nil || b == nil { + return true + } + + return *a != *b +} diff --git a/provider/pkg/provider/coreutils/mktemp.go b/provider/pkg/provider/coreutils/mktemp.go index 28f5ee42..c6940342 100644 --- a/provider/pkg/provider/coreutils/mktemp.go +++ b/provider/pkg/provider/coreutils/mktemp.go @@ -69,38 +69,34 @@ func (Mktemp) Diff(ctx context.Context, id string, olds MktempState, news cmd.Co return provider.DiffResponse{}, fmt.Errorf("mv: %w", err) } - if news.Args.Directory != olds.Args.Directory { + if cmd.Changed(news.Args.Directory, olds.Args.Directory) { diff["args.directory"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.DryRun != olds.Args.DryRun { - diff["args.destination"] = provider.PropertyDiff{Kind: provider.UpdateReplace} + if cmd.Changed(news.Args.DryRun, olds.Args.DryRun) { + diff["args.dryRun"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Quiet != olds.Args.Quiet { + if cmd.Changed(news.Args.Quiet, olds.Args.Quiet) { diff["args.quiet"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Suffix != olds.Args.Suffix { + if cmd.Changed(news.Args.Suffix, olds.Args.Suffix) { diff["args.suffix"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.T != olds.Args.T { + if cmd.Changed(news.Args.T, olds.Args.T) { diff["args.t"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Template != olds.Args.Template { + if cmd.Changed(news.Args.Template, olds.Args.Template) { diff["args.template"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.TmpDir != olds.Args.TmpDir { + if cmd.Changed(news.Args.TmpDir, olds.Args.TmpDir) { diff["args.tmpDir"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Directory != olds.Args.Directory { - diff["args.suffix"] = provider.PropertyDiff{Kind: provider.UpdateReplace} - } - return provider.DiffResponse{ DeleteBeforeReplace: true, HasChanges: len(diff) > 0, diff --git a/provider/pkg/provider/coreutils/mv.go b/provider/pkg/provider/coreutils/mv.go index 4ed8470b..93a7ce4a 100644 --- a/provider/pkg/provider/coreutils/mv.go +++ b/provider/pkg/provider/coreutils/mv.go @@ -101,27 +101,27 @@ func (Mv) Diff(ctx context.Context, id string, olds MvState, news cmd.CommandArg defaultKind := news.UpdateKind() - if news.Args.Backup != olds.Args.Backup { + if cmd.Changed(news.Args.Backup, olds.Args.Backup) { diff["args.backup"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.Destination != olds.Args.Destination { + if cmd.Changed(news.Args.Destination, olds.Args.Destination) { diff["args.destination"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.Directory != olds.Args.Directory { + if cmd.Changed(news.Args.Directory, olds.Args.Directory) { diff["args.directory"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.Force != olds.Args.Force { + if cmd.Changed(news.Args.Force, olds.Args.Force) { diff["args.force"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.NoClobber != olds.Args.NoClobber { + if cmd.Changed(news.Args.NoClobber, olds.Args.NoClobber) { diff["args.noClobber"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.NoTargetDirectory != olds.Args.NoTargetDirectory { + if cmd.Changed(news.Args.NoTargetDirectory, olds.Args.NoTargetDirectory) { diff["args.noTargetDirectory"] = provider.PropertyDiff{Kind: defaultKind} } @@ -129,19 +129,19 @@ func (Mv) Diff(ctx context.Context, id string, olds MvState, news cmd.CommandArg diff["args.source"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.StripTrailingSlashes != olds.Args.StripTrailingSlashes { + if cmd.Changed(news.Args.StripTrailingSlashes, olds.Args.StripTrailingSlashes) { diff["args.stripTrailingSlashes"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.Suffix != olds.Args.Suffix { + if cmd.Changed(news.Args.Suffix, olds.Args.Suffix) { diff["args.suffix"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.TargetDirectory != olds.Args.TargetDirectory { + if cmd.Changed(news.Args.TargetDirectory, olds.Args.TargetDirectory) { diff["args.targetDirectory"] = provider.PropertyDiff{Kind: defaultKind} } - if news.Args.Update != olds.Args.Update { + if cmd.Changed(news.Args.Update, olds.Args.Update) { diff["args.update"] = provider.PropertyDiff{Kind: defaultKind} } diff --git a/provider/pkg/provider/coreutils/rm.go b/provider/pkg/provider/coreutils/rm.go index ba12a8d1..7395ef3c 100644 --- a/provider/pkg/provider/coreutils/rm.go +++ b/provider/pkg/provider/coreutils/rm.go @@ -62,7 +62,7 @@ func (Rm) Diff(ctx context.Context, id string, olds RmState, news cmd.CommandArg return provider.DiffResponse{}, fmt.Errorf("rm: %w", err) } - if news.Args.Dir != olds.Args.Dir { + if cmd.Changed(news.Args.Dir, olds.Args.Dir) { diff["args.dir"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } @@ -70,15 +70,15 @@ func (Rm) Diff(ctx context.Context, id string, olds RmState, news cmd.CommandArg diff["args.files"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Force != olds.Args.Force { + if cmd.Changed(news.Args.Force, olds.Args.Force) { diff["args.force"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.OneFileSystem != olds.Args.OneFileSystem { + if cmd.Changed(news.Args.OneFileSystem, olds.Args.OneFileSystem) { diff["args.oneFileSystem"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Recursive != olds.Args.Recursive { + if cmd.Changed(news.Args.Recursive, olds.Args.Recursive) { diff["args.recursive"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } diff --git a/provider/pkg/provider/coreutils/tee.go b/provider/pkg/provider/coreutils/tee.go index d128148b..517ea5f4 100644 --- a/provider/pkg/provider/coreutils/tee.go +++ b/provider/pkg/provider/coreutils/tee.go @@ -64,11 +64,11 @@ func (Tee) Diff(ctx context.Context, id string, olds TeeState, news cmd.CommandA return provider.DiffResponse{}, fmt.Errorf("tee: %w", err) } - if news.Args.Append != olds.Args.Append { + if cmd.Changed(news.Args.Append, olds.Args.Append) { diff["args.append"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } - if news.Args.Stdin != olds.Args.Stdin { + if cmd.Changed(news.Args.Stdin, olds.Args.Stdin) { diff["args.stdin"] = provider.PropertyDiff{Kind: provider.UpdateReplace} } diff --git a/tests/lifecycle/mktemp_test.go b/tests/lifecycle/mktemp_test.go index 5845fc28..aca80537 100644 --- a/tests/lifecycle/mktemp_test.go +++ b/tests/lifecycle/mktemp_test.go @@ -74,9 +74,48 @@ var _ = Describe("Mktemp", func() { }, }, }) + }) + + It("should not execute when unchanged", func(ctx context.Context) { + var firstDir string - _, err := provisioner.Exec(ctx, "touch", "blah") - Expect(err).NotTo(HaveOccurred()) + run(server, integration.LifeCycleTest{ + Resource: resource, + Create: integration.Operation{ + Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{ + "args": map[string]interface{}{ + "tmpdir": true, + }, + }), + Hook: func(inputs, output pr.PropertyMap) { + Expect(output["stderr"]).To(HavePropertyValue("")) + Expect(output["stdout"].V).NotTo(BeEmpty()) + Expect(output["exitCode"].V).To(BeEquivalentTo(0)) + Expect(output["createdFiles"].V).To(BeEmpty()) + Expect(output["movedFiles"].V).To(BeEmpty()) + firstDir = output["stdout"].V.(string) + }, + }, + Updates: []integration.Operation{ + { + Inputs: pr.NewPropertyMapFromMap(map[string]interface{}{ + "args": map[string]interface{}{ + "tmpdir": true, + }, + }), + Hook: func(inputs, output pr.PropertyMap) { + Expect(output["stderr"]).To(HavePropertyValue("")) + Expect(firstDir).NotTo(BeEmpty()) + Expect(output["stdout"]).To(HavePropertyValue(firstDir)) + Expect(output["exitCode"].V).To(BeEquivalentTo(0)) + Expect(output["triggers"]).To(Equal(pr.NewArrayProperty([]pr.PropertyValue{ + pr.NewProperty("a trigger"), + }))) + Expect(inputs["args"]).To(Equal(output["args"])) + }, + }, + }, + }) }) It("should fail when template is invalid", func() {