From 7030b559fb6199201cf65dd70a58b3762bcf2000 Mon Sep 17 00:00:00 2001 From: Brent Baude Date: Wed, 19 Feb 2025 12:35:10 -0600 Subject: [PATCH] Define artifact error types In a different PR review, it was noted that defined error types for artifacts was lacking. We have these for most other commands and they help with error differentiation. The changes here are to define the errors, implement them in the library, and adopt test verifications to match. Signed-off-by: Brent Baude --- pkg/libartifact/artifact.go | 6 +++--- pkg/libartifact/store/store.go | 4 ++-- pkg/libartifact/types/errors.go | 12 ++++++++++++ test/e2e/artifact_test.go | 15 ++++++++------- 4 files changed, 25 insertions(+), 12 deletions(-) create mode 100644 pkg/libartifact/types/errors.go diff --git a/pkg/libartifact/artifact.go b/pkg/libartifact/artifact.go index 7d8304e533..d611b4351b 100644 --- a/pkg/libartifact/artifact.go +++ b/pkg/libartifact/artifact.go @@ -2,11 +2,11 @@ package libartifact import ( "encoding/json" - "errors" "fmt" "strings" "github.com/containers/image/v5/manifest" + "github.com/containers/podman/v5/pkg/libartifact/types" "github.com/opencontainers/go-digest" ) @@ -33,7 +33,7 @@ func (a *Artifact) GetName() (string, error) { } // We don't have a concept of None for artifacts yet, but if we do, // then we should probably not error but return `None` - return "", errors.New("artifact is unnamed") + return "", types.ErrArtifactUnamed } // SetName is a accessor for setting the artifact name @@ -75,5 +75,5 @@ func (al ArtifactList) GetByNameOrDigest(nameOrDigest string) (*Artifact, bool, return artifact, true, nil } } - return nil, false, fmt.Errorf("no artifact found with name or digest of %s", nameOrDigest) + return nil, false, fmt.Errorf("%s: %w", nameOrDigest, types.ErrArtifactNotExist) } diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index 6fcca3bdf2..ca6fbc8a3b 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -196,7 +196,7 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, paths []string, op // error means it exists _, _, err := artifacts.GetByNameOrDigest(dest) if err == nil { - return nil, fmt.Errorf("artifact %s already exists", dest) + return nil, fmt.Errorf("%s: %w", dest, libartTypes.ErrArtifactAlreadyExists) } artifactManifest = specV1.Manifest{ Versioned: specs.Versioned{SchemaVersion: ManifestSchemaVersion}, @@ -227,7 +227,7 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, paths []string, op for _, path := range paths { fileName := filepath.Base(path) if _, ok := fileNames[fileName]; ok { - return nil, fmt.Errorf("file: %q already exists in artifact", fileName) + return nil, fmt.Errorf("%s: %w", fileName, libartTypes.ErrArtifactFileExists) } fileNames[fileName] = struct{}{} } diff --git a/pkg/libartifact/types/errors.go b/pkg/libartifact/types/errors.go new file mode 100644 index 0000000000..8a9eedaa1d --- /dev/null +++ b/pkg/libartifact/types/errors.go @@ -0,0 +1,12 @@ +package types + +import ( + "errors" +) + +var ( + ErrArtifactUnamed = errors.New("artifact is unnamed") + ErrArtifactNotExist = errors.New("artifact does not exist") + ErrArtifactAlreadyExists = errors.New("artifact already exists") + ErrArtifactFileExists = errors.New("file already exists in artifact") +) diff --git a/test/e2e/artifact_test.go b/test/e2e/artifact_test.go index dba86eb931..5c41e6b9b3 100644 --- a/test/e2e/artifact_test.go +++ b/test/e2e/artifact_test.go @@ -88,7 +88,7 @@ var _ = Describe("Podman artifact", func() { // Adding an artifact with an existing name should fail addAgain := podmanTest.Podman([]string{"artifact", "add", artifact1Name, artifact1File}) addAgain.WaitWithDefaultTimeout() - Expect(addAgain).Should(ExitWithError(125, fmt.Sprintf("Error: artifact %s already exists", artifact1Name))) + Expect(addAgain).Should(ExitWithError(125, fmt.Sprintf("Error: %s: artifact already exists", artifact1Name))) }) It("podman artifact add with options", func() { @@ -157,7 +157,7 @@ var _ = Describe("Podman artifact", func() { // Trying to remove an image that does not exist should fail rmFail := podmanTest.Podman([]string{"artifact", "rm", "foobar"}) rmFail.WaitWithDefaultTimeout() - Expect(rmFail).Should(ExitWithError(125, fmt.Sprintf("Error: no artifact found with name or digest of %s", "foobar"))) + Expect(rmFail).Should(ExitWithError(125, fmt.Sprintf("Error: %s: artifact does not exist", "foobar"))) // Add an artifact to remove later artifact1File, err := createArtifactFile(4192) @@ -173,7 +173,7 @@ var _ = Describe("Podman artifact", func() { // Inspecting that the removed artifact should fail inspectArtifact := podmanTest.Podman([]string{"artifact", "inspect", artifact1Name}) inspectArtifact.WaitWithDefaultTimeout() - Expect(inspectArtifact).Should(ExitWithError(125, fmt.Sprintf("Error: no artifact found with name or digest of %s", artifact1Name))) + Expect(inspectArtifact).Should(ExitWithError(125, fmt.Sprintf("Error: %s: artifact does not exist", artifact1Name))) }) It("podman artifact inspect with full or partial digest", func() { @@ -433,7 +433,8 @@ var _ = Describe("Podman artifact", func() { appendFail := podmanTest.Podman([]string{"artifact", "add", "--append", artifact1Name, artifact1File}) appendFail.WaitWithDefaultTimeout() - Expect(appendFail).Should(ExitWithError(125, fmt.Sprintf("Error: file: \"%s\" already exists in artifact", filepath.Base(artifact1File)))) + // Error: PJYjLgoU: file already exists in artifact + Expect(appendFail).Should(ExitWithError(125, fmt.Sprintf("Error: %s: file already exists in artifact", filepath.Base(artifact1File)))) a := podmanTest.InspectArtifact(artifact1Name) @@ -449,11 +450,11 @@ var _ = Describe("Podman artifact", func() { addFail := podmanTest.Podman([]string{"artifact", "add", artifact1Name, artifact1File, artifact1File}) addFail.WaitWithDefaultTimeout() - Expect(addFail).Should(ExitWithError(125, fmt.Sprintf("Error: file: \"%s\" already exists in artifact", filepath.Base(artifact1File)))) + Expect(addFail).Should(ExitWithError(125, fmt.Sprintf("Error: %s: file already exists in artifact", filepath.Base(artifact1File)))) inspectFail := podmanTest.Podman([]string{"artifact", "inspect", artifact1Name}) inspectFail.WaitWithDefaultTimeout() - Expect(inspectFail).Should(ExitWithError(125, fmt.Sprintf("Error: no artifact found with name or digest of %s", artifact1Name))) + Expect(inspectFail).Should(ExitWithError(125, fmt.Sprintf("Error: %s: artifact does not exist", artifact1Name))) }) It("podman artifact add --append file already exists in artifact", func() { @@ -465,7 +466,7 @@ var _ = Describe("Podman artifact", func() { appendFail := podmanTest.Podman([]string{"artifact", "add", "--append", artifact1Name, artifact1File}) appendFail.WaitWithDefaultTimeout() - Expect(appendFail).Should(ExitWithError(125, fmt.Sprintf("Error: file: \"%s\" already exists in artifact", filepath.Base(artifact1File)))) + Expect(appendFail).Should(ExitWithError(125, fmt.Sprintf("Error: %s: file already exists in artifact", filepath.Base(artifact1File)))) a := podmanTest.InspectArtifact(artifact1Name) Expect(a.Manifest.Layers).To(HaveLen(1))