Skip to content

Commit c690b58

Browse files
zakiskzakisk
committed
fix: report cancelled status when PipelineRun is deleted
When a PipelineRun is deleted while it is in 'Running' or 'Queued' state (i.e., not yet completed), the status on the Git provider (e.g., GitHub) remained stuck in 'Pending' or 'Running'. This commit adds logic to the Finalizer to detect when a PipelineRun is being deleted (finalized) while in an active state. It now: 1. Reports a "cancelled" status to the Git provider, ensuring the commit status is correctly updated to "cancelled" instead of hanging. 2. Added unit test case to confirm that status is reported cancelled. This improves the user experience by accurately reflecting the PipelineRun lifecycle events on the Git provider UI. https://issues.redhat.com/browse/SRVKP-8318 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
1 parent 618fac5 commit c690b58

File tree

4 files changed

+168
-59
lines changed

4 files changed

+168
-59
lines changed

‎pkg/reconciler/finalizer.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@ package reconciler
22

33
import (
44
"context"
5+
"fmt"
56
"strings"
67

78
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
89
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
910
"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
11+
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
1012
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
1113
"k8s.io/apimachinery/pkg/api/errors"
1214
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -37,6 +39,13 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, pr *tektonv1.PipelineRun)
3739
if err != nil {
3840
return err
3941
}
42+
43+
// report the PipelineRun as cancelled as it was queued or started but it is deleted
44+
// but its status is still in progress or queued on git provider
45+
if err := r.reportPipelineRunAsCancelled(ctx, repo, pr); err != nil {
46+
logger.Errorf("failed to report deleted pipeline run as cancelled: %w", err)
47+
}
48+
4049
r.secretNS = repo.GetNamespace()
4150
if r.globalRepo, err = r.repoLister.Repositories(r.run.Info.Kube.Namespace).Get(r.run.Info.Controller.GlobalRepository); err == nil && r.globalRepo != nil {
4251
if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {
@@ -62,3 +71,28 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, pr *tektonv1.PipelineRun)
6271
}
6372
return nil
6473
}
74+
75+
func (r *Reconciler) reportPipelineRunAsCancelled(ctx context.Context, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun) error {
76+
logger := logging.FromContext(ctx)
77+
detectedProvider, event, err := r.initGitProviderClient(ctx, logger, repo, pr)
78+
if err != nil {
79+
return err
80+
}
81+
82+
consoleURL := r.run.Clients.ConsoleUI().DetailURL(pr)
83+
status := provider.StatusOpts{
84+
Conclusion: "cancelled",
85+
Text: fmt.Sprintf("PipelineRun %s was deleted", pr.GetName()),
86+
DetailsURL: consoleURL,
87+
PipelineRunName: pr.GetName(),
88+
PipelineRun: pr,
89+
OriginalPipelineRunName: pr.GetAnnotations()[keys.OriginalPRName],
90+
}
91+
92+
if err := createStatusWithRetry(ctx, logger, detectedProvider, event, status); err != nil {
93+
return fmt.Errorf("failed to report cancelled status to provider: %w", err)
94+
}
95+
96+
logger.Infof("updated cancelled status on provider platform for pipelineRun %s", pr.GetName())
97+
return nil
98+
}

‎pkg/reconciler/finalizer_test.go‎

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,25 @@
11
package reconciler
22

33
import (
4+
"strings"
45
"testing"
56

67
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
78
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
9+
"github.com/openshift-pipelines/pipelines-as-code/pkg/consoleui"
810
"github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction"
911
"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
1012
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
1113
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
1214
"github.com/openshift-pipelines/pipelines-as-code/pkg/sync"
1315
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
16+
testkubernetestint "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint"
1417
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
1518
"go.uber.org/zap"
1619
zapobserver "go.uber.org/zap/zaptest/observer"
1720
"gotest.tools/v3/assert"
1821
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
22+
"knative.dev/pkg/logging"
1923
rtesting "knative.dev/pkg/reconciler/testing"
2024
)
2125

@@ -29,6 +33,11 @@ var (
2933
Spec: v1alpha1.RepositorySpec{
3034
URL: "https://github.com/sm43/pac-app",
3135
ConcurrencyLimit: &concurrency,
36+
GitProvider: &v1alpha1.GitProvider{
37+
Secret: &v1alpha1.Secret{
38+
Name: "pac-git-basic-auth-owner-repo",
39+
},
40+
},
3241
},
3342
}
3443
)
@@ -43,8 +52,12 @@ func getTestPR(name, state string) *tektonv1.PipelineRun {
4352
Name: name,
4453
Namespace: finalizeTestRepo.Namespace,
4554
Annotations: map[string]string{
46-
keys.State: state,
47-
keys.Repository: finalizeTestRepo.Name,
55+
keys.State: state,
56+
keys.Repository: finalizeTestRepo.Name,
57+
keys.GitProvider: "github",
58+
keys.SHA: "123afc",
59+
keys.URLOrg: "sm43",
60+
keys.URLRepository: "pac-app",
4861
},
4962
},
5063
Spec: tektonv1.PipelineRunSpec{
@@ -92,30 +105,51 @@ func TestReconciler_FinalizeKind(t *testing.T) {
92105
},
93106
skipAddingRepo: true,
94107
},
108+
{
109+
name: "cancelled status reported",
110+
pipelinerun: getTestPR("pr3", kubeinteraction.StateStarted),
111+
addToQueue: []*tektonv1.PipelineRun{
112+
getTestPR("pr1", kubeinteraction.StateStarted),
113+
getTestPR("pr2", kubeinteraction.StateQueued),
114+
getTestPR("pr3", kubeinteraction.StateQueued),
115+
},
116+
},
95117
}
96118

97119
for _, tt := range tests {
98120
t.Run(tt.name, func(t *testing.T) {
99121
ctx, _ := rtesting.SetupFakeContext(t)
122+
ctx = logging.WithLogger(ctx, fakelogger)
100123
testData := testclient.Data{
101124
Repositories: []*v1alpha1.Repository{finalizeTestRepo},
102125
}
103126
if tt.skipAddingRepo {
104127
testData.Repositories = []*v1alpha1.Repository{}
105128
}
106129
stdata, informers := testclient.SeedTestData(t, ctx, testData)
130+
kinterfaceTest := &testkubernetestint.KinterfaceTest{
131+
GetSecretResult: map[string]string{
132+
"pac-git-basic-auth-owner-repo": "https://whateveryousayboss",
133+
},
134+
}
135+
136+
cs := &params.Run{
137+
Clients: clients.Clients{
138+
PipelineAsCode: stdata.PipelineAsCode,
139+
Log: fakelogger,
140+
},
141+
Info: info.Info{
142+
Kube: &info.KubeOpts{Namespace: "pac"},
143+
Controller: &info.ControllerInfo{GlobalRepository: "pac"},
144+
Pac: info.NewPacOpts(),
145+
},
146+
}
147+
cs.Clients.SetConsoleUI(consoleui.FallBackConsole{})
107148
r := Reconciler{
108149
repoLister: informers.Repository.Lister(),
109150
qm: sync.NewQueueManager(fakelogger),
110-
run: &params.Run{
111-
Clients: clients.Clients{
112-
PipelineAsCode: stdata.PipelineAsCode,
113-
},
114-
Info: info.Info{
115-
Kube: &info.KubeOpts{Namespace: "pac"},
116-
Controller: &info.ControllerInfo{GlobalRepository: "pac"},
117-
},
118-
},
151+
run: cs,
152+
kinteract: kinterfaceTest,
119153
}
120154

121155
if len(tt.addToQueue) != 0 {
@@ -125,7 +159,9 @@ func TestReconciler_FinalizeKind(t *testing.T) {
125159
}
126160
}
127161
err := r.FinalizeKind(ctx, tt.pipelinerun)
128-
assert.NilError(t, err)
162+
if err != nil && !strings.Contains(err.Error(), "401 Bad credentials []") {
163+
t.Fatalf("expected no error, got %v", err)
164+
}
129165

130166
// if repo was deleted then no queue will be there
131167
if tt.skipAddingRepo {

‎pkg/reconciler/reconciler.go‎

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -293,40 +293,10 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
293293
if err != nil {
294294
return fmt.Errorf("cannot update state: %w", err)
295295
}
296-
pacInfo := r.run.Info.GetPacOpts()
297-
detectedProvider, event, err := r.detectProvider(ctx, logger, pr)
298-
if err != nil {
299-
logger.Error(err)
300-
return nil
301-
}
302-
detectedProvider.SetPacInfo(&pacInfo)
303-
304-
if event.InstallationID > 0 {
305-
event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
306-
} else {
307-
// secretNS is needed when git provider is other than Github.
308-
secretNS := repo.GetNamespace()
309-
if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo != nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {
310-
secretNS = r.globalRepo.GetNamespace()
311-
}
312296

313-
secretFromRepo := pac.SecretFromRepository{
314-
K8int: r.kinteract,
315-
Config: detectedProvider.GetConfig(),
316-
Event: event,
317-
Repo: repo,
318-
WebhookType: pacInfo.WebhookType,
319-
Logger: logger,
320-
Namespace: secretNS,
321-
}
322-
if err := secretFromRepo.Get(ctx); err != nil {
323-
return fmt.Errorf("cannot get secret from repository: %w", err)
324-
}
325-
}
326-
327-
err = detectedProvider.SetClient(ctx, r.run, event, repo, r.eventEmitter)
297+
detectedProvider, event, err := r.initGitProviderClient(ctx, logger, repo, pr)
328298
if err != nil {
329-
return fmt.Errorf("cannot set client: %w", err)
299+
return fmt.Errorf("cannot initialize git provider client: %w", err)
330300
}
331301

332302
consoleURL := r.run.Clients.ConsoleUI().DetailURL(pr)
@@ -364,6 +334,46 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger *
364334
return nil
365335
}
366336

337+
func (r *Reconciler) initGitProviderClient(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun) (provider.Interface, *info.Event, error) {
338+
pacInfo := r.run.Info.GetPacOpts()
339+
detectedProvider, event, err := r.detectProvider(ctx, logger, pr)
340+
if err != nil {
341+
return nil, nil, fmt.Errorf("cannot detect provider: %w", err)
342+
}
343+
detectedProvider.SetPacInfo(&pacInfo)
344+
345+
// installation ID indicates Github App installation
346+
if event.InstallationID > 0 {
347+
event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run)
348+
} else {
349+
// secretNS is needed when git provider is other than Github App.
350+
secretNS := repo.GetNamespace()
351+
if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && r.globalRepo != nil && r.globalRepo.Spec.GitProvider != nil && r.globalRepo.Spec.GitProvider.Secret != nil {
352+
secretNS = r.globalRepo.GetNamespace()
353+
}
354+
355+
secretFromRepo := pac.SecretFromRepository{
356+
K8int: r.kinteract,
357+
Config: detectedProvider.GetConfig(),
358+
Event: event,
359+
Repo: repo,
360+
WebhookType: pacInfo.WebhookType,
361+
Logger: logger,
362+
Namespace: secretNS,
363+
}
364+
if err := secretFromRepo.Get(ctx); err != nil {
365+
return nil, nil, fmt.Errorf("cannot get secret from repository: %w", err)
366+
}
367+
}
368+
369+
err = detectedProvider.SetClient(ctx, r.run, event, repo, r.eventEmitter)
370+
if err != nil {
371+
return nil, nil, fmt.Errorf("cannot set client: %w", err)
372+
}
373+
374+
return detectedProvider, event, nil
375+
}
376+
367377
func (r *Reconciler) updatePipelineRunState(ctx context.Context, logger *zap.SugaredLogger, pr *tektonv1.PipelineRun, state string) (*tektonv1.PipelineRun, error) {
368378
currentState := pr.GetAnnotations()[keys.State]
369379
logger.Infof("updating pipelineRun %v/%v state from %s to %s", pr.GetNamespace(), pr.GetName(), currentState, state)

‎pkg/reconciler/reconciler_test.go‎

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/openshift-pipelines/pipelines-as-code/pkg/sync"
2525
testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
2626
ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github"
27+
testkubernetestint "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint"
2728
tektontest "github.com/openshift-pipelines/pipelines-as-code/pkg/test/tekton"
2829
tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
2930
"go.uber.org/zap"
@@ -315,7 +316,7 @@ func TestUpdatePipelineRunState(t *testing.T) {
315316

316317
func TestReconcileKind_SCMReportingLogic(t *testing.T) {
317318
observer, _ := zapobserver.New(zap.InfoLevel)
318-
_ = zap.New(observer).Sugar()
319+
logger := zap.New(observer).Sugar()
319320

320321
tests := []struct {
321322
name string
@@ -330,8 +331,12 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
330331
Namespace: "test",
331332
Name: "test-pr",
332333
Annotations: map[string]string{
333-
keys.State: kubeinteraction.StateQueued,
334-
keys.Repository: "test-repo",
334+
keys.State: kubeinteraction.StateQueued,
335+
keys.Repository: "test-repo",
336+
keys.GitProvider: "github",
337+
keys.SHA: "123afc",
338+
keys.URLOrg: "random",
339+
keys.URLRepository: "app",
335340
},
336341
},
337342
Spec: tektonv1.PipelineRunSpec{},
@@ -360,6 +365,10 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
360365
keys.State: kubeinteraction.StateStarted,
361366
keys.Repository: "test-repo",
362367
keys.SCMReportingPLRStarted: "true",
368+
keys.GitProvider: "github",
369+
keys.SHA: "123afc",
370+
keys.URLOrg: "random",
371+
keys.URLRepository: "app",
363372
},
364373
},
365374
Spec: tektonv1.PipelineRunSpec{},
@@ -385,8 +394,12 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
385394
Namespace: "test",
386395
Name: "test-pr",
387396
Annotations: map[string]string{
388-
keys.State: kubeinteraction.StateQueued,
389-
keys.Repository: "test-repo",
397+
keys.State: kubeinteraction.StateQueued,
398+
keys.Repository: "test-repo",
399+
keys.GitProvider: "github",
400+
keys.SHA: "123afc",
401+
keys.URLOrg: "random",
402+
keys.URLRepository: "app",
390403
},
391404
},
392405
Spec: tektonv1.PipelineRunSpec{
@@ -420,6 +433,11 @@ func TestReconcileKind_SCMReportingLogic(t *testing.T) {
420433
},
421434
Spec: v1alpha1.RepositorySpec{
422435
URL: randomURL,
436+
GitProvider: &v1alpha1.GitProvider{
437+
Secret: &v1alpha1.Secret{
438+
Name: "pac-git-basic-auth-owner-repo",
439+
},
440+
},
423441
},
424442
}
425443

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

435-
r := &Reconciler{
436-
repoLister: informers.Repository.Lister(),
437-
run: &params.Run{
438-
Clients: clients.Clients{
439-
Tekton: stdata.Pipeline,
440-
},
441-
Info: info.Info{
442-
Pac: &info.PacOpts{
443-
Settings: settings.Settings{},
444-
},
453+
kinterfaceTest := &testkubernetestint.KinterfaceTest{
454+
GetSecretResult: map[string]string{
455+
"pac-git-basic-auth-owner-repo": "https://whateveryousayboss",
456+
},
457+
}
458+
459+
cs := &params.Run{
460+
Clients: clients.Clients{
461+
Tekton: stdata.Pipeline,
462+
Log: logger,
463+
},
464+
Info: info.Info{
465+
Pac: &info.PacOpts{
466+
Settings: settings.Settings{},
445467
},
446468
},
447469
}
470+
cs.Clients.SetConsoleUI(consoleui.FallBackConsole{})
471+
472+
r := &Reconciler{
473+
repoLister: informers.Repository.Lister(),
474+
run: cs,
475+
kinteract: kinterfaceTest,
476+
}
448477

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

0 commit comments

Comments
 (0)
⚡