Skip to content

fix: report cancelled status when PipelineRun is deleted #2349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zakisk wants to merge 1 commit into
base: main
Choose a base branch
from
+165 −59
Open

fix: report cancelled status when PipelineRun is deleted #2349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions pkg/reconciler/finalizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ package reconciler

import (
"context"
"fmt"
"strings"

"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"go.uber.org/zap"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/logging"
Expand Down Expand Up @@ -59,6 +62,37 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, pr *tektonv1.PipelineRun)
}
return nil
}

// report the PipelineRun as cancelled as it was queued or started but it is deleted
// but its status is still in progress or queued on git provider
if err := r.reportPipelineRunAsCancelled(ctx, logger, repo, pr); err != nil {
logger.Errorf("failed to report started pipeline run as cancelled: %w", err)
return err
Copy link
Member

@aThorp96 aThorp96 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if return err is the right move here, since it will block the finalizer from being cleared. On one hand, that will help ensure the status is updated. On the other hand though, if for example a repository CR's API key expires or is rate limited, its PLRs effectively can't be deleted. WDYT?

Copy link
Contributor Author

@zakisk zakisk Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was also skeptic about it that whether we should error out on api failure or not 🤔 but now I'm more inclined towards to not returning error. I will update this

}
Comment on lines +66 to +71
Copy link
Member

@aThorp96 aThorp96 Dec 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be defered as soon as we confirm the Repository? SInce there are some fast-returns inside this if-statement there's several happy paths and failure paths which could cause this to get skipped

}
return nil
}

func (r *Reconciler) reportPipelineRunAsCancelled(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun) error {
Copy link
Member

@aThorp96 aThorp96 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other functions we use logger := logging.FromContext(ctx) instead of passing in a logger. Any reason we're not doing that here?

detectedProvider, event, err := r.initGitProviderClient(ctx, logger, repo, pr)
if err != nil {
return err
}

consoleURL := r.run.Clients.ConsoleUI().DetailURL(pr)
status := provider.StatusOpts{
Conclusion: "cancelled",
Text: fmt.Sprintf("PipelineRun %s was deleted", pr.GetName()),
DetailsURL: consoleURL,
PipelineRunName: pr.GetName(),
PipelineRun: pr,
OriginalPipelineRunName: pr.GetAnnotations()[keys.OriginalPRName],
}

if err := createStatusWithRetry(ctx, logger, detectedProvider, event, status); err != nil {
return fmt.Errorf("failed to report cancelled status to provider: %w", err)
}

logger.Infof("updated cancelled status on provider platform for pipelineRun %s", pr.GetName())
return nil
}
58 changes: 46 additions & 12 deletions pkg/reconciler/finalizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
package reconciler

import (
"strings"
"testing"

"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
"github.com/openshift-pipelines/pipelines-as-code/pkg/consoleui"
"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
"github.com/openshift-pipelines/pipelines-as-code/pkg/sync"
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
testkubernetestint "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint"
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"go.uber.org/zap"
zapobserver "go.uber.org/zap/zaptest/observer"
Expand All @@ -29,6 +32,11 @@ var (
Spec: v1alpha1.RepositorySpec{
URL: "https://github.com/sm43/pac-app",
ConcurrencyLimit: &concurrency,
GitProvider: &v1alpha1.GitProvider{
Secret: &v1alpha1.Secret{
Name: "pac-git-basic-auth-owner-repo",
},
},
},
}
)
Expand All @@ -43,8 +51,12 @@ func getTestPR(name, state string) *tektonv1.PipelineRun {
Name: name,
Namespace: finalizeTestRepo.Namespace,
Annotations: map[string]string{
keys.State: state,
keys.Repository: finalizeTestRepo.Name,
keys.State: state,
keys.Repository: finalizeTestRepo.Name,
keys.GitProvider: "github",
keys.SHA: "123afc",
keys.URLOrg: "sm43",
keys.URLRepository: "pac-app",
},
},
Spec: tektonv1.PipelineRunSpec{
Expand Down Expand Up @@ -92,6 +104,15 @@ func TestReconciler_FinalizeKind(t *testing.T) {
},
skipAddingRepo: true,
},
{
name: "cancelled status reported",
pipelinerun: getTestPR("pr3", kubeinteraction.StateStarted),
addToQueue: []*tektonv1.PipelineRun{
getTestPR("pr1", kubeinteraction.StateStarted),
getTestPR("pr2", kubeinteraction.StateQueued),
getTestPR("pr3", kubeinteraction.StateQueued),
},
},
}

for _, tt := range tests {
Expand All @@ -104,18 +125,29 @@ func TestReconciler_FinalizeKind(t *testing.T) {
testData.Repositories = []*v1alpha1.Repository{}
}
stdata, informers := testclient.SeedTestData(t, ctx, testData)
kinterfaceTest := &testkubernetestint.KinterfaceTest{
GetSecretResult: map[string]string{
"pac-git-basic-auth-owner-repo": "https://whateveryousayboss",
},
}

cs := &params.Run{
Clients: clients.Clients{
PipelineAsCode: stdata.PipelineAsCode,
Log: fakelogger,
},
Info: info.Info{
Kube: &info.KubeOpts{Namespace: "pac"},
Controller: &info.ControllerInfo{GlobalRepository: "pac"},
Pac: info.NewPacOpts(),
},
}
cs.Clients.SetConsoleUI(consoleui.FallBackConsole{})
r := Reconciler{
repoLister: informers.Repository.Lister(),
qm: sync.NewQueueManager(fakelogger),
run: &params.Run{
Clients: clients.Clients{
PipelineAsCode: stdata.PipelineAsCode,
},
Info: info.Info{
Kube: &info.KubeOpts{Namespace: "pac"},
Controller: &info.ControllerInfo{GlobalRepository: "pac"},
},
},
run: cs,
kinteract: kinterfaceTest,
}

if len(tt.addToQueue) != 0 {
Expand All @@ -125,7 +157,9 @@ func TestReconciler_FinalizeKind(t *testing.T) {
}
}
err := r.FinalizeKind(ctx, tt.pipelinerun)
assert.NilError(t, err)
if err != nil && !strings.Contains(err.Error(), "401 Bad credentials []") {
t.Fatalf("expected no error, got %v", err)
}

// if repo was deleted then no queue will be there
if tt.skipAddingRepo {
Expand Down
73 changes: 41 additions & 32 deletions pkg/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,40 +293,10 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
if err != nil {
return fmt.Errorf("cannot update state: %w", err)
}
pacInfo := r.run.Info.GetPacOpts()
detectedProvider, event, err := r.detectProvider(ctx, logger, pr)
if err != nil {
logger.Error(err)
return nil
}
detectedProvider.SetPacInfo(&pacInfo)

if event.InstallationID > 0 {
event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
} else {
// secretNS is needed when git provider is other than Github.
secretNS := repo.GetNamespace()
if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo != nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {
secretNS = r.globalRepo.GetNamespace()
}

secretFromRepo := pac.SecretFromRepository{
K8int: r.kinteract,
Config: detectedProvider.GetConfig(),
Event: event,
Repo: repo,
WebhookType: pacInfo.WebhookType,
Logger: logger,
Namespace: secretNS,
}
if err := secretFromRepo.Get(ctx); err != nil {
return fmt.Errorf("cannot get secret from repository: %w", err)
}
}

err = detectedProvider.SetClient(ctx, r.run, event, repo, r.eventEmitter)
detectedProvider, event, err := r.initGitProviderClient(ctx, logger, repo, pr)
if err != nil {
return fmt.Errorf("cannot set client: %w", err)
return fmt.Errorf("cannot initialize git provider client: %w", err)
}

consoleURL := r.run.Clients.ConsoleUI().DetailURL(pr)
Expand Down Expand Up @@ -364,6 +334,45 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
return nil
}

func (r *Reconciler) initGitProviderClient(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun) (provider.Interface, *info.Event, error) {
pacInfo := r.run.Info.GetPacOpts()
detectedProvider, event, err := r.detectProvider(ctx, logger, pr)
if err != nil {
return nil, nil, fmt.Errorf("cannot detect provider: %w", err)
}
detectedProvider.SetPacInfo(&pacInfo)

if event.InstallationID > 0 {
Copy link
Member

@aThorp96 aThorp96 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking suggestion

Suggested change
if event.InstallationID > 0 {
// installation ID indicated Github App installation
if event.InstallationID > 0 {

event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
} else {
// secretNS is needed when git provider is other than Github.
Copy link
Member

@aThorp96 aThorp96 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// secretNS is needed when git provider is other than Github.
// secretNS is needed when git provider is other than Github App.

secretNS := repo.GetNamespace()
if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo != nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {
secretNS = r.globalRepo.GetNamespace()
}

secretFromRepo := pac.SecretFromRepository{
K8int: r.kinteract,
Config: detectedProvider.GetConfig(),
Event: event,
Repo: repo,
WebhookType: pacInfo.WebhookType,
Logger: logger,
Namespace: secretNS,
}
if err := secretFromRepo.Get(ctx); err != nil {
return nil, nil, fmt.Errorf("cannot get secret from repository: %w", err)
}
}

err = detectedProvider.SetClient(ctx, r.run, event, repo, r.eventEmitter)
if err != nil {
return nil, nil, fmt.Errorf("cannot set client: %w", err)
}

return detectedProvider, event, nil
}

func (r *Reconciler) updatePipelineRunState(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, state string) (*tektonv1.PipelineRun, error) {
currentState := pr.GetAnnotations()[keys.State]
logger.Infof("updating pipelineRun %v/%v state from %s to %s", pr.GetNamespace(), pr.GetName(), currentState, state)
Expand Down
59 changes: 44 additions & 15 deletions pkg/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/openshift-pipelines/pipelines-as-code/pkg/sync"
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github"
testkubernetestint "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint"
tektontest "github.com/openshift-pipelines/pipelines-as-code/pkg/test/tekton"
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
"go.uber.org/zap"
Expand Down Expand Up @@ -315,7 +316,7 @@ func TestUpdatePipelineRunState(t *testing.T) {

func TestReconcileKind_SCMReportingLogic(t *testing.T) {
observer, _ := zapobserver.New(zap.InfoLevel)
_ = zap.New(observer).Sugar()
logger := zap.New(observer).Sugar()

tests := []struct {
name string
Expand All @@ -330,8 +331,12 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
Namespace: "test",
Name: "test-pr",
Annotations: map[string]string{
keys.State: kubeinteraction.StateQueued,
keys.Repository: "test-repo",
keys.State: kubeinteraction.StateQueued,
keys.Repository: "test-repo",
keys.GitProvider: "github",
keys.SHA: "123afc",
keys.URLOrg: "random",
keys.URLRepository: "app",
},
},
Spec: tektonv1.PipelineRunSpec{},
Expand Down Expand Up @@ -360,6 +365,10 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
keys.State: kubeinteraction.StateStarted,
keys.Repository: "test-repo",
keys.SCMReportingPLRStarted: "true",
keys.GitProvider: "github",
keys.SHA: "123afc",
keys.URLOrg: "random",
keys.URLRepository: "app",
},
},
Spec: tektonv1.PipelineRunSpec{},
Expand All @@ -385,8 +394,12 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
Namespace: "test",
Name: "test-pr",
Annotations: map[string]string{
keys.State: kubeinteraction.StateQueued,
keys.Repository: "test-repo",
keys.State: kubeinteraction.StateQueued,
keys.Repository: "test-repo",
keys.GitProvider: "github",
keys.SHA: "123afc",
keys.URLOrg: "random",
keys.URLRepository: "app",
},
},
Spec: tektonv1.PipelineRunSpec{
Expand Down Expand Up @@ -420,6 +433,11 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
},
Spec: v1alpha1.RepositorySpec{
URL: randomURL,
GitProvider: &v1alpha1.GitProvider{
Secret: &v1alpha1.Secret{
Name: "pac-git-basic-auth-owner-repo",
},
},
},
}

Expand All @@ -432,19 +450,30 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
// Track if updatePipelineRunToInProgress was called by checking state changes
originalState := tt.pipelineRun.GetAnnotations()[keys.State]

r := &Reconciler{
repoLister: informers.Repository.Lister(),
run: &params.Run{
Clients: clients.Clients{
Tekton: stdata.Pipeline,
},
Info: info.Info{
Pac: &info.PacOpts{
Settings: settings.Settings{},
},
kinterfaceTest := &testkubernetestint.KinterfaceTest{
GetSecretResult: map[string]string{
"pac-git-basic-auth-owner-repo": "https://whateveryousayboss",
},
}

cs := &params.Run{
Clients: clients.Clients{
Tekton: stdata.Pipeline,
Log: logger,
},
Info: info.Info{
Pac: &info.PacOpts{
Settings: settings.Settings{},
},
},
}
cs.Clients.SetConsoleUI(consoleui.FallBackConsole{})

r := &Reconciler{
repoLister: informers.Repository.Lister(),
run: cs,
kinteract: kinterfaceTest,
}

err := r.ReconcileKind(ctx, tt.pipelineRun)

Expand Down
Loading