From 9470234d758cc7853686ac00d64ae4c7c2aab157 Mon Sep 17 00:00:00 2001 From: Hongwei Liu Date: Mon, 4 Nov 2024 12:45:57 +0800 Subject: [PATCH 1/2] feat(STONEINTG-1071): skip checking snapshot not affected by pr group * don't check the snapshots which are not affected by pr group during group snapshot preparation Signed-off-by: Hongwei Liu --- gitops/snapshot.go | 18 +- gitops/snapshot_test.go | 13 +- go.mod | 2 +- .../controller/snapshot/snapshot_adapter.go | 192 +++++++++++------- .../snapshot/snapshot_adapter_test.go | 115 ++++++++--- loader/loader.go | 35 ++++ loader/loader_mock.go | 10 + loader/loader_mock_test.go | 15 ++ loader/loader_test.go | 8 + 9 files changed, 279 insertions(+), 129 deletions(-) diff --git a/gitops/snapshot.go b/gitops/snapshot.go index 97123878f..fcde83314 100644 --- a/gitops/snapshot.go +++ b/gitops/snapshot.go @@ -1033,20 +1033,12 @@ func HasPRGroupProcessed(snapshot *applicationapiv1alpha1.Snapshot) bool { return metadata.HasAnnotation(snapshot, PRGroupCreationAnnotation) } -// GetPRGroupHashFromSnapshot gets the value of label test.appstudio.openshift.io/pr-group-sha from component snapshot -func GetPRGroupHashFromSnapshot(snapshot *applicationapiv1alpha1.Snapshot) string { - if metadata.HasLabel(snapshot, PRGroupHashLabel) { - return snapshot.Labels[PRGroupHashLabel] +// GetPRGroupFromSnapshot gets the value of label test.appstudio.openshift.io/pr-group-sha and annotation from component snapshot +func GetPRGroupFromSnapshot(snapshot *applicationapiv1alpha1.Snapshot) (string, string) { + if metadata.HasLabel(snapshot, PRGroupHashLabel) && metadata.HasAnnotation(snapshot, PRGroupAnnotation) { + return snapshot.Labels[PRGroupHashLabel], snapshot.Annotations[PRGroupAnnotation] } - return "" -} - -// GetPRGroupFromSnapshot gets the value of annotation test.appstudio.openshift.io/pr-group from component snapshot -func GetPRGroupFromSnapshot(snapshot *applicationapiv1alpha1.Snapshot) string { - if metadata.HasAnnotation(snapshot, PRGroupAnnotation) { - return snapshot.Annotations[PRGroupAnnotation] - } - return "" + return "", "" } // FindMatchingSnapshotComponent find the snapshot component from the given snapshot according to the name of the given component name diff --git a/gitops/snapshot_test.go b/gitops/snapshot_test.go index 1232af0bf..d24f6a404 100644 --- a/gitops/snapshot_test.go +++ b/gitops/snapshot_test.go @@ -876,17 +876,18 @@ var _ = Describe("Gitops functions for managing Snapshots", Ordered, func() { Context("Group snapshot creation tests", func() { When("Snapshot has snapshot type label", func() { - prGroup := "feature1" - prGroupSha := "feature1hash" + expectedPRGroup := "feature1" + expectedPRGoupSha := "feature1hash" BeforeEach(func() { - hasComSnapshot1.Labels[gitops.PRGroupHashLabel] = prGroupSha - hasComSnapshot1.Annotations[gitops.PRGroupAnnotation] = prGroup + hasComSnapshot1.Labels[gitops.PRGroupHashLabel] = expectedPRGoupSha + hasComSnapshot1.Annotations[gitops.PRGroupAnnotation] = expectedPRGroup hasComSnapshot1.Annotations[gitops.PRGroupCreationAnnotation] = "group snapshot is created" }) It("make sure pr group annotation/label can be found in group", func() { - Expect(gitops.GetPRGroupFromSnapshot(hasComSnapshot1)).To(Equal(prGroup)) - Expect(gitops.GetPRGroupHashFromSnapshot(hasComSnapshot1)).To(Equal(prGroupSha)) + prGroupSha, prGroup := gitops.GetPRGroupFromSnapshot(hasComSnapshot1) + Expect(prGroup).To(Equal(expectedPRGroup)) + Expect(prGroupSha).To(Equal(expectedPRGoupSha)) Expect(gitops.HasPRGroupProcessed(hasComSnapshot1)).To(BeTrue()) }) diff --git a/go.mod b/go.mod index d044f0848..f6c2ba274 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/xanzy/go-gitlab v0.109.0 go.uber.org/mock v0.4.0 go.uber.org/zap v1.27.0 + golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 golang.org/x/oauth2 v0.21.0 k8s.io/api v0.29.4 k8s.io/apimachinery v0.29.4 @@ -90,7 +91,6 @@ require ( go.opencensus.io v0.24.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/crypto v0.24.0 // indirect - golang.org/x/exp v0.0.0-20240325151524-a685a6edb6d8 // indirect golang.org/x/net v0.26.0 // indirect golang.org/x/sync v0.7.0 // indirect golang.org/x/sys v0.21.0 // indirect diff --git a/internal/controller/snapshot/snapshot_adapter.go b/internal/controller/snapshot/snapshot_adapter.go index 43dde64ff..89cefacbc 100644 --- a/internal/controller/snapshot/snapshot_adapter.go +++ b/internal/controller/snapshot/snapshot_adapter.go @@ -26,6 +26,7 @@ import ( "k8s.io/client-go/util/retry" + "golang.org/x/exp/slices" clienterrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" ctrl "sigs.k8s.io/controller-runtime" @@ -538,69 +539,27 @@ func (a *Adapter) EnsureGroupSnapshotExist() (controller.OperationResult, error) return controller.ContinueProcessing() } - prGroupHash := gitops.GetPRGroupHashFromSnapshot(a.snapshot) - if prGroupHash == "" { - a.logger.Error(fmt.Errorf("NotFound"), fmt.Sprintf("Failed to get PR group hash from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name)) - err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("Failed to get PR group hash from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name), a.client) + prGroupHash, prGroup := gitops.GetPRGroupFromSnapshot(a.snapshot) + if prGroupHash == "" || prGroup == "" { + a.logger.Error(fmt.Errorf("NotFound"), fmt.Sprintf("Failed to get PR group label/annotation from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name)) + err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("Failed to get PR group label/annotation from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name), a.client) if err != nil { return controller.RequeueWithError(err) } return controller.ContinueProcessing() } - prGroup := gitops.GetPRGroupFromSnapshot(a.snapshot) - if prGroup == "" { - a.logger.Error(fmt.Errorf("NotFound"), fmt.Sprintf("Failed to get PR group from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name)) - err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("Failed to get PR group from snapshot %s/%s", a.snapshot.Namespace, a.snapshot.Name), a.client) - if err != nil { - return controller.RequeueWithError(err) - } - return controller.ContinueProcessing() - } - - pipelineRuns, err := a.loader.GetPipelineRunsWithPRGroupHash(a.context, a.client, a.snapshot, prGroupHash) + // check if all build plr have been processed for the given pr group + haveAllPipelineRunProcessedForPrGroup, err := a.haveAllPipelineRunProcessedForPrGroup(prGroup, prGroupHash) if err != nil { - a.logger.Error(err, fmt.Sprintf("Failed to get build pipelineRuns for given pr group hash %s", prGroupHash)) return controller.RequeueWithError(err) } - - for _, pipelineRun := range *pipelineRuns { - pipelineRun := pipelineRun - - // check if the build PLR is the latest existing one - if !isLatestBuildPipelineRunInComponent(&pipelineRun, pipelineRuns) { - a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is not the latest for its component, skipped", pipelineRun.Namespace, pipelineRun.Name, prGroup)) - continue - } - - // check if build PLR finishes - if !h.HasPipelineRunFinished(&pipelineRun) { - a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is still running, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup)) - err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is still running, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup), a.client) - if err != nil { - return controller.RequeueWithError(err) - } - return controller.ContinueProcessing() - } - - // check if build PLR succeeds - if !h.HasPipelineRunSucceeded(&pipelineRun) { - a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s failed, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup)) - err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("The build pipelineRun %s/%s with pr group %s failed, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup), a.client) - if err != nil { - return controller.RequeueWithError(err) - } - return controller.ContinueProcessing() - } - - // check if build PLR has component snapshot created except the build that snapshot is created from because the build plr has not been labeled with snapshot name - if !metadata.HasAnnotation(&pipelineRun, tekton.SnapshotNameLabel) && !metadata.HasLabelWithValue(a.snapshot, gitops.BuildPipelineRunNameLabel, pipelineRun.Name) { - a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s has succeeded but component snapshot has not been created now", pipelineRun.Namespace, pipelineRun.Name, prGroup)) - return controller.ContinueProcessing() - } + // don't need to create group snapshot if there is any unready pipelinerun for pr group + if !haveAllPipelineRunProcessedForPrGroup { + return controller.ContinueProcessing() } - groupSnapshot, componentSnapshotInfos, err := a.prepareGroupSnapshot(a.application, prGroupHash) + groupSnapshot, componentSnapshotInfos, err := a.prepareGroupSnapshot(a.application, prGroup, prGroupHash) if err != nil { a.logger.Error(err, "failed to prepare group snapshot") if h.IsUnrecoverableMetadataError(err) || clienterrors.IsNotFound(err) { @@ -895,7 +854,16 @@ func (a *Adapter) updateComponentLastPromotedImage(ctx context.Context, c client return nil } -func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Application, prGroupHash string) (*applicationapiv1alpha1.Snapshot, []gitops.ComponentSnapshotInfo, error) { +func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Application, prGroup, prGroupHash string) (*applicationapiv1alpha1.Snapshot, []gitops.ComponentSnapshotInfo, error) { + componentsToCheck, err := a.getComponentsForPRGroup(application, prGroup, prGroupHash) + if err != nil { + return nil, nil, err + } + if len(componentsToCheck) < 2 { + a.logger.Info(fmt.Sprintf("The number %d of components affected by this PR group %s is less than 2, skipping group snapshot creation", len(componentsToCheck), prGroup)) + return nil, nil, err + } + applicationComponents, err := a.loader.GetAllApplicationComponents(a.context, a.client, application) if err != nil { return nil, nil, err @@ -906,35 +874,38 @@ func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Appli for _, applicationComponent := range *applicationComponents { var isPRMROpened bool applicationComponent := applicationComponent // G601 - snapshots, err := a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client, a.snapshot, applicationComponent.Name, prGroupHash) - if err != nil { - a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", applicationComponent.Name) - return nil, nil, err - } - sortedSnapshots := gitops.SortSnapshots(*snapshots) - // find the latest component snapshot created for open PR/MR - for _, snapshot := range sortedSnapshots { - snapshot := snapshot - // find the built image for pull/merge request build PLR from the latest opened pull request component snapshot - isPRMROpened, err = a.status.IsPRMRInSnapshotOpened(a.context, &snapshot) + if slices.Contains(componentsToCheck, applicationComponent.Name) { + snapshots, err := a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client, a.snapshot, applicationComponent.Name, prGroupHash) if err != nil { - a.logger.Error(err, "Failed to fetch PR/MR status for component snapshot", "snapshot.Name", a.snapshot.Name) + a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", applicationComponent.Name) return nil, nil, err } - if isPRMROpened { - a.logger.Info("PR/MR in snapshot is opened, will find snapshotComponent and add to groupSnapshot") - snapshotComponent := gitops.FindMatchingSnapshotComponent(&snapshot, &applicationComponent) - componentSnapshotInfos = append(componentSnapshotInfos, gitops.ComponentSnapshotInfo{ - Component: applicationComponent.Name, - BuildPipelineRun: snapshot.Labels[gitops.BuildPipelineRunNameLabel], - Snapshot: snapshot.Name, - Namespace: a.snapshot.Namespace, - RepoUrl: snapshot.Annotations[gitops.PipelineAsCodeRepoUrlAnnotation], - PullRequestNumber: snapshot.Annotations[gitops.PipelineAsCodePullRequestAnnotation], - }) - snapshotComponents = append(snapshotComponents, snapshotComponent) - break + + sortedSnapshots := gitops.SortSnapshots(*snapshots) + // find the latest component snapshot created for open PR/MR + for _, snapshot := range sortedSnapshots { + snapshot := snapshot + // find the built image for pull/merge request build PLR from the latest opened pull request component snapshot + isPRMROpened, err = a.status.IsPRMRInSnapshotOpened(a.context, &snapshot) + if err != nil { + a.logger.Error(err, "Failed to fetch PR/MR status for component snapshot", "snapshot.Name", a.snapshot.Name) + return nil, nil, err + } + if isPRMROpened { + a.logger.Info("PR/MR in snapshot is opened, will find snapshotComponent and add to groupSnapshot") + snapshotComponent := gitops.FindMatchingSnapshotComponent(&snapshot, &applicationComponent) + componentSnapshotInfos = append(componentSnapshotInfos, gitops.ComponentSnapshotInfo{ + Component: applicationComponent.Name, + BuildPipelineRun: snapshot.Labels[gitops.BuildPipelineRunNameLabel], + Snapshot: snapshot.Name, + Namespace: a.snapshot.Namespace, + RepoUrl: snapshot.Annotations[gitops.PipelineAsCodeRepoUrlAnnotation], + PullRequestNumber: snapshot.Annotations[gitops.PipelineAsCodePullRequestAnnotation], + }) + snapshotComponents = append(snapshotComponents, snapshotComponent) + break + } } } // isPRMROpened represents snapshotComponent can be gottent from PR component snapshot @@ -962,6 +933,7 @@ func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Appli ContainerImage: containerImage, Source: *componentSource, } + a.logger.Info("component with containerImage from Global Candidate List will be added to group snapshot", "component.Name", snapshotComponent.Name) snapshotComponents = append(snapshotComponents, snapshotComponent) } } @@ -1010,3 +982,67 @@ func isLatestBuildPipelineRunInComponent(pipelineRun *tektonv1.PipelineRun, pipe } return true } + +// haveAllPipelineRunProcessedForPrGroup checks if all build plr has been processed for the given pr group +func (a *Adapter) haveAllPipelineRunProcessedForPrGroup(prGroup, prGroupHash string) (bool, error) { + pipelineRuns, err := a.loader.GetPipelineRunsWithPRGroupHash(a.context, a.client, a.snapshot, prGroupHash) + if err != nil { + a.logger.Error(err, fmt.Sprintf("Failed to get build pipelineRuns for given pr group hash %s", prGroupHash)) + return false, err + } + + for _, pipelineRun := range *pipelineRuns { + pipelineRun := pipelineRun //G601 + // check if the build PLR is the latest existing one + if !isLatestBuildPipelineRunInComponent(&pipelineRun, pipelineRuns) { + a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is not the latest for its component, skipped", pipelineRun.Namespace, pipelineRun.Name, prGroup)) + continue + } + + // check if build PLR finishes + if !h.HasPipelineRunFinished(&pipelineRun) { + a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is still running, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup)) + err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("The build pipelineRun %s/%s with pr group %s is still running, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup), a.client) + if err != nil { + return false, err + } + return false, nil + } + + // check if build PLR succeeds + if !h.HasPipelineRunSucceeded(&pipelineRun) { + a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s failed, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup)) + err := gitops.AnnotateSnapshot(a.context, a.snapshot, gitops.PRGroupCreationAnnotation, fmt.Sprintf("The build pipelineRun %s/%s with pr group %s failed, won't create group snapshot", pipelineRun.Namespace, pipelineRun.Name, prGroup), a.client) + if err != nil { + return false, err + } + return false, nil + } + + // check if build PLR has component snapshot created except the build that snapshot is created from because the build plr has not been labeled with snapshot name + if !metadata.HasAnnotation(&pipelineRun, tekton.SnapshotNameLabel) && !metadata.HasLabelWithValue(a.snapshot, gitops.BuildPipelineRunNameLabel, pipelineRun.Name) { + a.logger.Info(fmt.Sprintf("The build pipelineRun %s/%s with pr group %s has succeeded but component snapshot has not been created now", pipelineRun.Namespace, pipelineRun.Name, prGroup)) + return false, nil + } + } + return true, nil +} + +// getComponentsAffectedPrGroup returns the component names affected by the given pr group hash +func (a *Adapter) getComponentsForPRGroup(application *applicationapiv1alpha1.Application, prGroup, prGroupHash string) ([]string, error) { + snapshots, err := a.loader.GetMatchingComponentSnapshotsForPRGroupHash(a.context, a.client, a.snapshot, prGroupHash) + if err != nil { + a.logger.Error(err, fmt.Sprintf("Failed to fetch Snapshots for pr group %s", prGroupHash)) + return nil, err + } + + var componentNames []string + for _, snapshot := range *snapshots { + componentName := snapshot.Labels[gitops.SnapshotComponentLabel] + if slices.Contains(componentNames, componentName) { + continue + } + componentNames = append(componentNames, componentName) + } + return componentNames, nil +} diff --git a/internal/controller/snapshot/snapshot_adapter_test.go b/internal/controller/snapshot/snapshot_adapter_test.go index a83c0a5e5..f376dd12c 100644 --- a/internal/controller/snapshot/snapshot_adapter_test.go +++ b/internal/controller/snapshot/snapshot_adapter_test.go @@ -66,6 +66,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { hasApp *applicationapiv1alpha1.Application hasComp *applicationapiv1alpha1.Component hasCompMissingImageDigest *applicationapiv1alpha1.Component + hasCompWithValidImage *applicationapiv1alpha1.Component hasCom1 *applicationapiv1alpha1.Component hasCom3 *applicationapiv1alpha1.Component hasSnapshot *applicationapiv1alpha1.Snapshot @@ -183,6 +184,29 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { } Expect(k8sClient.Create(ctx, hasComp)).Should(Succeed()) + hasCompWithValidImage = &applicationapiv1alpha1.Component{ + ObjectMeta: metav1.ObjectMeta{ + Name: "component-sample-valid-image", + Namespace: "default", + }, + Spec: applicationapiv1alpha1.ComponentSpec{ + ComponentName: "component-sample", + Application: "application-sample", + ContainerImage: sample_image + "@" + sampleDigest, + Source: applicationapiv1alpha1.ComponentSource{ + ComponentSourceUnion: applicationapiv1alpha1.ComponentSourceUnion{ + GitSource: &applicationapiv1alpha1.GitSource{ + URL: SampleRepoLink, + }, + }, + }, + }, + Status: applicationapiv1alpha1.ComponentStatus{ + LastBuiltCommit: sourceRepoRef, + }, + } + Expect(k8sClient.Create(ctx, hasCompWithValidImage)).Should(Succeed()) + hasCom1 = &applicationapiv1alpha1.Component{ ObjectMeta: metav1.ObjectMeta{ Name: "component1-sample", @@ -596,6 +620,8 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(err == nil || errors.IsNotFound(err)).To(BeTrue()) err = k8sClient.Delete(ctx, hasCompMissingImageDigest) Expect(err == nil || errors.IsNotFound(err)).To(BeTrue()) + err = k8sClient.Delete(ctx, hasCompWithValidImage) + Expect(err == nil || errors.IsNotFound(err)).To(BeTrue()) err = k8sClient.Delete(ctx, integrationTestScenario) Expect(err == nil || errors.IsNotFound(err)).To(BeTrue()) err = k8sClient.Delete(ctx, testReleasePlan) @@ -1586,7 +1612,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) - It("ensures component snapshot will not be processed if it has no pr group sha label", func() { + It("ensures component snapshot will not be processed if it has no pr group label/annotation", func() { var buf bytes.Buffer log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} Expect(metadata.DeleteLabel(hasComSnapshot1, gitops.PRGroupHashLabel)).ShouldNot(HaveOccurred()) @@ -1606,32 +1632,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { result, err := adapter.EnsureGroupSnapshotExist() Expect(result.CancelRequest).To(BeFalse()) Expect(result.RequeueRequest).To(BeFalse()) - Expect(buf.String()).Should(ContainSubstring("Failed to get PR group hash from snapshot")) - Expect(err).ToNot(HaveOccurred()) - Expect(metadata.HasAnnotation(hasComSnapshot1, gitops.PRGroupCreationAnnotation)).To(BeTrue()) - }) - - It("ensures component snapshot will not be processed if it has no pr group annotation", func() { - var buf bytes.Buffer - log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} - Expect(metadata.DeleteAnnotation(hasComSnapshot1, gitops.PRGroupAnnotation)).ShouldNot(HaveOccurred()) - Expect(metadata.HasAnnotation(hasComSnapshot1, gitops.PRGroupAnnotation)).To(BeFalse()) - adapter = NewAdapter(ctx, hasComSnapshot1, hasApp, log, loader.NewMockLoader(), k8sClient) - adapter.context = toolkit.GetMockedContext(ctx, []toolkit.MockData{ - { - ContextKey: loader.ApplicationContextKey, - Resource: hasApp, - }, - { - ContextKey: loader.SnapshotContextKey, - Resource: hasComSnapshot1, - }, - }) - - result, err := adapter.EnsureGroupSnapshotExist() - Expect(result.CancelRequest).To(BeFalse()) - Expect(result.RequeueRequest).To(BeFalse()) - Expect(buf.String()).Should(ContainSubstring("Failed to get PR group from snapshot")) + Expect(buf.String()).Should(ContainSubstring("Failed to get PR group label/annotation from snapshot")) Expect(err).ToNot(HaveOccurred()) Expect(metadata.HasAnnotation(hasComSnapshot1, gitops.PRGroupCreationAnnotation)).To(BeTrue()) }) @@ -1781,12 +1782,34 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { ContextKey: loader.GetBuildPLRContextKey, Resource: []tektonv1.PipelineRun{}, }, + { + ContextKey: loader.GetPRSnapshotsKey, + Resource: []applicationapiv1alpha1.Snapshot{*hasComSnapshot1, *hasComSnapshot3}, + }, { ContextKey: loader.ApplicationComponentsContextKey, Resource: []applicationapiv1alpha1.Component{*hasCom1, *hasCom3}, }, }) + // create 3 snasphots here because we need to get snapshot twice so that we can't use the mocked snapshot + Expect(k8sClient.Create(adapter.context, hasComSnapshot2)).Should(Succeed()) + Eventually(func() error { + err := k8sClient.Get(adapter.context, types.NamespacedName{ + Name: hasComSnapshot2.Name, + Namespace: "default", + }, hasComSnapshot2) + return err + }, time.Second*10).ShouldNot(HaveOccurred()) + Expect(k8sClient.Create(adapter.context, hasComSnapshot3)).Should(Succeed()) + Eventually(func() error { + err := k8sClient.Get(adapter.context, types.NamespacedName{ + Name: hasComSnapshot3.Name, + Namespace: "default", + }, hasComSnapshot3) + return err + }, time.Second*10).ShouldNot(HaveOccurred()) + result, err := adapter.EnsureGroupSnapshotExist() Expect(result.CancelRequest).To(BeFalse()) Expect(result.RequeueRequest).To(BeFalse()) @@ -1794,6 +1817,36 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) + It("Stop processing when there is only one affected for pr group", func() { + var buf bytes.Buffer + log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} + adapter = NewAdapter(ctx, hasComSnapshot1, hasApp, log, loader.NewMockLoader(), k8sClient) + adapter.context = toolkit.GetMockedContext(ctx, []toolkit.MockData{ + { + ContextKey: loader.ApplicationContextKey, + Resource: hasApp, + }, + { + ContextKey: loader.SnapshotContextKey, + Resource: hasComSnapshot1, + }, + { + ContextKey: loader.GetBuildPLRContextKey, + Resource: []tektonv1.PipelineRun{}, + }, + { + ContextKey: loader.ApplicationComponentsContextKey, + Resource: []applicationapiv1alpha1.Component{*hasCom1, *hasCom3}, + }, + }) + + result, err := adapter.EnsureGroupSnapshotExist() + Expect(result.CancelRequest).To(BeFalse()) + Expect(result.RequeueRequest).To(BeFalse()) + Expect(buf.String()).Should(ContainSubstring("The number 1 of component affected by this pr group feature1 is less than 2, skipping group snapshot creation")) + Expect(err).ToNot(HaveOccurred()) + }) + It("Ensure group snasphot can be created", func() { var buf bytes.Buffer log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} @@ -1823,7 +1876,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { }, { ContextKey: loader.ApplicationComponentsContextKey, - Resource: []applicationapiv1alpha1.Component{*hasCom1, *hasCom3}, + Resource: []applicationapiv1alpha1.Component{*hasComp, *hasCom1, *hasCom3, *hasCompMissingImageDigest, *hasCompWithValidImage}, }, }) @@ -1848,9 +1901,9 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { result, err := adapter.EnsureGroupSnapshotExist() Expect(result.CancelRequest).To(BeFalse()) Expect(result.RequeueRequest).To(BeFalse()) - Expect(buf.String()).Should(ContainSubstring("")) - Expect(err).ToNot(HaveOccurred()) - fmt.Fprintf(GinkgoWriter, "-------test: %v\n", buf.String()) + Expect(buf.String()).Should(ContainSubstring("component cannot be added to snapshot for application due to invalid digest in containerImage")) + Expect(buf.String()).Should(ContainSubstring("component with containerImage from Global Candidate List will be added to group snapshot")) + Expect(err).NotTo(HaveOccurred()) Eventually(func() bool { _ = k8sClient.Get(adapter.context, types.NamespacedName{ Name: hasComSnapshot2.Name, diff --git a/loader/loader.go b/loader/loader.go index f2298790d..69fc42092 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -58,6 +58,7 @@ type ObjectLoader interface { GetComponent(ctx context.Context, c client.Client, name, namespace string) (*applicationapiv1alpha1.Component, error) GetPipelineRunsWithPRGroupHash(ctx context.Context, c client.Client, snapshot *applicationapiv1alpha1.Snapshot, prGroupHash string) (*[]tektonv1.PipelineRun, error) GetMatchingComponentSnapshotsForComponentAndPRGroupHash(ctx context.Context, c client.Client, snapshot *applicationapiv1alpha1.Snapshot, componentName, prGroupHash string) (*[]applicationapiv1alpha1.Snapshot, error) + GetMatchingComponentSnapshotsForPRGroupHash(ctx context.Context, c client.Client, snapshot *applicationapiv1alpha1.Snapshot, prGroupHash string) (*[]applicationapiv1alpha1.Snapshot, error) } type loader struct{} @@ -437,3 +438,37 @@ func (l *loader) GetMatchingComponentSnapshotsForComponentAndPRGroupHash(ctx con } return &snapshots.Items, nil } + +// GetMatchingComponentSnapshotsForPRGroupHash gets the component snapshot with the given pr group hash string and the the same namespace with the given snapshot +func (l *loader) GetMatchingComponentSnapshotsForPRGroupHash(ctx context.Context, c client.Client, snapshot *applicationapiv1alpha1.Snapshot, prGroupHash string) (*[]applicationapiv1alpha1.Snapshot, error) { + snapshots := &applicationapiv1alpha1.SnapshotList{} + + eventTypeLabelRequirement, err := labels.NewRequirement("pac.test.appstudio.openshift.io/event-type", selection.NotIn, []string{"push", "Push"}) + if err != nil { + return nil, err + } + prGroupLabelRequirement, err := labels.NewRequirement("test.appstudio.openshift.io/pr-group-sha", selection.In, []string{prGroupHash}) + if err != nil { + return nil, err + } + snapshotTypeLabelRequirement, err := labels.NewRequirement("test.appstudio.openshift.io/type", selection.In, []string{"component"}) + if err != nil { + return nil, err + } + + labelSelector := labels.NewSelector(). + Add(*eventTypeLabelRequirement). + Add(*prGroupLabelRequirement). + Add(*snapshotTypeLabelRequirement) + + opts := &client.ListOptions{ + Namespace: snapshot.Namespace, + LabelSelector: labelSelector, + } + + err = c.List(ctx, snapshots, opts) + if err != nil { + return nil, err + } + return &snapshots.Items, nil +} diff --git a/loader/loader_mock.go b/loader/loader_mock.go index 2478f6391..8d419d79a 100644 --- a/loader/loader_mock.go +++ b/loader/loader_mock.go @@ -57,6 +57,7 @@ const ( GetComponentContextKey GetBuildPLRContextKey GetComponentSnapshotsKey + GetPRSnapshotsKey ) func NewMockLoader() ObjectLoader { @@ -233,3 +234,12 @@ func (l *mockLoader) GetMatchingComponentSnapshotsForComponentAndPRGroupHash(ctx snapshots, err := toolkit.GetMockedResourceAndErrorFromContext(ctx, GetComponentSnapshotsKey, []applicationapiv1alpha1.Snapshot{}) return &snapshots, err } + +// GetMatchingComponentSnapshotsForPRGroupHash returns the resource and error passed as values of the context +func (l *mockLoader) GetMatchingComponentSnapshotsForPRGroupHash(ctx context.Context, c client.Client, snapshot *applicationapiv1alpha1.Snapshot, prGroupHash string) (*[]applicationapiv1alpha1.Snapshot, error) { + if ctx.Value(GetPRSnapshotsKey) == nil { + return l.loader.GetMatchingComponentSnapshotsForPRGroupHash(ctx, c, snapshot, prGroupHash) + } + snapshots, err := toolkit.GetMockedResourceAndErrorFromContext(ctx, GetPRSnapshotsKey, []applicationapiv1alpha1.Snapshot{}) + return &snapshots, err +} diff --git a/loader/loader_mock_test.go b/loader/loader_mock_test.go index d26bcbc9e..bcfa11195 100644 --- a/loader/loader_mock_test.go +++ b/loader/loader_mock_test.go @@ -320,4 +320,19 @@ var _ = Describe("Release Adapter", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) }) + + Context("When calling GetMatchingComponentSnapshotsForPRGroupHash", func() { + It("returns resource and error from the context", func() { + snapshots := []applicationapiv1alpha1.Snapshot{} + mockContext := toolkit.GetMockedContext(ctx, []toolkit.MockData{ + { + ContextKey: GetPRSnapshotsKey, + Resource: snapshots, + }, + }) + resource, err := loader.GetMatchingComponentSnapshotsForPRGroupHash(mockContext, nil, nil, "") + Expect(resource).To(Equal(&snapshots)) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) diff --git a/loader/loader_test.go b/loader/loader_test.go index d86e7131f..49af709ce 100644 --- a/loader/loader_test.go +++ b/loader/loader_test.go @@ -658,5 +658,13 @@ var _ = Describe("Loader", Ordered, func() { Expect((*fetchedSnapshots)[0].Namespace).To(Equal(hasSnapshot.Namespace)) Expect((*fetchedSnapshots)[0].Spec).To(Equal(hasSnapshot.Spec)) }) + + It("Can get matching snapshot for pr group hash", func() { + fetchedSnapshots, err := loader.GetMatchingComponentSnapshotsForPRGroupHash(ctx, k8sClient, hasSnapshot, "featuresha") + Expect(err).To(Succeed()) + Expect((*fetchedSnapshots)[0].Name).To(Equal(hasSnapshot.Name)) + Expect((*fetchedSnapshots)[0].Namespace).To(Equal(hasSnapshot.Namespace)) + Expect((*fetchedSnapshots)[0].Spec).To(Equal(hasSnapshot.Spec)) + }) }) }) From 503e395019aa49e7ff3bb5a3039d1529f6366aba Mon Sep 17 00:00:00 2001 From: Hongwei Liu Date: Tue, 5 Nov 2024 19:41:05 +0800 Subject: [PATCH 2/2] chore: reduce complexity in prepareGroupSnapshot * reduce complexity in func prepareGroupSnapshot Signed-off-by: Hongwei Liu --- .../controller/snapshot/snapshot_adapter.go | 82 ++++++++++--------- .../snapshot/snapshot_adapter_test.go | 4 +- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/internal/controller/snapshot/snapshot_adapter.go b/internal/controller/snapshot/snapshot_adapter.go index 89cefacbc..f56ae1ead 100644 --- a/internal/controller/snapshot/snapshot_adapter.go +++ b/internal/controller/snapshot/snapshot_adapter.go @@ -855,7 +855,7 @@ func (a *Adapter) updateComponentLastPromotedImage(ctx context.Context, c client } func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Application, prGroup, prGroupHash string) (*applicationapiv1alpha1.Snapshot, []gitops.ComponentSnapshotInfo, error) { - componentsToCheck, err := a.getComponentsForPRGroup(application, prGroup, prGroupHash) + componentsToCheck, err := a.getComponentsForPRGroup(prGroup, prGroupHash) if err != nil { return nil, nil, err } @@ -872,47 +872,30 @@ func (a *Adapter) prepareGroupSnapshot(application *applicationapiv1alpha1.Appli snapshotComponents := make([]applicationapiv1alpha1.SnapshotComponent, 0) componentSnapshotInfos := make([]gitops.ComponentSnapshotInfo, 0) for _, applicationComponent := range *applicationComponents { - var isPRMROpened bool applicationComponent := applicationComponent // G601 + var foundSnapshotWithOpenedPR *applicationapiv1alpha1.Snapshot if slices.Contains(componentsToCheck, applicationComponent.Name) { - snapshots, err := a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client, a.snapshot, applicationComponent.Name, prGroupHash) + foundSnapshotWithOpenedPR, err = a.findSnapshotWithOpenedPR(a.snapshot, &applicationComponent, prGroupHash) if err != nil { - a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", applicationComponent.Name) return nil, nil, err } - - sortedSnapshots := gitops.SortSnapshots(*snapshots) - // find the latest component snapshot created for open PR/MR - for _, snapshot := range sortedSnapshots { - snapshot := snapshot - // find the built image for pull/merge request build PLR from the latest opened pull request component snapshot - isPRMROpened, err = a.status.IsPRMRInSnapshotOpened(a.context, &snapshot) - if err != nil { - a.logger.Error(err, "Failed to fetch PR/MR status for component snapshot", "snapshot.Name", a.snapshot.Name) - return nil, nil, err - } - if isPRMROpened { - a.logger.Info("PR/MR in snapshot is opened, will find snapshotComponent and add to groupSnapshot") - snapshotComponent := gitops.FindMatchingSnapshotComponent(&snapshot, &applicationComponent) - componentSnapshotInfos = append(componentSnapshotInfos, gitops.ComponentSnapshotInfo{ - Component: applicationComponent.Name, - BuildPipelineRun: snapshot.Labels[gitops.BuildPipelineRunNameLabel], - Snapshot: snapshot.Name, - Namespace: a.snapshot.Namespace, - RepoUrl: snapshot.Annotations[gitops.PipelineAsCodeRepoUrlAnnotation], - PullRequestNumber: snapshot.Annotations[gitops.PipelineAsCodePullRequestAnnotation], - }) - snapshotComponents = append(snapshotComponents, snapshotComponent) - break - } + if foundSnapshotWithOpenedPR != nil { + a.logger.Info("PR/MR in snapshot is opened, will find snapshotComponent and add to groupSnapshot") + snapshotComponent := gitops.FindMatchingSnapshotComponent(foundSnapshotWithOpenedPR, &applicationComponent) + componentSnapshotInfos = append(componentSnapshotInfos, gitops.ComponentSnapshotInfo{ + Component: applicationComponent.Name, + BuildPipelineRun: foundSnapshotWithOpenedPR.Labels[gitops.BuildPipelineRunNameLabel], + Snapshot: foundSnapshotWithOpenedPR.Name, + Namespace: a.snapshot.Namespace, + RepoUrl: foundSnapshotWithOpenedPR.Annotations[gitops.PipelineAsCodeRepoUrlAnnotation], + PullRequestNumber: foundSnapshotWithOpenedPR.Annotations[gitops.PipelineAsCodePullRequestAnnotation], + }) + snapshotComponents = append(snapshotComponents, snapshotComponent) + continue } } - // isPRMROpened represents snapshotComponent can be gottent from PR component snapshot - // so continue next applicationComponent - if isPRMROpened { - continue - } + a.logger.Info("can't find snapshot with open pull/merge request for component, try to find snapshotComponent from Global Candidate List", "component", applicationComponent.Name) // if there is no component snapshot found for open PR/MR, we get snapshotComponent from gcl componentSource := gitops.GetComponentSourceFromComponent(&applicationComponent) @@ -1028,11 +1011,11 @@ func (a *Adapter) haveAllPipelineRunProcessedForPrGroup(prGroup, prGroupHash str return true, nil } -// getComponentsAffectedPrGroup returns the component names affected by the given pr group hash -func (a *Adapter) getComponentsForPRGroup(application *applicationapiv1alpha1.Application, prGroup, prGroupHash string) ([]string, error) { +// getComponentsForPRGroup returns the component names affected by the given pr group hash +func (a *Adapter) getComponentsForPRGroup(prGroup, prGroupHash string) ([]string, error) { snapshots, err := a.loader.GetMatchingComponentSnapshotsForPRGroupHash(a.context, a.client, a.snapshot, prGroupHash) if err != nil { - a.logger.Error(err, fmt.Sprintf("Failed to fetch Snapshots for pr group %s", prGroupHash)) + a.logger.Error(err, fmt.Sprintf("Failed to fetch Snapshots for pr group %s", prGroup)) return nil, err } @@ -1046,3 +1029,28 @@ func (a *Adapter) getComponentsForPRGroup(application *applicationapiv1alpha1.Ap } return componentNames, nil } + +func (a *Adapter) findSnapshotWithOpenedPR(snapshot *applicationapiv1alpha1.Snapshot, applicationComponent *applicationapiv1alpha1.Component, prGroupHash string) (*applicationapiv1alpha1.Snapshot, error) { + snapshots, err := a.loader.GetMatchingComponentSnapshotsForComponentAndPRGroupHash(a.context, a.client, a.snapshot, applicationComponent.Name, prGroupHash) + if err != nil { + a.logger.Error(err, "Failed to fetch Snapshots for component", "component.Name", applicationComponent.Name) + return nil, err + } + + sortedSnapshots := gitops.SortSnapshots(*snapshots) + // find the latest component snapshot created for open PR/MR + for _, snapshot := range sortedSnapshots { + snapshot := snapshot + // find the built image for pull/merge request build PLR from the latest opened pull request component snapshot + isPRMROpened, err := a.status.IsPRMRInSnapshotOpened(a.context, &snapshot) + if err != nil { + a.logger.Error(err, "Failed to fetch PR/MR status for component snapshot", "snapshot.Name", a.snapshot.Name) + return nil, err + } + if isPRMROpened { + a.logger.Info("PR/MR in snapshot is opened, will find snapshotComponent and add to groupSnapshot") + return &snapshot, nil + } + } + return nil, nil +} diff --git a/internal/controller/snapshot/snapshot_adapter_test.go b/internal/controller/snapshot/snapshot_adapter_test.go index f376dd12c..bdf7ca160 100644 --- a/internal/controller/snapshot/snapshot_adapter_test.go +++ b/internal/controller/snapshot/snapshot_adapter_test.go @@ -1817,7 +1817,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { Expect(err).ToNot(HaveOccurred()) }) - It("Stop processing when there is only one affected for pr group", func() { + It("Stop processing when there is only one component affected for pr group", func() { var buf bytes.Buffer log := helpers.IntegrationLogger{Logger: buflogr.NewWithBuffer(&buf)} adapter = NewAdapter(ctx, hasComSnapshot1, hasApp, log, loader.NewMockLoader(), k8sClient) @@ -1843,7 +1843,7 @@ var _ = Describe("Snapshot Adapter", Ordered, func() { result, err := adapter.EnsureGroupSnapshotExist() Expect(result.CancelRequest).To(BeFalse()) Expect(result.RequeueRequest).To(BeFalse()) - Expect(buf.String()).Should(ContainSubstring("The number 1 of component affected by this pr group feature1 is less than 2, skipping group snapshot creation")) + Expect(buf.String()).Should(ContainSubstring("The number 1 of components affected by this PR group feature1 is less than 2, skipping group snapshot creation")) Expect(err).ToNot(HaveOccurred()) })