From c547e15e7e5e8990b958ac257d8f47075cef3246 Mon Sep 17 00:00:00 2001 From: Michael Sauter Date: Mon, 17 Jul 2023 16:38:52 +0200 Subject: [PATCH] Consult artifact repository in artifact check Artifacts may be downloaded in the start task from repository A, but should be uploaded to repository B in the finish task. In this case, Contains erroneously returned true before this change. Contains should only return true if the artifacts were downloaded from the same repository earlier. When the repositories differ, the task must fetch a manifest from the target repository before checking the manifest. --- CHANGELOG.md | 1 + cmd/finish/main.go | 16 ++++-- cmd/finish/main_test.go | 28 +++++------ cmd/start/main.go | 2 +- pkg/pipelinectxt/artifacts.go | 60 +++++++++++++++-------- pkg/pipelinectxt/artifacts_test.go | 78 ++++++++++++++++++++++++++++++ test/tasks/ods-finish_test.go | 15 +++--- 7 files changed, 152 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 017491cd..2a8ad51f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ listed in the changelog. ### Fixed +- Artifacts may not be uploaded to target repository when the target repository differs from the source repository ([#715](https://github.com/opendevstack/ods-pipeline/pull/715)) - Gradle build script changes twice into working dir ([#705](https://github.com/opendevstack/ods-pipeline/issues/705)) ## [0.13.1] - 2023-06-05 diff --git a/cmd/finish/main.go b/cmd/finish/main.go index cdca93fb..0037b74a 100644 --- a/cmd/finish/main.go +++ b/cmd/finish/main.go @@ -213,19 +213,27 @@ func uploadArtifacts( if err != nil { return err } + if am.Repository != nexusRepository { + logger.Infof("Artifacts were downloaded from %q but should be uploaded to %q. Checking for existing artifacts in %q before proceeding ...", am.Repository, nexusRepository, nexusRepository) + group := pipelinectxt.ArtifactGroupBase(ctxt) + am, err = pipelinectxt.DownloadGroup(nexusClient, nexusRepository, group, "", logger) + if err != nil { + return err + } + } for artifactsSubDir, files := range artifactsMap { for _, filename := range files { - if am.Contains(artifactsSubDir, filename) { - logger.Infof("Artifact %s is already present in Nexus repository %s.", filename, nexusRepository) + if am.Contains(nexusRepository, artifactsSubDir, filename) { + logger.Infof("Artifact %q is already present in Nexus repository %q.", filename, nexusRepository) } else { nexusGroup := artifactGroup(ctxt, artifactsSubDir, opts) localFile := filepath.Join(checkoutDir, pipelinectxt.ArtifactsPath, artifactsSubDir, filename) - logger.Infof("Uploading %s to Nexus repository %s, group %s ...", localFile, nexusRepository, nexusGroup) + logger.Infof("Uploading %q to Nexus repository %q, group %q ...", localFile, nexusRepository, nexusGroup) link, err := nexusClient.Upload(nexusRepository, nexusGroup, localFile) if err != nil { return err } - logger.Infof("Successfully uploaded %s to %s", localFile, link) + logger.Infof("Successfully uploaded %q to %s", localFile, link) } } } diff --git a/cmd/finish/main_test.go b/cmd/finish/main_test.go index 9bb92d3b..94a42e00 100644 --- a/cmd/finish/main_test.go +++ b/cmd/finish/main_test.go @@ -28,33 +28,35 @@ func TestUploadArtifacts(t *testing.T) { Repository: "my-repo", GitCommitSHA: "8d351a10fb428c0c1239530256e21cf24f136e73", } - t.Log("Write dummy artifact") + t.Logf("Write dummy image-digest artifact to %q\n", pipelinectxt.ArtifactsPath) artifactsDir := filepath.Join(tempWorkingDir, pipelinectxt.ArtifactsPath) err = writeArtifactFile(artifactsDir, pipelinectxt.ImageDigestsDir, "foo.json") if err != nil { t.Fatal(err) } - err = uploadArtifacts(logger, nexusClient, nexusRepo, tempWorkingDir, ctxt, options{aggregateTasksStatus: "Succeeded"}) - if err != nil { + t.Logf("Upload dummy image-digest artifact to %q on PR failure\n", nexusRepo) + opts := options{pipelineRunName: "pipelineRun", aggregateTasksStatus: "Failed"} + if err := uploadArtifacts(logger, nexusClient, nexusRepo, tempWorkingDir, ctxt, opts); err != nil { t.Fatal(err) } if len(nexusClient.Artifacts[nexusRepo]) != 1 { t.Fatalf("want 1 uploaded file, got: %v", nexusClient.Artifacts[nexusRepo]) } - wantFile := "/my-project/my-repo/8d351a10fb428c0c1239530256e21cf24f136e73/image-digests/foo.json" + wantFile := "/my-project/my-repo/8d351a10fb428c0c1239530256e21cf24f136e73/failed-pipelineRun-artifacts/image-digests/foo.json" if !nexusRepoContains(nexusClient.Artifacts[nexusRepo], wantFile) { t.Fatalf("want: %s, got: %s", wantFile, nexusClient.Artifacts[nexusRepo][0]) } - err = uploadArtifacts(logger, nexusClient, nexusRepo, tempWorkingDir, ctxt, options{pipelineRunName: "pipelineRun", aggregateTasksStatus: "Failed"}) - if err != nil { + t.Logf("Upload dummy image-digest artifact to %q on PR success\n", nexusRepo) + opts = options{pipelineRunName: "pipelineRun", aggregateTasksStatus: "Succeeded"} + if err := uploadArtifacts(logger, nexusClient, nexusRepo, tempWorkingDir, ctxt, opts); err != nil { t.Fatal(err) } if len(nexusClient.Artifacts[nexusRepo]) != 2 { - t.Fatalf("expected one additional file upload, got: %v", nexusClient.Artifacts[nexusRepo]) + t.Fatalf("expected two artifacts in repo, got: %v", nexusClient.Artifacts[nexusRepo]) } - wantFile = "/my-project/my-repo/8d351a10fb428c0c1239530256e21cf24f136e73/failed-pipelineRun-artifacts/image-digests/foo.json" + wantFile = "/my-project/my-repo/8d351a10fb428c0c1239530256e21cf24f136e73/image-digests/foo.json" if !nexusRepoContains(nexusClient.Artifacts[nexusRepo], wantFile) { t.Fatalf("want: %s, got: %s", wantFile, nexusClient.Artifacts[nexusRepo][1]) } @@ -109,7 +111,7 @@ func TestHandleArtifacts(t *testing.T) { } t.Log("Write empty artifacts manifest for subrepository") subrepoArtifactsDir := filepath.Join(subrepoDir, pipelinectxt.ArtifactsPath) - err = writeArtifactsManifest(subrepoArtifactsDir) + err = writeArtifactsManifest("nexus-repo", subrepoArtifactsDir) if err != nil { t.Fatal(err) } @@ -156,7 +158,7 @@ func prepareTempWorkingDir(nexusRepo string) (string, func(), error) { } cleanup = func() { os.RemoveAll(tempWorkingDir) } artifactsDir := filepath.Join(tempWorkingDir, pipelinectxt.ArtifactsPath) - err = writeArtifactsManifest(artifactsDir) + err = writeArtifactsManifest("nexus-repo", artifactsDir) if err != nil { return tempWorkingDir, cleanup, err } @@ -190,9 +192,7 @@ func writeArtifactFile(artifactsDir, subdir, filename string) error { } // writeArtifactsManifest writes an artigact manifest JSON file into artifactsDir. -func writeArtifactsManifest(artifactsDir string) error { - am := &pipelinectxt.ArtifactsManifest{ - Artifacts: []pipelinectxt.ArtifactInfo{}, - } +func writeArtifactsManifest(repository, artifactsDir string) error { + am := pipelinectxt.NewArtifactsManifest(repository) return pipelinectxt.WriteJsonArtifact(am, artifactsDir, pipelinectxt.ArtifactsManifestFilename) } diff --git a/cmd/start/main.go b/cmd/start/main.go index 26ef4463..ff113bd2 100644 --- a/cmd/start/main.go +++ b/cmd/start/main.go @@ -232,7 +232,7 @@ func main() { } func writeEmptyArtifactManifests(subrepoContexts []*pipelinectxt.ODSContext) error { - emptyManifest := &pipelinectxt.ArtifactsManifest{Artifacts: []pipelinectxt.ArtifactInfo{}} + emptyManifest := pipelinectxt.NewArtifactsManifest("") err := pipelinectxt.WriteJsonArtifact(emptyManifest, pipelinectxt.ArtifactsPath, pipelinectxt.ArtifactsManifestFilename) if err != nil { return fmt.Errorf("write repo empty manifest: %w", err) diff --git a/pkg/pipelinectxt/artifacts.go b/pkg/pipelinectxt/artifacts.go index 7b18d6a2..cf3ce2cc 100644 --- a/pkg/pipelinectxt/artifacts.go +++ b/pkg/pipelinectxt/artifacts.go @@ -44,6 +44,9 @@ const ( // ArtifactsManifest represents all downloaded artifacts. type ArtifactsManifest struct { + // Repository is the artifact repository from which the manifests were downloaded. + Repository string `json:"repository"` + // Artifacts lists all artifacts downloaded. Artifacts []ArtifactInfo `json:"artifacts"` } @@ -54,6 +57,11 @@ type ArtifactInfo struct { Name string `json:"name"` } +// NewArtifactsManifest returns a new ArtifactsManifest instance. +func NewArtifactsManifest(repository string, artifacts ...ArtifactInfo) *ArtifactsManifest { + return &ArtifactsManifest{Repository: repository, Artifacts: artifacts} +} + // ReadArtifactsManifestFromFile reads an artifact manifest from given filename or errors. func ReadArtifactsManifestFromFile(filename string) (*ArtifactsManifest, error) { body, err := os.ReadFile(filename) @@ -72,7 +80,10 @@ func ReadArtifactsManifestFromFile(filename string) (*ArtifactsManifest, error) } // Contains checks whether given directory/name is already present in repository. -func (am *ArtifactsManifest) Contains(directory, name string) bool { +func (am *ArtifactsManifest) Contains(repository, directory, name string) bool { + if am.Repository != repository { + return false + } for _, a := range am.Artifacts { if a.Directory == directory && a.Name == name { return true @@ -203,46 +214,53 @@ func ArtifactGroup(ctxt *ODSContext, subdir string) string { // DownloadGroup searches given repositories in order for assets in given group. // As soon as one repository has any asset in the group, the search is stopped -// and all fond artifacts are downloaded into artifactsDir. +// and all found artifacts are downloaded into artifactsDir. // An artifacts manifest is returned describing the downloaded files. // When none of the given repositories contains any artifacts under the group, // no artifacts are downloaded and no error is returned. -func DownloadGroup(nexusClient nexus.ClientInterface, repository string, group, artifactsDir string, logger logging.LeveledLoggerInterface) (*ArtifactsManifest, error) { +// If artifactsDir is an empty string, the searched files are not downloaded but +// the artifacts are still recorded in the returned manifest. +func DownloadGroup( + nexusClient nexus.ClientInterface, + repository, group, artifactsDir string, + logger logging.LeveledLoggerInterface) (*ArtifactsManifest, error) { // We want to target all artifacts underneath the group, hence the trailing '*'. nexusSearchGroup := fmt.Sprintf("%s/*", group) - am := &ArtifactsManifest{ - Artifacts: []ArtifactInfo{}, - } + am := NewArtifactsManifest(repository) urls, err := searchForAssets(nexusClient, nexusSearchGroup, repository, logger) if err != nil { return nil, err } + if artifactsDir == "" { + logger.Debugf("Artifacts will not be downloaded but only added to the manifest ...") + } + for _, s := range urls { u, err := url.Parse(s) if err != nil { return nil, err } - urlPathParts := strings.Split(u.Path, fmt.Sprintf("%s/", group)) - if len(urlPathParts) != 2 { - return nil, fmt.Errorf("unexpected URL path (must contain two parts after group '%s'): %s", group, u.Path) + _, fileWithSubPath, ok := strings.Cut(u.Path, fmt.Sprintf("%s/", group)) // e.g. "pipeline-runs/foo-zh9gt0.json" + if !ok { + return nil, fmt.Errorf("unexpected URL path (must contain group '%s'): %s", group, u.Path) } - fileWithSubPath := urlPathParts[1] // e.g. "pipeline-runs/foo-zh9gt0.json" if !strings.Contains(fileWithSubPath, "/") { return nil, fmt.Errorf("unexpected URL path (must contain a subfolder after the commit SHA): %s", fileWithSubPath) } aritfactName := path.Base(fileWithSubPath) // e.g. "pipeline-runs" artifactType := path.Dir(fileWithSubPath) // e.g. "foo-zh9gt0.json" - artifactsSubPath := filepath.Join(artifactsDir, artifactType) - if _, err := os.Stat(artifactsSubPath); os.IsNotExist(err) { - if err := os.MkdirAll(artifactsSubPath, 0755); err != nil { - return nil, fmt.Errorf("failed to create directory: %s, error: %w", artifactsSubPath, err) + if artifactsDir != "" { + artifactsSubPath := filepath.Join(artifactsDir, artifactType) + if _, err := os.Stat(artifactsSubPath); os.IsNotExist(err) { + if err := os.MkdirAll(artifactsSubPath, 0755); err != nil { + return nil, fmt.Errorf("failed to create directory: %s, error: %w", artifactsSubPath, err) + } + } + outfile := filepath.Join(artifactsDir, fileWithSubPath) + if _, err := nexusClient.Download(s, outfile); err != nil { + return nil, err } - } - outfile := filepath.Join(artifactsDir, fileWithSubPath) - _, err = nexusClient.Download(s, outfile) - if err != nil { - return nil, err } am.Artifacts = append(am.Artifacts, ArtifactInfo{ URL: s, @@ -261,9 +279,9 @@ func searchForAssets(nexusClient nexus.ClientInterface, searchGroup string, repo return nil, err } if len(urls) > 0 { - logger.Infof("Found artifacts in repository %s inside group %s ...", repository, searchGroup) + logger.Infof("Found artifacts in repository %q inside group %q ...", repository, searchGroup) return urls, nil } - logger.Infof("No artifacts found in repository %s inside group %s.", repository, searchGroup) + logger.Infof("No artifacts found in repository %q inside group %q.", repository, searchGroup) return []string{}, nil } diff --git a/pkg/pipelinectxt/artifacts_test.go b/pkg/pipelinectxt/artifacts_test.go index 5086e7ec..46d7c759 100644 --- a/pkg/pipelinectxt/artifacts_test.go +++ b/pkg/pipelinectxt/artifacts_test.go @@ -118,6 +118,84 @@ func TestDownloadGroup(t *testing.T) { } } +func TestContains(t *testing.T) { + tests := map[string]struct { + manifest *ArtifactsManifest + repo string + dir string + name string + want bool + }{ + "different repo": { + manifest: &ArtifactsManifest{ + Repository: "a", + Artifacts: []ArtifactInfo{ + { + Directory: "b", + Name: "c", + }, + }, + }, + repo: "x", + dir: "b", + name: "c", + want: false, + }, + "same repo, different dir": { + manifest: &ArtifactsManifest{ + Repository: "a", + Artifacts: []ArtifactInfo{ + { + Directory: "b", + Name: "c", + }, + }, + }, + repo: "a", + dir: "x", + name: "c", + want: false, + }, + "same repo, same dir, different name": { + manifest: &ArtifactsManifest{ + Repository: "a", + Artifacts: []ArtifactInfo{ + { + Directory: "b", + Name: "c", + }, + }, + }, + repo: "a", + dir: "b", + name: "x", + want: false, + }, + "match": { + manifest: &ArtifactsManifest{ + Repository: "a", + Artifacts: []ArtifactInfo{ + { + Directory: "b", + Name: "c", + }, + }, + }, + repo: "a", + dir: "b", + name: "c", + want: true, + }, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + if tc.manifest.Contains(tc.repo, tc.dir, tc.name) != tc.want { + t.Errorf("Want %q to contain=%v, but did not", name, tc.want) + } + }) + } +} + func findArtifact(url string, artifacts []ArtifactInfo) *ArtifactInfo { for _, a := range artifacts { if a.URL == url { diff --git a/test/tasks/ods-finish_test.go b/test/tasks/ods-finish_test.go index c177a6aa..1b4c5737 100644 --- a/test/tasks/ods-finish_test.go +++ b/test/tasks/ods-finish_test.go @@ -59,14 +59,13 @@ func TestTaskODSFinish(t *testing.T) { ); err != nil { t.Fatal(err) } - am := pipelinectxt.ArtifactsManifest{ - Artifacts: []pipelinectxt.ArtifactInfo{ - { - Directory: pipelinectxt.CodeCoveragesDir, - Name: "coverage.out", - }, + am := pipelinectxt.NewArtifactsManifest( + nexus.TestTemporaryRepository, + pipelinectxt.ArtifactInfo{ + Directory: pipelinectxt.CodeCoveragesDir, + Name: "coverage.out", }, - } + ) if err := pipelinectxt.WriteJsonArtifact( am, filepath.Join(wsDir, pipelinectxt.ArtifactsPath), @@ -87,7 +86,7 @@ func TestTaskODSFinish(t *testing.T) { checkBuildStatus(t, bitbucketClient, ctxt.ODS.GitCommitSHA, bitbucket.BuildStatusSuccessful) checkArtifactsAreInNexus(t, ctxt, nexus.TestTemporaryRepository) - wantLogMsg := "Artifact coverage.out is already present in Nexus repository" + wantLogMsg := "Artifact \"coverage.out\" is already present in Nexus repository" if !strings.Contains(string(ctxt.CollectedLogs), wantLogMsg) { t.Fatalf("Want:\n%s\n\nGot:\n%s", wantLogMsg, string(ctxt.CollectedLogs)) }