Skip to content

Commit

Permalink
when offbording don't remove webhook from gitlab if other component is
Browse files Browse the repository at this point in the history
using same repository as well

KONFLUX-5944

Signed-off-by: Robert Cerven <[email protected]>
  • Loading branch information
rcerven committed Dec 16, 2024
1 parent da65d94 commit 9c8e857
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 11 deletions.
24 changes: 22 additions & 2 deletions controllers/component_build_controller_pac.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
41 changes: 35 additions & 6 deletions controllers/component_build_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
}

Expand All @@ -1028,19 +1026,50 @@ 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())
Eventually(func() bool {
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() {
Expand Down
6 changes: 3 additions & 3 deletions controllers/renovate_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 9c8e857

Please sign in to comment.