From 72a948e8a95d420079f6c86f1d063ce383637d6f Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Wed, 7 Aug 2024 17:01:19 +0200 Subject: [PATCH 01/10] flux diff artifact: Print the differences in human readable form. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I was hoping to use `flux diff artifact` as part of a CI pipeline to show the difference between the merge request and the currently deployed artifact. The existing implementation doesn't work for us, since it only compares the checksums. This commit changes the output produced by `flux diff artifact` to a list of changes in human readable form. The code is using the `dyff` package to produce a semantic diff of the YAML files. That means, for example, that changes in the order of map fields are ignored, while changes in the order of lists are not. Example output: ``` $ ./bin/flux diff artifact "oci://${IMAGE}" --path=example-service/ spec.replicas (apps/v1/Deployment/example-service-t205j6/backend-production) ± value change - 1 + 7 ✗ "oci://registry.gitlab.com/${REDACTED}/example-service-t205j6/deploy:production" and "example-service/" differ ``` The new `--brief` / `-q` flag enables users to revert to the previous behavior of only printing a has changed/has not changed line. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 121 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 117 insertions(+), 4 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index edfd9382..9158be04 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -17,12 +17,17 @@ limitations under the License. package main import ( + "bytes" "context" + "errors" "fmt" + "io" "os" oci "github.com/fluxcd/pkg/oci/client" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/gonvenience/ytbx" + "github.com/homeport/dyff/pkg/dyff" "github.com/spf13/cobra" "github.com/fluxcd/flux2/v2/internal/flags" @@ -42,6 +47,7 @@ type diffArtifactFlags struct { creds string provider flags.SourceOCIProvider ignorePaths []string + brief bool } var diffArtifactArgs = newDiffArtifactArgs() @@ -57,6 +63,7 @@ func init() { diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.creds, "creds", "", "credentials for OCI registry in the format [:] if --provider is generic") diffArtifactCmd.Flags().Var(&diffArtifactArgs.provider, "provider", sourceOCIRepositoryArgs.provider.Description()) diffArtifactCmd.Flags().StringSliceVar(&diffArtifactArgs.ignorePaths, "ignore-paths", excludeOCI, "set paths to ignore in .gitignore format") + diffArtifactCmd.Flags().BoolVarP(&diffArtifactArgs.brief, "brief", "q", false, "Just print a line when the resources differ. Does not output a list of changes.") diffCmd.AddCommand(diffArtifactCmd) } @@ -67,7 +74,7 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { ociURL := args[0] if diffArtifactArgs.path == "" { - return fmt.Errorf("invalid path %q", diffArtifactArgs.path) + return errors.New("the '--path' flag is required") } url, err := oci.ParseArtifactURL(ociURL) @@ -103,10 +110,116 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { } } - if err := ociClient.Diff(ctx, url, diffArtifactArgs.path, diffArtifactArgs.ignorePaths); err != nil { + diff, err := diffArtifact(ctx, ociClient, url, diffArtifactArgs.path) + if err != nil { return err } - logger.Successf("no changes detected") - return nil + if diff == "" { + logger.Successf("no changes detected") + return nil + } + + if !diffArtifactArgs.brief { + fmt.Print(diff) + } + + return fmt.Errorf("%q and %q differ", ociURL, diffArtifactArgs.path) +} + +func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath string) (string, error) { + localFile, err := loadLocal(localPath) + if err != nil { + return "", err + } + + remoteFile, cleanup, err := loadRemote(ctx, client, remoteURL) + if err != nil { + return "", err + } + defer cleanup() + + report, err := dyff.CompareInputFiles(remoteFile, localFile, + dyff.KubernetesEntityDetection(true), + ) + if err != nil { + return "", fmt.Errorf("dyff.CompareInputFiles(): %w", err) + } + + if len(report.Diffs) == 0 { + return "", nil + } + + var buf bytes.Buffer + + hr := &dyff.HumanReport{ + Report: report, + OmitHeader: true, + MultilineContextLines: 3, + } + if err := hr.WriteReport(&buf); err != nil { + return "", fmt.Errorf("WriteReport(): %w", err) + } + + return buf.String(), nil +} + +func loadLocal(path string) (ytbx.InputFile, error) { + if ytbx.IsStdin(path) { + buf, err := io.ReadAll(os.Stdin) + if err != nil { + return ytbx.InputFile{}, fmt.Errorf("os.ReadAll(os.Stdin): %w", err) + } + + nodes, err := ytbx.LoadDocuments(buf) + if err != nil { + return ytbx.InputFile{}, fmt.Errorf("ytbx.LoadDocuments(): %w", err) + } + + return ytbx.InputFile{ + Location: "STDIN", + Documents: nodes, + }, nil + } + + sb, err := os.Stat(path) + if err != nil { + return ytbx.InputFile{}, fmt.Errorf("os.Stat(%q): %w", path, err) + } + + if sb.IsDir() { + return ytbx.LoadDirectory(path) + } + + return ytbx.LoadFile(path) +} + +func loadRemote(ctx context.Context, client *oci.Client, url string) (ytbx.InputFile, func(), error) { + noopCleanup := func() {} + + tmpDir, err := os.MkdirTemp("", "flux-diff-artifact") + if err != nil { + return ytbx.InputFile{}, noopCleanup, fmt.Errorf("could not create temporary directory: %w", err) + } + + cleanup := func() { + if err := os.RemoveAll(tmpDir); err != nil { + fmt.Fprintf(os.Stderr, "os.RemoveAll(%q): %v\n", tmpDir, err) + } + } + + if _, err := client.Pull(ctx, url, tmpDir); err != nil { + cleanup() + return ytbx.InputFile{}, noopCleanup, fmt.Errorf("Pull(%q): %w", url, err) + } + + inputFile, err := ytbx.LoadDirectory(tmpDir) + if err != nil { + cleanup() + return ytbx.InputFile{}, noopCleanup, fmt.Errorf("ytbx.LoadDirectory(%q): %w", tmpDir, err) + } + + inputFile.Location = url + + return inputFile, cleanup, nil } From 2c4194e0a52c5533cc7b0c8334da665ea6fe30cd Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 8 Aug 2024 09:26:46 +0200 Subject: [PATCH 02/10] flux diff artifact: Check for an expected error using `errors.Is`. This fixes the `TestDiffArtifact` unit test. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 4 +++- cmd/flux/diff_artifact_test.go | 2 +- cmd/flux/main_test.go | 14 ++++++++++++-- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index 9158be04..88116be7 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -33,6 +33,8 @@ import ( "github.com/fluxcd/flux2/v2/internal/flags" ) +var ErrDiffArtifactChanged = errors.New("the remote and local artifact contents differ") + var diffArtifactCmd = &cobra.Command{ Use: "artifact", Short: "Diff Artifact", @@ -124,7 +126,7 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { fmt.Print(diff) } - return fmt.Errorf("%q and %q differ", ociURL, diffArtifactArgs.path) + return fmt.Errorf("%q and %q: %w", ociURL, diffArtifactArgs.path, ErrDiffArtifactChanged) } func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath string) (string, error) { diff --git a/cmd/flux/diff_artifact_test.go b/cmd/flux/diff_artifact_test.go index 05e29915..08e3c15e 100644 --- a/cmd/flux/diff_artifact_test.go +++ b/cmd/flux/diff_artifact_test.go @@ -81,7 +81,7 @@ func TestDiffArtifact(t *testing.T) { argsTpl: "diff artifact %s --path=%s", pushFile: "./testdata/diff-artifact/deployment.yaml", diffFile: "./testdata/diff-artifact/deployment-diff.yaml", - assert: assertError("the remote artifact contents differs from the local one"), + assert: assertErrorIs(ErrDiffArtifactChanged), }, } diff --git a/cmd/flux/main_test.go b/cmd/flux/main_test.go index b13bb6ce..d25de059 100644 --- a/cmd/flux/main_test.go +++ b/cmd/flux/main_test.go @@ -20,6 +20,7 @@ import ( "bufio" "bytes" "context" + "errors" "flag" "fmt" "io" @@ -33,7 +34,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/mattn/go-shellwords" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" k8syaml "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/client-go/tools/clientcmd" @@ -115,7 +116,7 @@ func (m *testEnvKubeManager) CreateObjects(clientObjects []*unstructured.Unstruc obj.SetResourceVersion(createObj.GetResourceVersion()) err = m.client.Status().Update(context.Background(), obj) // Updating status of static objects results in not found error. - if err != nil && !errors.IsNotFound(err) { + if err != nil && !k8serrors.IsNotFound(err) { return err } } @@ -272,6 +273,15 @@ func assertError(expected string) assertFunc { } } +func assertErrorIs(want error) assertFunc { + return func(_ string, got error) error { + if errors.Is(got, want) { + return nil + } + return fmt.Errorf("Expected error '%v' but got '%v'", want, got) + } +} + // Expect the command to succeed with the expected test output. func assertGoldenValue(expected string) assertFunc { return assert( From d28d9ff58de4a12f89d33b97f2a453895e60b05f Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 8 Aug 2024 09:35:08 +0200 Subject: [PATCH 03/10] Use `printers.DyffPrinter` to format the output. Also updates the list of options passed to `dyff.CompareInputFiles` to be the same as in the internal `build` package. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index 88116be7..1aebd3de 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -31,6 +31,7 @@ import ( "github.com/spf13/cobra" "github.com/fluxcd/flux2/v2/internal/flags" + "github.com/fluxcd/flux2/v2/pkg/printers" ) var ErrDiffArtifactChanged = errors.New("the remote and local artifact contents differ") @@ -142,6 +143,7 @@ func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath defer cleanup() report, err := dyff.CompareInputFiles(remoteFile, localFile, + dyff.IgnoreOrderChanges(false), dyff.KubernetesEntityDetection(true), ) if err != nil { @@ -154,13 +156,8 @@ func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath var buf bytes.Buffer - hr := &dyff.HumanReport{ - Report: report, - OmitHeader: true, - MultilineContextLines: 3, - } - if err := hr.WriteReport(&buf); err != nil { - return "", fmt.Errorf("WriteReport(): %w", err) + if err := printers.NewDyffPrinter().Print(&buf, report); err != nil { + return "", fmt.Errorf("formatting dyff report: %w", err) } return buf.String(), nil From 0a56dabf2d9aa3847bd9875231225ffe1b8c85cc Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 8 Aug 2024 09:45:48 +0200 Subject: [PATCH 04/10] flux diff artifact: Fix and document `--path=-` which reads from STDIN. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index 1aebd3de..ef53a6b9 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -39,7 +39,7 @@ var ErrDiffArtifactChanged = errors.New("the remote and local artifact contents var diffArtifactCmd = &cobra.Command{ Use: "artifact", Short: "Diff Artifact", - Long: withPreviewNote(`The diff artifact command computes the diff between the remote OCI artifact and a local directory or file`), + Long: withPreviewNote(`The diff artifact command prints the diff between the remote OCI artifact and a local directory or file`), Example: `# Check if local files differ from remote flux diff artifact oci://ghcr.io/stefanprodan/manifests:podinfo:6.2.0 --path=./kustomize`, RunE: diffArtifactCmdRun, @@ -62,7 +62,7 @@ func newDiffArtifactArgs() diffArtifactFlags { } func init() { - diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.path, "path", "", "path to the directory where the Kubernetes manifests are located") + diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.path, "path", "", "path to the directory or file containing the Kubernetes manifests, or '-' to read from STDIN") diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.creds, "creds", "", "credentials for OCI registry in the format [:] if --provider is generic") diffArtifactCmd.Flags().Var(&diffArtifactArgs.provider, "provider", sourceOCIRepositoryArgs.provider.Description()) diffArtifactCmd.Flags().StringSliceVar(&diffArtifactArgs.ignorePaths, "ignore-paths", excludeOCI, "set paths to ignore in .gitignore format") @@ -85,10 +85,6 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { return err } - if _, err := os.Stat(diffArtifactArgs.path); err != nil { - return fmt.Errorf("invalid path '%s', must point to an existing directory or file", diffArtifactArgs.path) - } - ctx, cancel := context.WithTimeout(context.Background(), rootArgs.timeout) defer cancel() @@ -183,7 +179,7 @@ func loadLocal(path string) (ytbx.InputFile, error) { sb, err := os.Stat(path) if err != nil { - return ytbx.InputFile{}, fmt.Errorf("os.Stat(%q): %w", path, err) + return ytbx.InputFile{}, err } if sb.IsDir() { From 506a44d740ce51341e232b70a0b1b693d8cee9f5 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 8 Aug 2024 09:48:06 +0200 Subject: [PATCH 05/10] flux diff artifact: Use `cmd.Print()` instead of `fmt.Print()`. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index ef53a6b9..0c5d4f99 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -120,7 +120,7 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { } if !diffArtifactArgs.brief { - fmt.Print(diff) + cmd.Print(diff) } return fmt.Errorf("%q and %q: %w", ociURL, diffArtifactArgs.path, ErrDiffArtifactChanged) From ba36b37ac13a8c2a6e83af660079b02bb1245351 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 8 Aug 2024 15:36:25 +0200 Subject: [PATCH 06/10] flux diff artifact: Provide a non-semantic diff option. Artifacts may contain other files types, not just YAML files, meaning the semantic YAML diff provided by `dyff` is not a safe default. This change implements purely textual diffing using the `diff` command line tool. This tool can be overridden by users using the `FLUX_EXTERNAL_DIFF` environment variable. Users that store Kubernetes resource manifests in the artifact can re-enable the semantic YAML diff behavior using the `--semantic-diff yaml` flag. The arguments to the diff subcommand may be: * A directory * A .tar.gz or .tgz file * An OCI url * An individual file The two arguments to the command are treated the same way, allowing users to diff in either direction. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 382 ++++++++++++++++++++++++++++++-------- go.mod | 4 +- 2 files changed, 306 insertions(+), 80 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index 0c5d4f99..25a8dfad 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -17,32 +17,43 @@ limitations under the License. package main import ( + "archive/tar" "bytes" + "compress/gzip" "context" "errors" "fmt" "io" "os" + "os/exec" + "path/filepath" + "sort" + "strings" oci "github.com/fluxcd/pkg/oci/client" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/gonvenience/ytbx" + "github.com/google/shlex" "github.com/homeport/dyff/pkg/dyff" "github.com/spf13/cobra" + "golang.org/x/exp/maps" "github.com/fluxcd/flux2/v2/internal/flags" "github.com/fluxcd/flux2/v2/pkg/printers" ) -var ErrDiffArtifactChanged = errors.New("the remote and local artifact contents differ") +var ErrDiffArtifactChanged = errors.New("the artifact contents differ") var diffArtifactCmd = &cobra.Command{ - Use: "artifact", + Use: "artifact ", Short: "Diff Artifact", - Long: withPreviewNote(`The diff artifact command prints the diff between the remote OCI artifact and a local directory or file`), + Long: withPreviewNote(fmt.Sprintf( + "The diff artifact command prints the diff between the remote OCI artifact and a local directory or file.\n\n"+ + "You can overwrite the command used for diffing by setting the %q environment variable.", externalDiffVar)), Example: `# Check if local files differ from remote -flux diff artifact oci://ghcr.io/stefanprodan/manifests:podinfo:6.2.0 --path=./kustomize`, +flux diff artifact oci://ghcr.io/stefanprodan/manifests:podinfo:6.2.0 ./kustomize`, RunE: diffArtifactCmdRun, + Args: cobra.RangeArgs(1, 2), } type diffArtifactFlags struct { @@ -51,38 +62,62 @@ type diffArtifactFlags struct { provider flags.SourceOCIProvider ignorePaths []string brief bool + differ *semanticDiffFlag } var diffArtifactArgs = newDiffArtifactArgs() func newDiffArtifactArgs() diffArtifactFlags { + defaultDiffer := mustExternalDiff() + return diffArtifactFlags{ provider: flags.SourceOCIProvider(sourcev1.GenericOCIProvider), + + differ: &semanticDiffFlag{ + options: map[string]differ{ + "yaml": dyffBuiltin{ + opts: []dyff.CompareOption{ + dyff.IgnoreOrderChanges(false), + dyff.KubernetesEntityDetection(true), + }, + }, + "false": defaultDiffer, + }, + value: "false", + differ: defaultDiffer, + }, } } func init() { - diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.path, "path", "", "path to the directory or file containing the Kubernetes manifests, or '-' to read from STDIN") + diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.path, "path", "", "path to the directory or file containing the Kubernetes manifests (deprecated, use a second positional argument instead)") diffArtifactCmd.Flags().StringVar(&diffArtifactArgs.creds, "creds", "", "credentials for OCI registry in the format [:] if --provider is generic") diffArtifactCmd.Flags().Var(&diffArtifactArgs.provider, "provider", sourceOCIRepositoryArgs.provider.Description()) diffArtifactCmd.Flags().StringSliceVar(&diffArtifactArgs.ignorePaths, "ignore-paths", excludeOCI, "set paths to ignore in .gitignore format") - diffArtifactCmd.Flags().BoolVarP(&diffArtifactArgs.brief, "brief", "q", false, "Just print a line when the resources differ. Does not output a list of changes.") + diffArtifactCmd.Flags().BoolVarP(&diffArtifactArgs.brief, "brief", "q", false, "just print a line when the resources differ; does not output a list of changes") + diffArtifactCmd.Flags().Var(diffArtifactArgs.differ, "semantic-diff", "use a semantic diffing algorithm") + diffCmd.AddCommand(diffArtifactCmd) } func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { + var from, to string + if len(args) < 1 { return fmt.Errorf("artifact URL is required") } - ociURL := args[0] + from = args[0] - if diffArtifactArgs.path == "" { - return errors.New("the '--path' flag is required") - } + switch { + case len(args) >= 2: + to = args[1] - url, err := oci.ParseArtifactURL(ociURL) - if err != nil { - return err + case diffArtifactArgs.path != "": + // for backwards compatibility + to = diffArtifactArgs.path + + default: + return errors.New("a second artifact is required") } ctx, cancel := context.WithTimeout(context.Background(), rootArgs.timeout) @@ -104,12 +139,20 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { return fmt.Errorf("provider not supported: %w", err) } - if err := ociClient.LoginWithProvider(ctx, url, ociProvider); err != nil { - return fmt.Errorf("error during login with provider: %w", err) + if url, err := oci.ParseArtifactURL(from); err == nil { + if err := ociClient.LoginWithProvider(ctx, url, ociProvider); err != nil { + return fmt.Errorf("error during login with provider: %w", err) + } + } + + if url, err := oci.ParseArtifactURL(to); err == nil { + if err := ociClient.LoginWithProvider(ctx, url, ociProvider); err != nil { + return fmt.Errorf("error during login with provider: %w", err) + } } } - diff, err := diffArtifact(ctx, ociClient, url, diffArtifactArgs.path) + diff, err := diffArtifact(ctx, ociClient, from, to, diffArtifactArgs) if err != nil { return err } @@ -123,25 +166,237 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { cmd.Print(diff) } - return fmt.Errorf("%q and %q: %w", ociURL, diffArtifactArgs.path, ErrDiffArtifactChanged) + return fmt.Errorf("%q and %q: %w", from, to, ErrDiffArtifactChanged) } -func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath string) (string, error) { - localFile, err := loadLocal(localPath) +func diffArtifact(ctx context.Context, client *oci.Client, from, to string, flags diffArtifactFlags) (string, error) { + fromDir, fromCleanup, err := loadArtifact(ctx, client, from) if err != nil { return "", err } + defer fromCleanup() - remoteFile, cleanup, err := loadRemote(ctx, client, remoteURL) + toDir, toCleanup, err := loadArtifact(ctx, client, to) if err != nil { return "", err } - defer cleanup() + defer toCleanup() - report, err := dyff.CompareInputFiles(remoteFile, localFile, - dyff.IgnoreOrderChanges(false), - dyff.KubernetesEntityDetection(true), - ) + return flags.differ.Diff(ctx, fromDir, toDir) +} + +// loadArtifact ensures that the artifact is in a local directory that can be +// recursively diffed. If necessary, files are downloaded, extracted, and/or +// copied into temporary directories for this purpose. +func loadArtifact(ctx context.Context, client *oci.Client, path string) (dir string, cleanup func(), err error) { + fi, err := os.Stat(path) + if err == nil && fi.IsDir() { + return path, func() {}, nil + } + + if err == nil && fi.Mode().IsRegular() { + return loadArtifactFile(path) + } + + url, err := oci.ParseArtifactURL(path) + if err == nil { + return loadArtifactOCI(ctx, client, url) + } + + return "", nil, fmt.Errorf("%q: %w", path, os.ErrNotExist) +} + +// loadArtifactOCI pulls the remove artifact into a temporary directory. +func loadArtifactOCI(ctx context.Context, client *oci.Client, url string) (dir string, cleanup func(), err error) { + tmpDir, err := os.MkdirTemp("", "flux-diff-artifact") + if err != nil { + return "", nil, fmt.Errorf("could not create temporary directory: %w", err) + } + + cleanup = func() { + if err := os.RemoveAll(tmpDir); err != nil { + fmt.Fprintf(os.Stderr, "os.RemoveAll(%q): %v\n", tmpDir, err) + } + } + + if _, err := client.Pull(ctx, url, tmpDir); err != nil { + cleanup() + return "", nil, fmt.Errorf("Pull(%q): %w", url, err) + } + + return tmpDir, cleanup, nil +} + +// loadArtifactFile copies a file into a temporary directory to allow for recursive diffing. +// If path is a .tar.gz or .tgz file, the archive is extracted into a temporary directory. +// Otherwise the file is copied verbatim. +func loadArtifactFile(path string) (dir string, cleanup func(), err error) { + tmpDir, err := os.MkdirTemp("", "flux-diff-artifact") + if err != nil { + return "", nil, fmt.Errorf("could not create temporary directory: %w", err) + } + + cleanup = func() { + if err := os.RemoveAll(tmpDir); err != nil { + fmt.Fprintf(os.Stderr, "os.RemoveAll(%q): %v\n", tmpDir, err) + } + } + + if strings.HasSuffix(path, ".tar.gz") || strings.HasSuffix(path, ".tgz") { + if err := extractTo(path, tmpDir); err != nil { + cleanup() + return "", nil, err + } + } else { + fh, err := os.Open(path) + if err != nil { + cleanup() + return "", nil, fmt.Errorf("os.Open(%q): %w", path, err) + } + defer fh.Close() + + name := filepath.Join(tmpDir, filepath.Base(path)) + if err := copyFile(fh, name); err != nil { + cleanup() + return "", nil, fmt.Errorf("os.Open(%q): %w", path, err) + } + } + + return tmpDir, cleanup, nil +} + +// extractTo extracts the .tar.gz / .tgz archive at archivePath into the destDir directory. +func extractTo(archivePath, destDir string) error { + archiveFH, err := os.Open(archivePath) + if err != nil { + return err + } + + gzipReader, err := gzip.NewReader(archiveFH) + if err != nil { + return fmt.Errorf("gzip.NewREader(): %w", err) + } + + tarReader := tar.NewReader(gzipReader) + + for { + header, err := tarReader.Next() + if errors.Is(err, io.EOF) { + break + } + if err != nil { + return fmt.Errorf("tarReader.Next(): %w", err) + } + + switch header.Typeflag { + case tar.TypeDir: + dir := filepath.Join(destDir, header.Name) + if err := os.Mkdir(dir, 0755); err != nil { + return fmt.Errorf("os.Mkdir(%q): %w", dir, err) + } + + case tar.TypeReg: + name := filepath.Join(destDir, header.Name) + if err := copyFile(tarReader, name); err != nil { + return fmt.Errorf("extracting %q: %w", header.Name, err) + } + + default: + logger.Warningf("unsupported tar type: %v", header.Typeflag) + } + } + + return nil +} + +func copyFile(from io.Reader, to string) error { + fh, err := os.Create(to) + if err != nil { + return fmt.Errorf("os.Create(%q): %w", to, err) + } + defer fh.Close() + + if _, err := io.Copy(fh, from); err != nil { + return fmt.Errorf("io.Copy(%q): %w", to, err) + } + + return nil +} + +type differ interface { + // Diff compares the two local directories "to" and "from" and returns their differences, or an empty string if they are equal. + Diff(ctx context.Context, from, to string) (string, error) +} + +// externalDiffCommand implements the differ interface using an external diff command. +type externalDiffCommand struct { + name string + flags []string +} + +// externalDiffVar is the environment variable users can use to overwrite the external diff command. +const externalDiffVar = "FLUX_EXTERNAL_DIFF" + +// mustExternalDiff initializes an externalDiffCommand using the externalDiffVar environment variable. +func mustExternalDiff() externalDiffCommand { + cmdline := os.Getenv(externalDiffVar) + if cmdline == "" { + cmdline = "diff -ur" + } + + args, err := shlex.Split(cmdline) + if err != nil { + panic(fmt.Sprintf("shlex.Split(%q): %v", cmdline, err)) + } + + return externalDiffCommand{ + name: args[0], + flags: args[1:], + } +} + +func (c externalDiffCommand) Diff(ctx context.Context, fromDir, toDir string) (string, error) { + var args []string + + args = append(args, c.flags...) + args = append(args, fromDir, toDir) + + cmd := exec.CommandContext(ctx, c.name, args...) + + var stdout bytes.Buffer + + cmd.Stdout = &stdout + cmd.Stderr = os.Stderr + + err := cmd.Run() + + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { + // exit code 1 only means there was a difference => ignore + } else if err != nil { + return "", fmt.Errorf("executing %q: %w", c.name, err) + } + + return stdout.String(), nil +} + +// dyffBuiltin implements the differ interface using `dyff`, a semantic diff for YAML documents. +type dyffBuiltin struct { + opts []dyff.CompareOption +} + +func (d dyffBuiltin) Diff(ctx context.Context, fromDir, toDir string) (string, error) { + fromFile, err := ytbx.LoadDirectory(fromDir) + if err != nil { + return "", fmt.Errorf("ytbx.LoadDirectory(%q): %w", fromDir, err) + } + + toFile, err := ytbx.LoadDirectory(toDir) + if err != nil { + return "", fmt.Errorf("ytbx.LoadDirectory(%q): %w", toDir, err) + } + + report, err := dyff.CompareInputFiles(fromFile, toFile, d.opts...) if err != nil { return "", fmt.Errorf("dyff.CompareInputFiles(): %w", err) } @@ -159,62 +414,33 @@ func diffArtifact(ctx context.Context, client *oci.Client, remoteURL, localPath return buf.String(), nil } -func loadLocal(path string) (ytbx.InputFile, error) { - if ytbx.IsStdin(path) { - buf, err := io.ReadAll(os.Stdin) - if err != nil { - return ytbx.InputFile{}, fmt.Errorf("os.ReadAll(os.Stdin): %w", err) - } - - nodes, err := ytbx.LoadDocuments(buf) - if err != nil { - return ytbx.InputFile{}, fmt.Errorf("ytbx.LoadDocuments(): %w", err) - } - - return ytbx.InputFile{ - Location: "STDIN", - Documents: nodes, - }, nil - } - - sb, err := os.Stat(path) - if err != nil { - return ytbx.InputFile{}, err - } - - if sb.IsDir() { - return ytbx.LoadDirectory(path) - } - - return ytbx.LoadFile(path) +// semanticDiffFlag implements pflag.Value for choosing a semantic diffing algorithm. +type semanticDiffFlag struct { + options map[string]differ + value string + differ } -func loadRemote(ctx context.Context, client *oci.Client, url string) (ytbx.InputFile, func(), error) { - noopCleanup := func() {} - - tmpDir, err := os.MkdirTemp("", "flux-diff-artifact") - if err != nil { - return ytbx.InputFile{}, noopCleanup, fmt.Errorf("could not create temporary directory: %w", err) +func (f *semanticDiffFlag) Set(s string) error { + d, ok := f.options[s] + if !ok { + return fmt.Errorf("invalid value: %q", s) } - cleanup := func() { - if err := os.RemoveAll(tmpDir); err != nil { - fmt.Fprintf(os.Stderr, "os.RemoveAll(%q): %v\n", tmpDir, err) - } - } + f.value = s + f.differ = d - if _, err := client.Pull(ctx, url, tmpDir); err != nil { - cleanup() - return ytbx.InputFile{}, noopCleanup, fmt.Errorf("Pull(%q): %w", url, err) - } - - inputFile, err := ytbx.LoadDirectory(tmpDir) - if err != nil { - cleanup() - return ytbx.InputFile{}, noopCleanup, fmt.Errorf("ytbx.LoadDirectory(%q): %w", tmpDir, err) - } - - inputFile.Location = url - - return inputFile, cleanup, nil + return nil +} + +func (f *semanticDiffFlag) String() string { + return f.value +} + +func (f *semanticDiffFlag) Type() string { + keys := maps.Keys(f.options) + + sort.Strings(keys) + + return strings.Join(keys, "|") } diff --git a/go.mod b/go.mod index 86b00a7c..fe76555a 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,7 @@ require ( github.com/gonvenience/ytbx v1.4.4 github.com/google/go-cmp v0.6.0 github.com/google/go-containerregistry v0.20.2 + github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/hashicorp/go-cleanhttp v0.5.2 github.com/homeport/dyff v1.7.1 github.com/lucasb-eyer/go-colorful v1.2.0 @@ -50,6 +51,7 @@ require ( github.com/spf13/pflag v1.0.5 github.com/theckman/yacspin v0.13.12 golang.org/x/crypto v0.26.0 + golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/term v0.23.0 golang.org/x/text v0.17.0 k8s.io/api v0.31.0 @@ -144,7 +146,6 @@ require ( github.com/google/go-github/v64 v64.0.0 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/google/gofuzz v1.2.0 // indirect - github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect github.com/google/uuid v1.6.0 // indirect github.com/gorilla/handlers v1.5.2 // indirect github.com/gorilla/mux v1.8.1 // indirect @@ -232,7 +233,6 @@ require ( go.opentelemetry.io/otel/trace v1.28.0 // indirect go.opentelemetry.io/proto/otlp v1.3.1 // indirect go.starlark.net v0.0.0-20231121155337-90ade8b19d09 // indirect - golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect golang.org/x/net v0.28.0 // indirect golang.org/x/oauth2 v0.22.0 // indirect golang.org/x/sync v0.8.0 // indirect From a45a0ee7c51d6249a7ba30ae43cc247d77bbb731 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Tue, 3 Sep 2024 10:31:58 +0200 Subject: [PATCH 07/10] flux diff artifact: Use the `tar.Untar()` convenience function. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 37 +++---------------------------------- 1 file changed, 3 insertions(+), 34 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index 25a8dfad..d5f94aab 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -17,9 +17,7 @@ limitations under the License. package main import ( - "archive/tar" "bytes" - "compress/gzip" "context" "errors" "fmt" @@ -31,6 +29,7 @@ import ( "strings" oci "github.com/fluxcd/pkg/oci/client" + "github.com/fluxcd/pkg/tar" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/gonvenience/ytbx" "github.com/google/shlex" @@ -272,38 +271,8 @@ func extractTo(archivePath, destDir string) error { return err } - gzipReader, err := gzip.NewReader(archiveFH) - if err != nil { - return fmt.Errorf("gzip.NewREader(): %w", err) - } - - tarReader := tar.NewReader(gzipReader) - - for { - header, err := tarReader.Next() - if errors.Is(err, io.EOF) { - break - } - if err != nil { - return fmt.Errorf("tarReader.Next(): %w", err) - } - - switch header.Typeflag { - case tar.TypeDir: - dir := filepath.Join(destDir, header.Name) - if err := os.Mkdir(dir, 0755); err != nil { - return fmt.Errorf("os.Mkdir(%q): %w", dir, err) - } - - case tar.TypeReg: - name := filepath.Join(destDir, header.Name) - if err := copyFile(tarReader, name); err != nil { - return fmt.Errorf("extracting %q: %w", header.Name, err) - } - - default: - logger.Warningf("unsupported tar type: %v", header.Typeflag) - } + if err := tar.Untar(archiveFH, destDir); err != nil { + return fmt.Errorf("Untar(%q, %q): %w", archivePath, destDir, err) } return nil From 72d32a248b06d1f1b67f31fa327c5656921c4faa Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Wed, 11 Sep 2024 15:56:20 +0200 Subject: [PATCH 08/10] flux diff artifact: Compute a unified diff internally by default. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 169 ++++++++++++++++++++++++++++++-------- go.mod | 2 + go.sum | 4 + 3 files changed, 139 insertions(+), 36 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index d5f94aab..b19849aa 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -22,17 +22,22 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" "sort" "strings" + "bitbucket.org/creachadair/stringset" oci "github.com/fluxcd/pkg/oci/client" "github.com/fluxcd/pkg/tar" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/gonvenience/ytbx" "github.com/google/shlex" + "github.com/hexops/gotextdiff" + "github.com/hexops/gotextdiff/myers" + "github.com/hexops/gotextdiff/span" "github.com/homeport/dyff/pkg/dyff" "github.com/spf13/cobra" "golang.org/x/exp/maps" @@ -61,29 +66,33 @@ type diffArtifactFlags struct { provider flags.SourceOCIProvider ignorePaths []string brief bool - differ *semanticDiffFlag + differ *differFlag } var diffArtifactArgs = newDiffArtifactArgs() func newDiffArtifactArgs() diffArtifactFlags { - defaultDiffer := mustExternalDiff() - return diffArtifactFlags{ provider: flags.SourceOCIProvider(sourcev1.GenericOCIProvider), - differ: &semanticDiffFlag{ + differ: &differFlag{ options: map[string]differ{ - "yaml": dyffBuiltin{ + "dyff": dyffBuiltin{ opts: []dyff.CompareOption{ dyff.IgnoreOrderChanges(false), dyff.KubernetesEntityDetection(true), }, }, - "false": defaultDiffer, + "external": externalDiff{}, + "unified": unifiedDiff{}, }, - value: "false", - differ: defaultDiffer, + description: map[string]string{ + "dyff": `semantic diff for YAML inputs`, + "external": `execute the command in the "` + externalDiffVar + `" environment variable`, + "unified": "generic unified diff for arbitrary text inputs", + }, + value: "unified", + differ: unifiedDiff{}, }, } } @@ -94,7 +103,7 @@ func init() { diffArtifactCmd.Flags().Var(&diffArtifactArgs.provider, "provider", sourceOCIRepositoryArgs.provider.Description()) diffArtifactCmd.Flags().StringSliceVar(&diffArtifactArgs.ignorePaths, "ignore-paths", excludeOCI, "set paths to ignore in .gitignore format") diffArtifactCmd.Flags().BoolVarP(&diffArtifactArgs.brief, "brief", "q", false, "just print a line when the resources differ; does not output a list of changes") - diffArtifactCmd.Flags().Var(diffArtifactArgs.differ, "semantic-diff", "use a semantic diffing algorithm") + diffArtifactCmd.Flags().Var(diffArtifactArgs.differ, "differ", diffArtifactArgs.differ.usage()) diffCmd.AddCommand(diffArtifactCmd) } @@ -297,53 +306,125 @@ type differ interface { Diff(ctx context.Context, from, to string) (string, error) } -// externalDiffCommand implements the differ interface using an external diff command. -type externalDiffCommand struct { - name string - flags []string +type unifiedDiff struct{} + +func (d unifiedDiff) Diff(_ context.Context, fromDir, toDir string) (string, error) { + fromFiles, err := filesInDir(fromDir) + if err != nil { + return "", err + } + + toFiles, err := filesInDir(toDir) + if err != nil { + return "", err + } + + allFiles := fromFiles.Union(toFiles) + + var sb strings.Builder + + for _, relPath := range allFiles.Elements() { + diff, err := d.diffFiles(fromDir, toDir, relPath) + if err != nil { + return "", err + } + + fmt.Fprint(&sb, diff) + } + + return sb.String(), nil } +func (d unifiedDiff) diffFiles(fromDir, toDir, relPath string) (string, error) { + fromPath := filepath.Join(fromDir, relPath) + fromData, err := d.readFile(fromPath) + if err != nil { + return "", fmt.Errorf("readFile(%q): %w", fromPath, err) + } + + toPath := filepath.Join(toDir, relPath) + toData, err := d.readFile(toPath) + if err != nil { + return "", fmt.Errorf("readFile(%q): %w", toPath, err) + } + + edits := myers.ComputeEdits(span.URIFromPath(fromPath), string(fromData), string(toData)) + return fmt.Sprint(gotextdiff.ToUnified(fromPath, toPath, string(fromData), edits)), nil +} + +func (d unifiedDiff) readFile(path string) ([]byte, error) { + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("os.Open(%q): %w", path, err) + } + defer file.Close() + + return io.ReadAll(file) +} + +func filesInDir(root string) (stringset.Set, error) { + var files stringset.Set + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + + if !d.Type().IsRegular() { + return nil + } + + relPath, err := filepath.Rel(root, path) + if err != nil { + return fmt.Errorf("filepath.Rel(%q, %q): %w", root, path, err) + } + + files.Add(relPath) + return nil + }) + if err != nil { + return nil, err + } + + return files, err +} + +// externalDiff implements the differ interface using an external diff command. +type externalDiff struct{} + // externalDiffVar is the environment variable users can use to overwrite the external diff command. const externalDiffVar = "FLUX_EXTERNAL_DIFF" -// mustExternalDiff initializes an externalDiffCommand using the externalDiffVar environment variable. -func mustExternalDiff() externalDiffCommand { +func (externalDiff) Diff(ctx context.Context, fromDir, toDir string) (string, error) { cmdline := os.Getenv(externalDiffVar) if cmdline == "" { - cmdline = "diff -ur" + return "", fmt.Errorf("the required %q environment variable is unset", externalDiffVar) } args, err := shlex.Split(cmdline) if err != nil { - panic(fmt.Sprintf("shlex.Split(%q): %v", cmdline, err)) + return "", fmt.Errorf("shlex.Split(%q): %w", cmdline, err) } - return externalDiffCommand{ - name: args[0], - flags: args[1:], - } -} + var executable string + executable, args = args[0], args[1:] -func (c externalDiffCommand) Diff(ctx context.Context, fromDir, toDir string) (string, error) { - var args []string - - args = append(args, c.flags...) args = append(args, fromDir, toDir) - cmd := exec.CommandContext(ctx, c.name, args...) + cmd := exec.CommandContext(ctx, executable, args...) var stdout bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = os.Stderr - err := cmd.Run() + err = cmd.Run() var exitErr *exec.ExitError if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 { // exit code 1 only means there was a difference => ignore } else if err != nil { - return "", fmt.Errorf("executing %q: %w", c.name, err) + return "", fmt.Errorf("executing %q: %w", executable, err) } return stdout.String(), nil @@ -383,14 +464,15 @@ func (d dyffBuiltin) Diff(ctx context.Context, fromDir, toDir string) (string, e return buf.String(), nil } -// semanticDiffFlag implements pflag.Value for choosing a semantic diffing algorithm. -type semanticDiffFlag struct { - options map[string]differ - value string +// differFlag implements pflag.Value for choosing a diffing implementation. +type differFlag struct { + options map[string]differ + description map[string]string + value string differ } -func (f *semanticDiffFlag) Set(s string) error { +func (f *differFlag) Set(s string) error { d, ok := f.options[s] if !ok { return fmt.Errorf("invalid value: %q", s) @@ -402,14 +484,29 @@ func (f *semanticDiffFlag) Set(s string) error { return nil } -func (f *semanticDiffFlag) String() string { +func (f *differFlag) String() string { return f.value } -func (f *semanticDiffFlag) Type() string { +func (f *differFlag) Type() string { keys := maps.Keys(f.options) sort.Strings(keys) return strings.Join(keys, "|") } + +func (f *differFlag) usage() string { + var b strings.Builder + fmt.Fprint(&b, "how the diff is generated:") + + keys := maps.Keys(f.options) + + sort.Strings(keys) + + for _, key := range keys { + fmt.Fprintf(&b, "\n %q: %s", key, f.description[key]) + } + + return b.String() +} diff --git a/go.mod b/go.mod index fe76555a..222ecf9c 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ go 1.22.4 replace gopkg.in/yaml.v3 => gopkg.in/yaml.v3 v3.0.1 require ( + bitbucket.org/creachadair/stringset v0.0.14 github.com/Masterminds/semver/v3 v3.2.1 github.com/ProtonMail/go-crypto v1.0.0 github.com/cyphar/filepath-securejoin v0.3.1 @@ -158,6 +159,7 @@ require ( github.com/hashicorp/go-version v1.7.0 // indirect github.com/hashicorp/golang-lru/arc/v2 v2.0.5 // indirect github.com/hashicorp/golang-lru/v2 v2.0.5 // indirect + github.com/hexops/gotextdiff v1.0.3 github.com/imdario/mergo v0.3.16 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect diff --git a/go.sum b/go.sum index 53c00fd1..c8216353 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +bitbucket.org/creachadair/stringset v0.0.14 h1:t1ejQyf8utS4GZV/4fM+1gvYucggZkfhb+tMobDxYOE= +bitbucket.org/creachadair/stringset v0.0.14/go.mod h1:Ej8fsr6rQvmeMDf6CCWMWGb14H9mz8kmDgPPTdiVT0w= code.gitea.io/sdk/gitea v0.19.0 h1:8I6s1s4RHgzxiPHhOQdgim1RWIRcr0LVMbHBjBFXq4Y= code.gitea.io/sdk/gitea v0.19.0/go.mod h1:IG9xZJoltDNeDSW0qiF2Vqx5orMWa7OhVWrjvrd5NpI= dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= @@ -323,6 +325,8 @@ github.com/hashicorp/golang-lru/arc/v2 v2.0.5 h1:l2zaLDubNhW4XO3LnliVj0GXO3+/CGN github.com/hashicorp/golang-lru/arc/v2 v2.0.5/go.mod h1:ny6zBSQZi2JxIeYcv7kt2sH2PXJtirBN7RDhRpxPkxU= github.com/hashicorp/golang-lru/v2 v2.0.5 h1:wW7h1TG88eUIJ2i69gaE3uNVtEPIagzhGvHgwfx2Vm4= github.com/hashicorp/golang-lru/v2 v2.0.5/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= +github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM= +github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg= github.com/homeport/dyff v1.7.1 h1:B3KJUtnU53H2UryxGcfYKQPrde8VjjbwlHZbczH3giQ= github.com/homeport/dyff v1.7.1/go.mod h1:iLe5b3ymc9xmHZNuJlNVKERE8L2isQMBLxFiTXcwZY0= github.com/imdario/mergo v0.3.16 h1:wwQJbIsHYGMUyLSPrEq1CT16AhnhNJQ51+4fdHUnCl4= From 672b759f2e0e2a2752f4f8c8e3dfca1060f29465 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 19 Sep 2024 14:26:51 +0200 Subject: [PATCH 09/10] flux diff artifact: Add support for the `--ignore-paths` flag. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact.go | 73 +++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 18 deletions(-) diff --git a/cmd/flux/diff_artifact.go b/cmd/flux/diff_artifact.go index b19849aa..af7603c4 100644 --- a/cmd/flux/diff_artifact.go +++ b/cmd/flux/diff_artifact.go @@ -33,6 +33,7 @@ import ( oci "github.com/fluxcd/pkg/oci/client" "github.com/fluxcd/pkg/tar" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" + "github.com/go-git/go-git/v5/plumbing/format/gitignore" "github.com/gonvenience/ytbx" "github.com/google/shlex" "github.com/hexops/gotextdiff" @@ -177,6 +178,16 @@ func diffArtifactCmdRun(cmd *cobra.Command, args []string) error { return fmt.Errorf("%q and %q: %w", from, to, ErrDiffArtifactChanged) } +func newMatcher(ignorePaths []string) gitignore.Matcher { + var patterns []gitignore.Pattern + + for _, path := range ignorePaths { + patterns = append(patterns, gitignore.ParsePattern(path, nil)) + } + + return gitignore.NewMatcher(patterns) +} + func diffArtifact(ctx context.Context, client *oci.Client, from, to string, flags diffArtifactFlags) (string, error) { fromDir, fromCleanup, err := loadArtifact(ctx, client, from) if err != nil { @@ -190,7 +201,7 @@ func diffArtifact(ctx context.Context, client *oci.Client, from, to string, flag } defer toCleanup() - return flags.differ.Diff(ctx, fromDir, toDir) + return flags.differ.Diff(ctx, fromDir, toDir, flags.ignorePaths) } // loadArtifact ensures that the artifact is in a local directory that can be @@ -279,6 +290,7 @@ func extractTo(archivePath, destDir string) error { if err != nil { return err } + defer archiveFH.Close() if err := tar.Untar(archiveFH, destDir); err != nil { return fmt.Errorf("Untar(%q, %q): %w", archivePath, destDir, err) @@ -303,21 +315,25 @@ func copyFile(from io.Reader, to string) error { type differ interface { // Diff compares the two local directories "to" and "from" and returns their differences, or an empty string if they are equal. - Diff(ctx context.Context, from, to string) (string, error) + Diff(ctx context.Context, from, to string, ignorePaths []string) (string, error) } type unifiedDiff struct{} -func (d unifiedDiff) Diff(_ context.Context, fromDir, toDir string) (string, error) { - fromFiles, err := filesInDir(fromDir) - if err != nil { - return "", err - } +func (d unifiedDiff) Diff(_ context.Context, fromDir, toDir string, ignorePaths []string) (string, error) { + matcher := newMatcher(ignorePaths) - toFiles, err := filesInDir(toDir) + fromFiles, err := filesInDir(fromDir, matcher) if err != nil { return "", err } + fmt.Fprintf(os.Stderr, "fromFiles = %v\n", fromFiles) + + toFiles, err := filesInDir(toDir, matcher) + if err != nil { + return "", err + } + fmt.Fprintf(os.Stderr, "toFiles = %v\n", toFiles) allFiles := fromFiles.Union(toFiles) @@ -338,13 +354,19 @@ func (d unifiedDiff) Diff(_ context.Context, fromDir, toDir string) (string, err func (d unifiedDiff) diffFiles(fromDir, toDir, relPath string) (string, error) { fromPath := filepath.Join(fromDir, relPath) fromData, err := d.readFile(fromPath) - if err != nil { + switch { + case errors.Is(err, fs.ErrNotExist): + return fmt.Sprintf("Only in %s: %s\n", toDir, relPath), nil + case err != nil: return "", fmt.Errorf("readFile(%q): %w", fromPath, err) } toPath := filepath.Join(toDir, relPath) toData, err := d.readFile(toPath) - if err != nil { + switch { + case errors.Is(err, fs.ErrNotExist): + return fmt.Sprintf("Only in %s: %s\n", fromDir, relPath), nil + case err != nil: return "", fmt.Errorf("readFile(%q): %w", toPath, err) } @@ -355,14 +377,18 @@ func (d unifiedDiff) diffFiles(fromDir, toDir, relPath string) (string, error) { func (d unifiedDiff) readFile(path string) ([]byte, error) { file, err := os.Open(path) if err != nil { - return nil, fmt.Errorf("os.Open(%q): %w", path, err) + return nil, err } defer file.Close() return io.ReadAll(file) } -func filesInDir(root string) (stringset.Set, error) { +func splitPath(path string) []string { + return strings.Split(path, string([]rune{filepath.Separator})) +} + +func filesInDir(root string, matcher gitignore.Matcher) (stringset.Set, error) { var files stringset.Set err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { @@ -370,15 +396,22 @@ func filesInDir(root string) (stringset.Set, error) { return err } - if !d.Type().IsRegular() { - return nil - } - relPath, err := filepath.Rel(root, path) if err != nil { return fmt.Errorf("filepath.Rel(%q, %q): %w", root, path, err) } + if matcher.Match(splitPath(relPath), d.IsDir()) { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + + if !d.Type().IsRegular() { + return nil + } + files.Add(relPath) return nil }) @@ -395,7 +428,7 @@ type externalDiff struct{} // externalDiffVar is the environment variable users can use to overwrite the external diff command. const externalDiffVar = "FLUX_EXTERNAL_DIFF" -func (externalDiff) Diff(ctx context.Context, fromDir, toDir string) (string, error) { +func (externalDiff) Diff(ctx context.Context, fromDir, toDir string, ignorePaths []string) (string, error) { cmdline := os.Getenv(externalDiffVar) if cmdline == "" { return "", fmt.Errorf("the required %q environment variable is unset", externalDiffVar) @@ -409,6 +442,10 @@ func (externalDiff) Diff(ctx context.Context, fromDir, toDir string) (string, er var executable string executable, args = args[0], args[1:] + for _, path := range ignorePaths { + args = append(args, "--exclude", path) + } + args = append(args, fromDir, toDir) cmd := exec.CommandContext(ctx, executable, args...) @@ -435,7 +472,7 @@ type dyffBuiltin struct { opts []dyff.CompareOption } -func (d dyffBuiltin) Diff(ctx context.Context, fromDir, toDir string) (string, error) { +func (d dyffBuiltin) Diff(ctx context.Context, fromDir, toDir string, _ []string) (string, error) { fromFile, err := ytbx.LoadDirectory(fromDir) if err != nil { return "", fmt.Errorf("ytbx.LoadDirectory(%q): %w", fromDir, err) From a3fc5a92e44f4ae42e014d8add3b9118ccbc86f0 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 19 Sep 2024 14:28:25 +0200 Subject: [PATCH 10/10] flux diff artifact: Add (and fix) unit tests. Signed-off-by: Florian Forster --- cmd/flux/diff_artifact_test.go | 71 ++++++++++++++++++- cmd/flux/main_test.go | 12 ++++ .../diff-artifact/deployment-diff.yaml | 2 +- .../testdata/diff-artifact/deployment.yaml | 2 +- 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/cmd/flux/diff_artifact_test.go b/cmd/flux/diff_artifact_test.go index 08e3c15e..fca6443c 100644 --- a/cmd/flux/diff_artifact_test.go +++ b/cmd/flux/diff_artifact_test.go @@ -22,6 +22,9 @@ package main import ( "context" "fmt" + "io" + "os" + "path/filepath" "testing" "time" @@ -65,6 +68,7 @@ func TestDiffArtifact(t *testing.T) { argsTpl string pushFile string diffFile string + diffName string assert assertFunc }{ { @@ -75,14 +79,50 @@ func TestDiffArtifact(t *testing.T) { diffFile: "./testdata/diff-artifact/deployment.yaml", assert: assertGoldenFile("testdata/diff-artifact/success.golden"), }, + { + name: "create unified diff output by default", + url: "oci://%s/podinfo:2.0.0", + argsTpl: "diff artifact %s --path=%s", + pushFile: "./testdata/diff-artifact/deployment.yaml", + diffFile: "./testdata/diff-artifact/deployment-diff.yaml", + diffName: "deployment.yaml", + assert: assert( + assertErrorIs(ErrDiffArtifactChanged), + assertRegexp(`(?m)^- cpu: 1000m$`), + assertRegexp(`(?m)^\+ cpu: 2000m$`), + ), + }, { name: "should fail if there is a diff", url: "oci://%s/podinfo:2.0.0", argsTpl: "diff artifact %s --path=%s", pushFile: "./testdata/diff-artifact/deployment.yaml", diffFile: "./testdata/diff-artifact/deployment-diff.yaml", - assert: assertErrorIs(ErrDiffArtifactChanged), + diffName: "only-local.yaml", + assert: assert( + assertErrorIs(ErrDiffArtifactChanged), + assertRegexp(`(?m)^Only in [^:]+: deployment.yaml$`), + assertRegexp(`(?m)^Only in [^:]+: only-local.yaml$`), + ), }, + { + name: "semantic diff using dyff", + url: "oci://%s/podinfo:2.0.0", + argsTpl: "diff artifact %s --path=%s --differ=dyff", + pushFile: "./testdata/diff-artifact/deployment.yaml", + diffFile: "./testdata/diff-artifact/deployment-diff.yaml", + diffName: "deployment.yaml", + assert: assert( + assertErrorIs(ErrDiffArtifactChanged), + assertRegexp(`(?m)^spec.template.spec.containers.podinfod.resources.limits.cpu$`), + assertRegexp(`(?m)^ ± value change$`), + assertRegexp(`(?m)^ - 1000m$`), + assertRegexp(`(?m)^ \+ 2000m$`), + ), + }, + // Attention: tests do not spawn a new process when executing commands. + // That means that the --differ flag remains set to "dyff" for + // subsequent tests. } ctx := ctrl.SetupSignalHandler() @@ -99,11 +139,38 @@ func TestDiffArtifact(t *testing.T) { t.Fatalf(fmt.Errorf("failed to push image: %w", err).Error()) } + diffFile := tt.diffFile + if tt.diffName != "" { + diffFile = makeTempFile(t, tt.diffFile, tt.diffName) + } + cmd := cmdTestCase{ - args: fmt.Sprintf(tt.argsTpl, tt.url, tt.diffFile), + args: fmt.Sprintf(tt.argsTpl, tt.url, diffFile), assert: tt.assert, } cmd.runTestCmd(t) }) } } + +func makeTempFile(t *testing.T, source, basename string) string { + path := filepath.Join(t.TempDir(), basename) + out, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + defer out.Close() + + in, err := os.Open(source) + if err != nil { + t.Fatal(err) + } + defer in.Close() + + _, err = io.Copy(out, in) + if err != nil { + t.Fatal(err) + } + + return path +} diff --git a/cmd/flux/main_test.go b/cmd/flux/main_test.go index d25de059..87ad0a40 100644 --- a/cmd/flux/main_test.go +++ b/cmd/flux/main_test.go @@ -26,6 +26,7 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "sync/atomic" "testing" @@ -338,6 +339,17 @@ func assertGoldenTemplateFile(goldenFile string, templateValues map[string]strin }) } +func assertRegexp(expected string) assertFunc { + re := regexp.MustCompile(expected) + + return func(output string, _ error) error { + if !re.MatchString(output) { + return fmt.Errorf("Output does not match regular expression:\nOutput:\n%s\n\nRegular expression:\n%s", output, expected) + } + return nil + } +} + type TestClusterMode int const ( diff --git a/cmd/flux/testdata/diff-artifact/deployment-diff.yaml b/cmd/flux/testdata/diff-artifact/deployment-diff.yaml index 350d4c1b..3910da6a 100644 --- a/cmd/flux/testdata/diff-artifact/deployment-diff.yaml +++ b/cmd/flux/testdata/diff-artifact/deployment-diff.yaml @@ -4,7 +4,7 @@ metadata: labels: kustomize.toolkit.fluxcd.io/name: podinfo kustomize.toolkit.fluxcd.io/namespace: {{ .fluxns }} - name: podinfo-diff + name: podinfo namespace: default spec: minReadySeconds: 3 diff --git a/cmd/flux/testdata/diff-artifact/deployment.yaml b/cmd/flux/testdata/diff-artifact/deployment.yaml index 3910da6a..dc7584c5 100644 --- a/cmd/flux/testdata/diff-artifact/deployment.yaml +++ b/cmd/flux/testdata/diff-artifact/deployment.yaml @@ -71,7 +71,7 @@ spec: timeoutSeconds: 5 resources: limits: - cpu: 2000m + cpu: 1000m memory: 512Mi requests: cpu: 100m