From 9c8e857364daec084e60e394d183643784cb0cc6 Mon Sep 17 00:00:00 2001 From: Robert Cerven Date: Tue, 10 Dec 2024 21:33:46 +0100 Subject: [PATCH] when offbording don't remove webhook from gitlab if other component is using same repository as well KONFLUX-5944 Signed-off-by: Robert Cerven --- controllers/component_build_controller_pac.go | 24 ++++++++++- .../component_build_controller_test.go | 41 ++++++++++++++++--- controllers/renovate_util.go | 6 +-- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/controllers/component_build_controller_pac.go b/controllers/component_build_controller_pac.go index d814b7fb..a7960b30 100644 --- a/controllers/component_build_controller_pac.go +++ b/controllers/component_build_controller_pac.go @@ -33,6 +33,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" ctrllog "sigs.k8s.io/controller-runtime/pkg/log" "github.com/konflux-ci/build-service/pkg/boerrors" @@ -534,8 +535,27 @@ func (r *ComponentBuildReconciler) UnconfigureRepositoryForPaC(ctx context.Conte } isAppUsed := IsPaCApplicationConfigured(gitProvider, pacConfig) - if !isAppUsed { - if webhookTargetUrl != "" { + if !isAppUsed && webhookTargetUrl != "" { + gitUrl := strings.TrimSuffix(strings.TrimSuffix(component.Spec.Source.GitSource.URL, ".git"), "/") + componentList := &appstudiov1alpha1.ComponentList{} + if err := r.Client.List(ctx, componentList, &client.ListOptions{Namespace: component.Namespace}); err != nil { + log.Error(err, "failed to list components") + return "", "", "", err + } + + sameRepoUsed := false + for _, comp := range componentList.Items { + if comp.Name == component.Name { + continue + } + componentUrl := strings.TrimSuffix(strings.TrimSuffix(comp.Spec.Source.GitSource.URL, ".git"), "/") + if componentUrl == gitUrl { + sameRepoUsed = true + break + } + } + + if !sameRepoUsed { err = gitClient.DeletePaCWebhook(repoUrl, webhookTargetUrl) if err != nil { // Just log the error and continue with merge request creation diff --git a/controllers/component_build_controller_test.go b/controllers/component_build_controller_test.go index 154904cf..817450bf 100644 --- a/controllers/component_build_controller_test.go +++ b/controllers/component_build_controller_test.go @@ -1000,7 +1000,7 @@ var _ = Describe("Component initial build controller", func() { isRemovePaCPullRequestInvoked := false UndoPaCMergeRequestFunc = func(repoUrl string, d *gp.MergeRequestData) (webUrl string, err error) { isRemovePaCPullRequestInvoked = true - Expect(repoUrl).To(Equal(SampleRepoLink + "-" + resourceCleanupKey.Name)) + Expect(repoUrl).To(Equal(SampleRepoLink + "-samerepo")) Expect(len(d.Files)).To(Equal(2)) for _, file := range d.Files { Expect(strings.HasPrefix(file.FullPath, ".tekton/")).To(BeTrue()) @@ -1014,11 +1014,9 @@ var _ = Describe("Component initial build controller", func() { Expect(d.AuthorEmail).ToNot(BeEmpty()) return mergeUrl, nil } - isDeletePaCWebhookInvoked := false DeletePaCWebhookFunc = func(repoUrl string, webhookUrl string) error { - isDeletePaCWebhookInvoked = true - Expect(repoUrl).To(Equal(SampleRepoLink + "-" + resourceCleanupKey.Name)) - Expect(webhookUrl).To(Equal(pacWebhookUrl)) + defer GinkgoRecover() + Fail("Should not remove webhook because another component is using the same repository") return nil } @@ -1028,11 +1026,42 @@ var _ = Describe("Component initial build controller", func() { createComponentAndProcessBuildRequest(componentConfig{ componentKey: resourceCleanupKey, annotations: defaultPipelineAnnotations, + gitURL: SampleRepoLink + "-samerepo", }, BuildRequestConfigurePaCAnnotationValue) waitPaCFinalizerOnComponent(resourceCleanupKey) + // create second component which uses the same url + secondComponentKey := types.NamespacedName{Name: HASCompName + "-cleanup-second", Namespace: HASAppNamespace} + createComponentAndProcessBuildRequest(componentConfig{ + componentKey: secondComponentKey, + annotations: defaultPipelineAnnotations, + gitURL: SampleRepoLink + "-samerepo", + }, BuildRequestConfigurePaCAnnotationValue) + waitPaCFinalizerOnComponent(secondComponentKey) + + // shouldn't remove webhook because another component exists for the same repo setComponentBuildRequest(resourceCleanupKey, BuildRequestUnconfigurePaCAnnotationValue) + Eventually(func() bool { + return isRemovePaCPullRequestInvoked + }, timeout, interval).Should(BeTrue()) + + expectPacBuildStatus(resourceCleanupKey, "disabled", 0, "", mergeUrl) + // delete component so there will be just 1 component using the repository + deleteComponent(resourceCleanupKey) + + isRemovePaCPullRequestInvoked = false + isDeletePaCWebhookInvoked := false + DeletePaCWebhookFunc = func(repoUrl string, webhookUrl string) error { + isDeletePaCWebhookInvoked = true + Expect(repoUrl).To(Equal(SampleRepoLink + "-samerepo")) + Expect(webhookUrl).To(Equal(pacWebhookUrl)) + return nil + } + + // should remove webhook because there isn't another component which uses the same repo + setComponentBuildRequest(secondComponentKey, BuildRequestUnconfigurePaCAnnotationValue) + Eventually(func() bool { return isRemovePaCPullRequestInvoked }, timeout, interval).Should(BeTrue()) @@ -1040,7 +1069,7 @@ var _ = Describe("Component initial build controller", func() { return isDeletePaCWebhookInvoked }, timeout, interval).Should(BeTrue()) - expectPacBuildStatus(resourceCleanupKey, "disabled", 0, "", mergeUrl) + expectPacBuildStatus(secondComponentKey, "disabled", 0, "", mergeUrl) }) It("should not block component deletion if PaC definitions removal failed", func() { diff --git a/controllers/renovate_util.go b/controllers/renovate_util.go index bc11dc2b..bbef994d 100644 --- a/controllers/renovate_util.go +++ b/controllers/renovate_util.go @@ -273,9 +273,9 @@ func (u ComponentDependenciesUpdater) GetUpdateTargetsGithubApp(ctx context.Cont } targetsToUpdate = append(targetsToUpdate, updateTarget{ - ComponentName: component.Name, - GitProvider: gitProvider, - Username: fmt.Sprintf("%s[bot]", slug), + ComponentName: component.Name, + GitProvider: gitProvider, + Username: fmt.Sprintf("%s[bot]", slug), // hardcoding the number because mintmaker has it hardcoded as well, so that way mintmaker will recognize the same author GitAuthor: fmt.Sprintf("%s <126015336+%s[bot]@users.noreply.github.com>", slug, slug), Token: githubAppInstallation.Token,