From f120625dbc9659605cb9f4eb8a5742518b67dab9 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Mon, 11 Aug 2025 09:47:37 +0200 Subject: [PATCH 1/6] chore: Run E2E tests based on author association - Replaced the hardcoded list of usernames in the E2E workflow trigger - Triggered E2E tests for pull requests from authors with OWNER, MEMBER, or COLLABORATOR association - Simplified maintenance by automatically allowing trusted contributors to run E2E tests Signed-off-by: Chmouel Boudjnah --- .github/workflows/e2e.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 5e59c059c..ac8f49e01 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -31,7 +31,8 @@ jobs: if: > github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || - (github.event_name == 'pull_request_target' && contains(fromJSON('["zakisk", "infernus01", "savitaashture", "chmouel", "vdemeester", "PuneetPunamiya", "enarha", "aThorp96", "sm43", "waveywaves", "dependabot[bot]"]'), github.event.pull_request.user.login)) + (github.event_name == 'pull_request_target' && + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.pull_request.author_association)) concurrency: group: ${{ github.workflow }}-${{ matrix.provider }}-${{ github.event.pull_request.number || github.ref_name }} cancel-in-progress: true From 1ba08cf18363de7f08c1116ff2e29d1d4c1ca507 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Fri, 1 Aug 2025 19:06:19 +0530 Subject: [PATCH 2/6] feat: Add template support for auto-configured repository CR names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces the ability to customize Repository custom resource names when auto-configuring new GitHub repositories using configurable templates with placeholder variables. Changes: • Add `auto-configure-repo-repository-template` configuration setting • Support `{{repo_owner}}` and `{{repo_name}}` template variables • Implement template processing in repository auto-configuration flow • Add comprehensive documentation with examples Configuration: The new `auto-configure-repo-repository-template` setting allows users to define custom naming patterns for automatically created Repository CRs. Supported template variables: - `{{repo_owner}}`: GitHub repository owner/organization name - `{{repo_name}}`: GitHub repository name Examples: - Template: `{{repo_owner}}-{{repo_name}}-repo-cr` - Repository: `https://github.com/acme/webapp` - Generated CR name: `acme-webapp-repo-cr` Backward compatibility: If no template is specified, the system falls back to the existing default format: `{{repo_name}}-repo-cr` This enhancement provides organizations with flexible naming conventions that can align with their existing Kubernetes resource management practices while maintaining the convenience of automatic repository setup. Signed-off-by: Zaki Shaikh --- config/302-pac-configmap.yaml | 14 ++- docs/content/docs/install/settings.md | 13 +++ pkg/params/settings/config.go | 21 ++-- pkg/params/settings/config_test.go | 96 ++++++++-------- pkg/provider/github/repository.go | 39 +++++-- pkg/provider/github/repository_test.go | 145 ++++++++++++++++--------- 6 files changed, 206 insertions(+), 122 deletions(-) diff --git a/config/302-pac-configmap.yaml b/config/302-pac-configmap.yaml index 6fd0c1bea..2416a3923 100644 --- a/config/302-pac-configmap.yaml +++ b/config/302-pac-configmap.yaml @@ -139,12 +139,18 @@ data: # namespace and repository CR, supported only with GitHub App auto-configure-new-github-repo: "false" - # add a template to generate name for namespace for your auto configured - # github repo supported fields are repo_owner, repo_name eg. if defined as - # `{{repo_owner}}-{{repo_name}}-ci`, then namespace generated for repository - # https://github.com/owner/repo will be `owner-repo-ci` + # Add a template to generate the name for a Repository CR for an auto-configured + # GitHub repository. Supported fields are `repo_owner` and `repo_name`. e.g., + # if defined as `{{repo_owner}}-{{repo_name}}-repo-cr`, the generated CR name for + # https://github.com/owner/test will be `owner-test-repo-cr` auto-configure-repo-namespace-template: "" + # add a template to generate name for repository for your auto configured + # github repo. supported fields are repo_owner, repo_name eg. if defined as + # `{{repo_owner}}-{{repo_name}}-repo-cr`, then repository CR generated for git repository + # https://github.com/owner/test will be `owner-test-repo-cr` + auto-configure-repo-repository-template: "" + # Enable or disable the feature to rerun the CI if push event happens on # a pull request # diff --git a/docs/content/docs/install/settings.md b/docs/content/docs/install/settings.md index 71afba243..57f35d42d 100644 --- a/docs/content/docs/install/settings.md +++ b/docs/content/docs/install/settings.md @@ -124,6 +124,19 @@ There is a few things you can configure through the config map `https://github.com/owner/repo` will be `owner-repo-ci` +* `auto-configure-repo-repository-template` + + If `auto-configure-new-github-repo` is enabled then you can provide a template + for generating the name for your new repository custom resource. By default, the repository custom resource name will be generated using this format `{{repo_name}}-repo-cr`. + + You can override the default using the following variables + + * `{{repo_owner}}`: The repository owner. + * `{{repo_name}}`: The repository name. + For example, if the template is defined as `{{repo_owner}}-{{repo_name}}-repo-cr`, + then the Repository CR name generated for the repository + `https://github.com/owner/test` will be `owner-test-repo-cr` + * `remember-ok-to-test` If `remember-ok-to-test` is true then if `ok-to-test` is done on pull request then in diff --git a/pkg/params/settings/config.go b/pkg/params/settings/config.go index 86aa1659d..6c853fd56 100644 --- a/pkg/params/settings/config.go +++ b/pkg/params/settings/config.go @@ -48,16 +48,17 @@ type HubCatalog struct { // if there is a change performed on the default value, // update the same on "config/302-pac-configmap.yaml". type Settings struct { - ApplicationName string `default:"Pipelines as Code CI" json:"application-name"` - HubCatalogs *sync.Map - RemoteTasks bool `default:"true" json:"remote-tasks"` - MaxKeepRunsUpperLimit int `json:"max-keep-run-upper-limit"` - DefaultMaxKeepRuns int `json:"default-max-keep-runs"` - BitbucketCloudCheckSourceIP bool `default:"true" json:"bitbucket-cloud-check-source-ip"` - BitbucketCloudAdditionalSourceIP string `json:"bitbucket-cloud-additional-source-ip"` - TektonDashboardURL string `json:"tekton-dashboard-url"` - AutoConfigureNewGitHubRepo bool `default:"false" json:"auto-configure-new-github-repo"` - AutoConfigureRepoNamespaceTemplate string `json:"auto-configure-repo-namespace-template"` + ApplicationName string `default:"Pipelines as Code CI" json:"application-name"` + HubCatalogs *sync.Map + RemoteTasks bool `default:"true" json:"remote-tasks"` + MaxKeepRunsUpperLimit int `json:"max-keep-run-upper-limit"` + DefaultMaxKeepRuns int `json:"default-max-keep-runs"` + BitbucketCloudCheckSourceIP bool `default:"true" json:"bitbucket-cloud-check-source-ip"` + BitbucketCloudAdditionalSourceIP string `json:"bitbucket-cloud-additional-source-ip"` + TektonDashboardURL string `json:"tekton-dashboard-url"` + AutoConfigureNewGitHubRepo bool `default:"false" json:"auto-configure-new-github-repo"` + AutoConfigureRepoNamespaceTemplate string `json:"auto-configure-repo-namespace-template"` + AutoConfigureRepoRepositoryTemplate string `json:"auto-configure-repo-repository-template"` SecretAutoCreation bool `default:"true" json:"secret-auto-create"` SecretGHAppRepoScoped bool `default:"true" json:"secret-github-app-token-scoped"` diff --git a/pkg/params/settings/config_test.go b/pkg/params/settings/config_test.go index 64ae79c0c..707fdb646 100644 --- a/pkg/params/settings/config_test.go +++ b/pkg/params/settings/config_test.go @@ -52,55 +52,57 @@ func TestSyncConfig(t *testing.T) { { name: "override values", configMap: map[string]string{ - "application-name": "pac-pac", - "remote-tasks": "false", - "max-keep-run-upper-limit": "10", - "default-max-keep-runs": "5", - "bitbucket-cloud-check-source-ip": "false", - "bitbucket-cloud-additional-source-ip": "some-ip", - "tekton-dashboard-url": "https://tekton-dashboard", - "auto-configure-new-github-repo": "true", - "auto-configure-repo-namespace-template": "template", - "secret-auto-create": "false", - "secret-github-app-token-scoped": "false", - "secret-github-app-scope-extra-repos": "extra-repos", - "error-log-snippet": "false", - "error-detection-from-container-logs": "false", - "error-detection-max-number-of-lines": "100", - "error-detection-simple-regexp": "^(?P[^:]*):(?P[0-9]+):(?P[0-9]+)?([ ]*)?(?P.*)", - "custom-console-name": "custom-console", - "custom-console-url": "https://custom-console", - "custom-console-url-pr-details": "https://custom-console-pr-details", - "custom-console-url-pr-tasklog": "https://custom-console-pr-tasklog", - "custom-console-url-namespace": "https://custom-console-namespace", - "remember-ok-to-test": "false", - "skip-push-event-for-pr-commits": "true", + "application-name": "pac-pac", + "remote-tasks": "false", + "max-keep-run-upper-limit": "10", + "default-max-keep-runs": "5", + "bitbucket-cloud-check-source-ip": "false", + "bitbucket-cloud-additional-source-ip": "some-ip", + "tekton-dashboard-url": "https://tekton-dashboard", + "auto-configure-new-github-repo": "true", + "auto-configure-repo-namespace-template": "template", + "auto-configure-repo-repository-template": "template", + "secret-auto-create": "false", + "secret-github-app-token-scoped": "false", + "secret-github-app-scope-extra-repos": "extra-repos", + "error-log-snippet": "false", + "error-detection-from-container-logs": "false", + "error-detection-max-number-of-lines": "100", + "error-detection-simple-regexp": "^(?P[^:]*):(?P[0-9]+):(?P[0-9]+)?([ ]*)?(?P.*)", + "custom-console-name": "custom-console", + "custom-console-url": "https://custom-console", + "custom-console-url-pr-details": "https://custom-console-pr-details", + "custom-console-url-pr-tasklog": "https://custom-console-pr-tasklog", + "custom-console-url-namespace": "https://custom-console-namespace", + "remember-ok-to-test": "false", + "skip-push-event-for-pr-commits": "true", }, expectedStruct: Settings{ - ApplicationName: "pac-pac", - HubCatalogs: nil, - RemoteTasks: false, - MaxKeepRunsUpperLimit: 10, - DefaultMaxKeepRuns: 5, - BitbucketCloudCheckSourceIP: false, - BitbucketCloudAdditionalSourceIP: "some-ip", - TektonDashboardURL: "https://tekton-dashboard", - AutoConfigureNewGitHubRepo: true, - AutoConfigureRepoNamespaceTemplate: "template", - SecretAutoCreation: false, - SecretGHAppRepoScoped: false, - SecretGhAppTokenScopedExtraRepos: "extra-repos", - ErrorLogSnippet: false, - ErrorDetection: false, - ErrorDetectionNumberOfLines: 100, - ErrorDetectionSimpleRegexp: "^(?P[^:]*):(?P[0-9]+):(?P[0-9]+)?([ ]*)?(?P.*)", - CustomConsoleName: "custom-console", - CustomConsoleURL: "https://custom-console", - CustomConsolePRdetail: "https://custom-console-pr-details", - CustomConsolePRTaskLog: "https://custom-console-pr-tasklog", - CustomConsoleNamespaceURL: "https://custom-console-namespace", - RememberOKToTest: false, - SkipPushEventForPRCommits: true, + ApplicationName: "pac-pac", + HubCatalogs: nil, + RemoteTasks: false, + MaxKeepRunsUpperLimit: 10, + DefaultMaxKeepRuns: 5, + BitbucketCloudCheckSourceIP: false, + BitbucketCloudAdditionalSourceIP: "some-ip", + TektonDashboardURL: "https://tekton-dashboard", + AutoConfigureNewGitHubRepo: true, + AutoConfigureRepoNamespaceTemplate: "template", + AutoConfigureRepoRepositoryTemplate: "template", + SecretAutoCreation: false, + SecretGHAppRepoScoped: false, + SecretGhAppTokenScopedExtraRepos: "extra-repos", + ErrorLogSnippet: false, + ErrorDetection: false, + ErrorDetectionNumberOfLines: 100, + ErrorDetectionSimpleRegexp: "^(?P[^:]*):(?P[0-9]+):(?P[0-9]+)?([ ]*)?(?P.*)", + CustomConsoleName: "custom-console", + CustomConsoleURL: "https://custom-console", + CustomConsolePRdetail: "https://custom-console-pr-details", + CustomConsolePRTaskLog: "https://custom-console-pr-tasklog", + CustomConsoleNamespaceURL: "https://custom-console-namespace", + RememberOKToTest: false, + SkipPushEventForPRCommits: true, }, }, { diff --git a/pkg/provider/github/repository.go b/pkg/provider/github/repository.go index de320bffc..0f82685f1 100644 --- a/pkg/provider/github/repository.go +++ b/pkg/provider/github/repository.go @@ -19,7 +19,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -const defaultNsTemplate = "%v-pipelines" +const ( + defaultNsTemplate = "%v-pipelines" + defaultRepoTemplate = "%v-repo-cr" +) func ConfigureRepository(ctx context.Context, run *params.Run, req *http.Request, payload string, pacInfo *info.PacOpts, logger *zap.SugaredLogger) (bool, bool, error) { // check if repo auto configuration is enabled @@ -48,7 +51,9 @@ func ConfigureRepository(ctx context.Context, run *params.Run, req *http.Request } logger.Infof("github: configuring repository cr for repo: %v", repoEvent.Repo.GetHTMLURL()) - if err := createRepository(ctx, pacInfo.AutoConfigureRepoNamespaceTemplate, run.Clients, repoEvent, logger); err != nil { + nsTemplate := pacInfo.AutoConfigureRepoNamespaceTemplate + repoTemplate := pacInfo.AutoConfigureRepoRepositoryTemplate + if err := createRepository(ctx, nsTemplate, repoTemplate, run.Clients, repoEvent, logger); err != nil { logger.Errorf("failed repository creation: %v", err) return true, true, err } @@ -56,8 +61,8 @@ func ConfigureRepository(ctx context.Context, run *params.Run, req *http.Request return true, true, nil } -func createRepository(ctx context.Context, nsTemplate string, clients clients.Clients, gitEvent *github.RepositoryEvent, logger *zap.SugaredLogger) error { - repoNsName, err := generateNamespaceName(nsTemplate, gitEvent) +func createRepository(ctx context.Context, nsTemplate, repoTemplate string, clients clients.Clients, gitEvent *github.RepositoryEvent, logger *zap.SugaredLogger) error { + repoNsName, repoCRName, err := generateNamespaceAndRepositoryName(nsTemplate, repoTemplate, gitEvent) if err != nil { return fmt.Errorf("failed to generate namespace for repo: %w", err) } @@ -84,7 +89,7 @@ func createRepository(ctx context.Context, nsTemplate string, clients clients.Cl // create repository repo := &v1alpha1.Repository{ ObjectMeta: metav1.ObjectMeta{ - Name: repoNsName, + Name: repoCRName, Namespace: repoNsName, }, Spec: v1alpha1.RepositorySpec{ @@ -100,19 +105,29 @@ func createRepository(ctx context.Context, nsTemplate string, clients clients.Cl return nil } -func generateNamespaceName(nsTemplate string, gitEvent *github.RepositoryEvent) (string, error) { +func generateNamespaceAndRepositoryName(nsTemplate, repoTemplate string, gitEvent *github.RepositoryEvent) (string, string, error) { repoOwner, repoName, err := formatting.GetRepoOwnerSplitted(gitEvent.Repo.GetHTMLURL()) if err != nil { - return "", fmt.Errorf("failed to parse git repo url: %w", err) + return "", "", fmt.Errorf("failed to parse git repo url: %w", err) + } + + nsName := "" + repoCRName := "" + placeholders := map[string]string{ + "repo_owner": repoOwner, + "repo_name": repoName, } if nsTemplate == "" { - return fmt.Sprintf(defaultNsTemplate, repoName), nil + nsName = fmt.Sprintf(defaultNsTemplate, repoName) + } else { + nsName = templates.ReplacePlaceHoldersVariables(nsTemplate, placeholders, nil, http.Header{}, map[string]any{}) } - maptemplate := map[string]string{ - "repo_owner": repoOwner, - "repo_name": repoName, + if repoTemplate == "" { + repoCRName = fmt.Sprintf(defaultRepoTemplate, repoName) + } else { + repoCRName = templates.ReplacePlaceHoldersVariables(repoTemplate, placeholders, nil, http.Header{}, map[string]any{}) } - return templates.ReplacePlaceHoldersVariables(nsTemplate, maptemplate, nil, http.Header{}, map[string]any{}), nil + return nsName, repoCRName, nil } diff --git a/pkg/provider/github/repository_test.go b/pkg/provider/github/repository_test.go index 76a501a99..eca3b7ba5 100644 --- a/pkg/provider/github/repository_test.go +++ b/pkg/provider/github/repository_test.go @@ -39,16 +39,18 @@ func TestConfigureRepository(t *testing.T) { assert.NilError(t, err) tests := []struct { - name string - request *http.Request - eventType string - event []byte - detected bool - configuring bool - wantErr string - expectedNs string - nsTemplate string - testData testclient.Data + name string + request *http.Request + eventType string + event []byte + detected bool + configuring bool + wantErr string + expectedNs string + expectedRepo string + nsTemplate string + repoTemplate string + testData testclient.Data }{ { name: "non supported event", @@ -70,34 +72,49 @@ func TestConfigureRepository(t *testing.T) { testData: testclient.Data{}, }, { - name: "repo create event with no ns template", - event: repoCreateEvent, - eventType: "repository", - detected: true, - configuring: true, - wantErr: "", - expectedNs: "test-repo-pipelines", - testData: testclient.Data{}, + name: "repo create event with no ns template", + event: repoCreateEvent, + eventType: "repository", + detected: true, + configuring: true, + wantErr: "", + expectedNs: "test-repo-pipelines", + expectedRepo: "test-repo-repo-cr", + testData: testclient.Data{}, }, { - name: "repo create event with ns template", - event: repoCreateEvent, - eventType: "repository", - detected: true, - configuring: true, - wantErr: "", - expectedNs: "pac-test-repo-ci", - nsTemplate: "{{repo_owner}}-{{repo_name}}-ci", - testData: testclient.Data{}, + name: "repo create event with ns template", + event: repoCreateEvent, + eventType: "repository", + detected: true, + configuring: true, + wantErr: "", + expectedNs: "pac-test-repo-ci", + expectedRepo: "test-repo-repo-cr", + nsTemplate: "{{repo_owner}}-{{repo_name}}-ci", + testData: testclient.Data{}, }, { - name: "repo create event with ns already exist", - event: repoCreateEvent, - eventType: "repository", - detected: true, - configuring: true, - wantErr: "", - expectedNs: "test-repo-pipelines", + name: "repo create event with repo template", + event: repoCreateEvent, + eventType: "repository", + detected: true, + configuring: true, + wantErr: "", + expectedNs: "test-repo-pipelines", + expectedRepo: "pac-test-repo-cr", + repoTemplate: "{{repo_owner}}-{{repo_name}}-cr", + testData: testclient.Data{}, + }, + { + name: "repo create event with ns already exist", + event: repoCreateEvent, + eventType: "repository", + detected: true, + configuring: true, + wantErr: "", + expectedNs: "test-repo-pipelines", + expectedRepo: "test-repo-repo-cr", testData: testclient.Data{ Namespaces: []*v12.Namespace{ { @@ -127,8 +144,9 @@ func TestConfigureRepository(t *testing.T) { infoPac := &info.PacOpts{ Settings: settings.Settings{ - AutoConfigureNewGitHubRepo: true, - AutoConfigureRepoNamespaceTemplate: tt.nsTemplate, + AutoConfigureNewGitHubRepo: true, + AutoConfigureRepoNamespaceTemplate: tt.nsTemplate, + AutoConfigureRepoRepositoryTemplate: tt.repoTemplate, }, } detected, configuring, err := ConfigureRepository(ctx, run, req, string(tt.event), infoPac, logger) @@ -146,47 +164,76 @@ func TestConfigureRepository(t *testing.T) { assert.NilError(t, err) assert.Equal(t, ns.Name, tt.expectedNs) - repo, err := run.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(tt.expectedNs).Get(ctx, tt.expectedNs, v1.GetOptions{}) + repo, err := run.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(tt.expectedNs).Get(ctx, tt.expectedRepo, v1.GetOptions{}) assert.NilError(t, err) - assert.Equal(t, repo.Name, tt.expectedNs) + assert.Equal(t, repo.Name, tt.expectedRepo) } }) } } -func TestGetNamespace(t *testing.T) { +func TestGenerateNamespaceAndRepositoryName(t *testing.T) { tests := []struct { - name string - nsTemplate string - gitEvent *github.RepositoryEvent - want string + name string + nsTemplate string + repoTemplate string + gitEvent *github.RepositoryEvent + want string + wantRepo string }{ { - name: "no template", - nsTemplate: "", + name: "no template", + nsTemplate: "", + repoTemplate: "", gitEvent: &github.RepositoryEvent{ Repo: &github.Repository{ HTMLURL: github.Ptr("https://github.com/user/pac"), }, }, - want: "pac-pipelines", + want: "pac-pipelines", + wantRepo: "pac-repo-cr", }, { - name: "template", + name: "template", + nsTemplate: "{{repo_owner}}-{{repo_name}}-ci", + repoTemplate: "{{repo_owner}}-{{repo_name}}-repo-cr", + gitEvent: &github.RepositoryEvent{ + Repo: &github.Repository{ + HTMLURL: github.Ptr("https://github.com/user/pac"), + }, + }, + want: "user-pac-ci", + wantRepo: "user-pac-repo-cr", + }, + { + name: "empty repo template", nsTemplate: "{{repo_owner}}-{{repo_name}}-ci", gitEvent: &github.RepositoryEvent{ Repo: &github.Repository{ HTMLURL: github.Ptr("https://github.com/user/pac"), }, }, - want: "user-pac-ci", + want: "user-pac-ci", + wantRepo: "pac-repo-cr", + }, + { + name: "empty ns template", + repoTemplate: "{{repo_owner}}-{{repo_name}}-repo-cr", + gitEvent: &github.RepositoryEvent{ + Repo: &github.Repository{ + HTMLURL: github.Ptr("https://github.com/user/pac"), + }, + }, + want: "pac-pipelines", + wantRepo: "user-pac-repo-cr", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := generateNamespaceName(tt.nsTemplate, tt.gitEvent) + got, gotRepo, err := generateNamespaceAndRepositoryName(tt.nsTemplate, tt.repoTemplate, tt.gitEvent) assert.NilError(t, err) assert.Equal(t, got, tt.want) + assert.Equal(t, gotRepo, tt.wantRepo) }) } } From 633f025f97de57df0a73b21c111e395f6628f2ec Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Mon, 11 Aug 2025 13:49:35 +0200 Subject: [PATCH 3/6] fix: Allow contributors to trigger end-to-end tests - Modified the e2e workflow to include contributors - Ensured contributor pull requests trigger tests Signed-off-by: Chmouel Boudjnah --- .github/workflows/e2e.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index ac8f49e01..c4437c856 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -32,7 +32,7 @@ jobs: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && - contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.pull_request.author_association)) + contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR", "CONTRIBUTOR"]'), github.event.pull_request.author_association)) concurrency: group: ${{ github.workflow }}-${{ matrix.provider }}-${{ github.event.pull_request.number || github.ref_name }} cancel-in-progress: true From 39b60ba91499c3a4e5623146e3e9a0ecbf34b969 Mon Sep 17 00:00:00 2001 From: Gal Ben Haim Date: Wed, 6 Aug 2025 10:31:05 +0300 Subject: [PATCH 4/6] fix: Annotate PLR when started status is reported The use of the state label (which is mutable) for deciding when to report to the SCM that the PLR was started is flaky. It was seen that the reconciler get events about PLRs with unexpected value for the state label. For example, after the status is reported to the SCM, and the label value is patched to "started", after serval reconcile iterations the label had the "queued" value again. This can happen because of unsafe patching done by controllers (not just the PAC controllers) which reconciles PLRs. Introduce a new annotation for indicating the the status was reported to the SCM. By adding an annotation which is set once, we remove the risk that its value will get overwritten by other controllers (since maps are merged when patched, values are not getting removed unless explicitly defined in the patch - https://datatracker.ietf.org/doc/html/rfc7386#section-2) In addition, at the start of each reconcile loop, ensure that we operate on the latest version of the PLR and not using a stale value from the cache. Assisted-By: Cursor Signed-off-by: Gal Ben Haim --- pkg/apis/pipelinesascode/keys/keys.go | 75 ++++----- pkg/pipelineascode/pipelineascode.go | 17 ++ pkg/pipelineascode/pipelineascode_test.go | 9 ++ pkg/reconciler/reconciler.go | 37 ++++- pkg/reconciler/reconciler_test.go | 183 ++++++++++++++++++++++ 5 files changed, 280 insertions(+), 41 deletions(-) diff --git a/pkg/apis/pipelinesascode/keys/keys.go b/pkg/apis/pipelinesascode/keys/keys.go index f61b729bf..10febd501 100644 --- a/pkg/apis/pipelinesascode/keys/keys.go +++ b/pkg/apis/pipelinesascode/keys/keys.go @@ -23,43 +23,44 @@ import ( ) const ( - ControllerInfo = pipelinesascode.GroupName + "/controller-info" - Task = pipelinesascode.GroupName + "/task" - Pipeline = pipelinesascode.GroupName + "/pipeline" - URLOrg = pipelinesascode.GroupName + "/url-org" - URLRepository = pipelinesascode.GroupName + "/url-repository" - SHA = pipelinesascode.GroupName + "/sha" - Sender = pipelinesascode.GroupName + "/sender" - EventType = pipelinesascode.GroupName + "/event-type" - Branch = pipelinesascode.GroupName + "/branch" - SourceBranch = pipelinesascode.GroupName + "/source-branch" - Repository = pipelinesascode.GroupName + "/repository" - GitProvider = pipelinesascode.GroupName + "/git-provider" - State = pipelinesascode.GroupName + "/state" - ShaTitle = pipelinesascode.GroupName + "/sha-title" - ShaURL = pipelinesascode.GroupName + "/sha-url" - RepoURL = pipelinesascode.GroupName + "/repo-url" - SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url" - PullRequest = pipelinesascode.GroupName + "/pull-request" - InstallationID = pipelinesascode.GroupName + "/installation-id" - GHEURL = pipelinesascode.GroupName + "/ghe-url" - SourceProjectID = pipelinesascode.GroupName + "/source-project-id" - TargetProjectID = pipelinesascode.GroupName + "/target-project-id" - OriginalPRName = pipelinesascode.GroupName + "/original-prname" - GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret" - CheckRunID = pipelinesascode.GroupName + "/check-run-id" - OnEvent = pipelinesascode.GroupName + "/on-event" - OnComment = pipelinesascode.GroupName + "/on-comment" - OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch" - OnPathChange = pipelinesascode.GroupName + "/on-path-change" - OnLabel = pipelinesascode.GroupName + "/on-label" - OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore" - OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression" - TargetNamespace = pipelinesascode.GroupName + "/target-namespace" - MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs" - CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress" - LogURL = pipelinesascode.GroupName + "/log-url" - ExecutionOrder = pipelinesascode.GroupName + "/execution-order" + ControllerInfo = pipelinesascode.GroupName + "/controller-info" + Task = pipelinesascode.GroupName + "/task" + Pipeline = pipelinesascode.GroupName + "/pipeline" + URLOrg = pipelinesascode.GroupName + "/url-org" + URLRepository = pipelinesascode.GroupName + "/url-repository" + SHA = pipelinesascode.GroupName + "/sha" + Sender = pipelinesascode.GroupName + "/sender" + EventType = pipelinesascode.GroupName + "/event-type" + Branch = pipelinesascode.GroupName + "/branch" + SourceBranch = pipelinesascode.GroupName + "/source-branch" + Repository = pipelinesascode.GroupName + "/repository" + GitProvider = pipelinesascode.GroupName + "/git-provider" + State = pipelinesascode.GroupName + "/state" + ShaTitle = pipelinesascode.GroupName + "/sha-title" + ShaURL = pipelinesascode.GroupName + "/sha-url" + RepoURL = pipelinesascode.GroupName + "/repo-url" + SourceRepoURL = pipelinesascode.GroupName + "/source-repo-url" + PullRequest = pipelinesascode.GroupName + "/pull-request" + InstallationID = pipelinesascode.GroupName + "/installation-id" + GHEURL = pipelinesascode.GroupName + "/ghe-url" + SourceProjectID = pipelinesascode.GroupName + "/source-project-id" + TargetProjectID = pipelinesascode.GroupName + "/target-project-id" + OriginalPRName = pipelinesascode.GroupName + "/original-prname" + GitAuthSecret = pipelinesascode.GroupName + "/git-auth-secret" + CheckRunID = pipelinesascode.GroupName + "/check-run-id" + OnEvent = pipelinesascode.GroupName + "/on-event" + OnComment = pipelinesascode.GroupName + "/on-comment" + OnTargetBranch = pipelinesascode.GroupName + "/on-target-branch" + OnPathChange = pipelinesascode.GroupName + "/on-path-change" + OnLabel = pipelinesascode.GroupName + "/on-label" + OnPathChangeIgnore = pipelinesascode.GroupName + "/on-path-change-ignore" + OnCelExpression = pipelinesascode.GroupName + "/on-cel-expression" + TargetNamespace = pipelinesascode.GroupName + "/target-namespace" + MaxKeepRuns = pipelinesascode.GroupName + "/max-keep-runs" + CancelInProgress = pipelinesascode.GroupName + "/cancel-in-progress" + LogURL = pipelinesascode.GroupName + "/log-url" + ExecutionOrder = pipelinesascode.GroupName + "/execution-order" + SCMReportingPLRStarted = pipelinesascode.GroupName + "/scm-reporting-plr-started" // PublicGithubAPIURL default is "https://api.github.com" but it can be overridden by X-GitHub-Enterprise-Host header. PublicGithubAPIURL = "https://api.github.com" GithubApplicationID = "github-application-id" diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index 4a1bdcbe4..f108861cd 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -257,6 +257,9 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi whatPatching = "annotations.state and labels.state" patchAnnotations[keys.State] = kubeinteraction.StateQueued patchLabels[keys.State] = kubeinteraction.StateQueued + } else { + patchAnnotations[keys.SCMReportingPLRStarted] = "true" + whatPatching = fmt.Sprintf("annotation.%s", keys.SCMReportingPLRStarted) } if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil { @@ -278,6 +281,20 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi // unneeded SIGSEGV's return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err) } + currentReason := "" + if len(pr.Status.GetConditions()) > 0 { + currentReason = pr.Status.GetConditions()[0].GetReason() + } + + p.logger.Infof("PipelineRun %s/%s patched successfully - Spec.Status: %s, State annotation: '%s', SCMReportingPLRStarted annotation: '%s', Status reason: '%s', Git provider status: '%s', Patched: %s", + pr.GetNamespace(), + pr.GetName(), + pr.Spec.Status, + pr.GetAnnotations()[keys.State], + pr.GetAnnotations()[keys.SCMReportingPLRStarted], + currentReason, + status.Status, + whatPatching) } return pr, nil diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 9b9f23e5c..3c752aed3 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -722,6 +722,15 @@ func TestRun(t *testing.T) { state, ok := pr.GetAnnotations()[keys.State] assert.Assert(t, ok, "State hasn't been set on PR", state) assert.Equal(t, state, kubeinteraction.StateQueued) + + // When PipelineRun is queued, SCMReportingPLRStarted should not be set + _, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, !scmStartedExists, "SCMReportingPLRStarted should not be set for queued PipelineRuns") + } else { + // When PipelineRun is not queued, SCMReportingPLRStarted should be set to "true" + scmStarted, scmStartedExists := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, scmStartedExists, "SCMReportingPLRStarted should be set for non-queued PipelineRuns") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") } } } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 95aba5dc0..b75f64095 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -54,6 +54,21 @@ var ( func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun) pkgreconciler.Event { ctx = info.StoreNS(ctx, system.Namespace()) logger := logging.FromContext(ctx).With("namespace", pr.GetNamespace()) + + logger.Debugf("reconciling pipelineRun %s/%s", pr.GetNamespace(), pr.GetName()) + + // Get fresh PipelineRun from API server to avoid informer cache staleness + freshPR, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) + if err != nil { + logger.Errorf("failed to get fresh pipelineRun %s/%s: %v", pr.GetNamespace(), pr.GetName(), err) + return fmt.Errorf("cannot get fresh PipelineRun: %w", err) + } + + if freshPR.GetResourceVersion() != pr.GetResourceVersion() { + logger.Debugf("using fresh pipelineRun data (cached version %s vs fresh version %s)", pr.GetResourceVersion(), freshPR.GetResourceVersion()) + pr = freshPR + } + // if pipelineRun is in completed or failed state then return state, exist := pr.GetAnnotations()[keys.State] if exist && (state == kubeinteraction.StateCompleted || state == kubeinteraction.StateFailed) { @@ -69,7 +84,12 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun // another controller outside PaC). To maintain consistency between the PipelineRun // status and the Git provider status, we update both the PipelineRun resource and // the corresponding status on the Git provider here. - if reason == string(tektonv1.PipelineRunReasonRunning) && state == kubeinteraction.StateQueued { + scmReportingPLRStarted, exist := pr.GetAnnotations()[keys.SCMReportingPLRStarted] + startReported := exist && scmReportingPLRStarted == "true" + logger.Debugf("pipelineRun %s/%s scmReportingPLRStarted=%v, exist=%v", pr.GetNamespace(), pr.GetName(), startReported, exist) + + if reason == string(tektonv1.PipelineRunReasonRunning) && !startReported { + logger.Infof("pipelineRun %s/%s is running but not yet reported to provider, updating status", pr.GetNamespace(), pr.GetName()) repoName := pr.GetAnnotations()[keys.Repository] repo, err := r.repoLister.Repositories(pr.Namespace).Get(repoName) if err != nil { @@ -77,6 +97,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun } return r.updatePipelineRunToInProgress(ctx, logger, repo, pr) } + logger.Debugf("pipelineRun %s/%s condition not met: reason='%s', startReported=%v", pr.GetNamespace(), pr.GetName(), reason, startReported) // if its a GitHub App pipelineRun PR then process only if check run id is added otherwise wait if _, ok := pr.Annotations[keys.InstallationID]; ok { @@ -343,16 +364,24 @@ func (r *Reconciler) updatePipelineRunToInProgress(ctx context.Context, logger * } 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) + annotations := map[string]string{ + keys.State: state, + } + if state == kubeinteraction.StateStarted { + annotations[keys.SCMReportingPLRStarted] = "true" + } + mergePatch := map[string]any{ "metadata": map[string]any{ "labels": map[string]string{ keys.State: state, }, - "annotations": map[string]string{ - keys.State: state, - }, + "annotations": annotations, }, } + // if state is started then remove pipelineRun pending status if state == kubeinteraction.StateStarted { mergePatch["spec"] = map[string]any{ diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index e3c4001a1..b001de831 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -299,6 +299,189 @@ func TestUpdatePipelineRunState(t *testing.T) { assert.Equal(t, updatedPR.Annotations[keys.State], tt.state) assert.Equal(t, updatedPR.Spec.Status, tektonv1.PipelineRunSpecStatus("")) + + // Test SCMReportingPLRStarted annotation for started state + if tt.state == kubeinteraction.StateStarted { + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, exists, "SCMReportingPLRStarted annotation should exist when state is started") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") + } else { + _, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, !exists, "SCMReportingPLRStarted annotation should not exist for non-started states") + } + }) + } +} + +func TestReconcileKind_SCMReportingLogic(t *testing.T) { + observer, _ := zapobserver.New(zap.InfoLevel) + _ = zap.New(observer).Sugar() + + tests := []struct { + name string + pipelineRun *tektonv1.PipelineRun + shouldCallUpdateToInProgress bool + description string + }{ + { + name: "Running reason without SCMReportingPLRStarted - should call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateQueued, + keys.Repository: "test-repo", + }, + }, + Spec: tektonv1.PipelineRunSpec{}, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonRunning), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: true, + description: "PipelineRun is Running but no SCM reporting done yet", + }, + { + name: "Running reason with SCMReportingPLRStarted=true - should NOT call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateStarted, + keys.Repository: "test-repo", + keys.SCMReportingPLRStarted: "true", + }, + }, + Spec: tektonv1.PipelineRunSpec{}, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonRunning), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: false, + description: "PipelineRun is Running and SCM reporting already done", + }, + { + name: "Non-Running reason - should NOT call updatePipelineRunToInProgress", + pipelineRun: &tektonv1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "test-pr", + Annotations: map[string]string{ + keys.State: kubeinteraction.StateQueued, + keys.Repository: "test-repo", + }, + }, + Spec: tektonv1.PipelineRunSpec{ + Status: tektonv1.PipelineRunSpecStatusPending, + }, + Status: tektonv1.PipelineRunStatus{ + Status: knativeduckv1.Status{ + Conditions: knativeduckv1.Conditions{ + { + Type: knativeapi.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: string(tektonv1.PipelineRunReasonPending), + }, + }, + }, + }, + }, + shouldCallUpdateToInProgress: false, + description: "PipelineRun is not in Running state", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + + testRepo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + Namespace: tt.pipelineRun.GetNamespace(), + }, + Spec: v1alpha1.RepositorySpec{ + URL: randomURL, + }, + } + + testData := testclient.Data{ + Repositories: []*v1alpha1.Repository{testRepo}, + PipelineRuns: []*tektonv1.PipelineRun{tt.pipelineRun}, + } + stdata, informers := testclient.SeedTestData(t, ctx, testData) + + // Track if updatePipelineRunToInProgress was called by checking state changes + originalState := tt.pipelineRun.GetAnnotations()[keys.State] + + r := &Reconciler{ + repoLister: informers.Repository.Lister(), + run: ¶ms.Run{ + Clients: clients.Clients{ + Tekton: stdata.Pipeline, + }, + Info: info.Info{ + Pac: &info.PacOpts{ + Settings: settings.Settings{}, + }, + }, + }, + } + + err := r.ReconcileKind(ctx, tt.pipelineRun) + + // For test cases that should call updatePipelineRunToInProgress, + // we expect no error and the state should be updated + if tt.shouldCallUpdateToInProgress { + assert.NilError(t, err, tt.description) + + // Get the updated PipelineRun to check if state was changed + updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{}) + assert.NilError(t, getErr) + + // Should have been updated to started state with SCMReportingPLRStarted annotation + assert.Equal(t, updatedPR.GetAnnotations()[keys.State], kubeinteraction.StateStarted) + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + assert.Assert(t, exists, "SCMReportingPLRStarted should be set") + assert.Equal(t, scmStarted, "true") + } else { + // For cases that should NOT call updatePipelineRunToInProgress, + // the state should remain unchanged + updatedPR, getErr := stdata.Pipeline.TektonV1().PipelineRuns(tt.pipelineRun.GetNamespace()).Get(ctx, tt.pipelineRun.GetName(), metav1.GetOptions{}) + assert.NilError(t, getErr) + + // State should be unchanged + assert.Equal(t, updatedPR.GetAnnotations()[keys.State], originalState, tt.description) + + // Check SCMReportingPLRStarted annotation based on original state + scmStarted, exists := updatedPR.GetAnnotations()[keys.SCMReportingPLRStarted] + if originalState == kubeinteraction.StateStarted { + // If original state was already 'started', SCMReportingPLRStarted should exist and be "true" + assert.Assert(t, exists, "SCMReportingPLRStarted should exist when original state is started") + assert.Equal(t, scmStarted, "true", "SCMReportingPLRStarted should be 'true'") + } else { + // If original state was not 'started', SCMReportingPLRStarted should not exist + assert.Assert(t, !exists, "SCMReportingPLRStarted should not exist when original state is not started") + } + } }) } } From d9eb22ef65bde4440ac5c5be99823d4475729bf7 Mon Sep 17 00:00:00 2001 From: Gal Ben Haim Date: Thu, 7 Aug 2025 09:55:28 +0300 Subject: [PATCH 5/6] fix: Set state only after status report Set the state annotation and labels in pipelineascode only after the state was reported to the SCM. This is achieved by patching the PLR with the state label/annotation after it was created. This change is needed in order to avoid a case where the watcher will report a status before PAC. With this change the issue can't happen, since the watcher only reconciles PLRs that has the state annotation, so the first reconcile of the PLRs will happen only after PAC already sent the initial status to the SCM. In addition, simplify the condition which checks if the "queued" state annotation/label should be set. We can only check if the PLR is pending, it doesn't matter if the reason is because of PAC's concurrency control or an external controller. Signed-off-by: Gal Ben Haim --- pkg/kubeinteraction/labels.go | 1 - pkg/pipelineascode/pipelineascode.go | 23 ++++++++++++----------- pkg/pipelineascode/pipelineascode_test.go | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/kubeinteraction/labels.go b/pkg/kubeinteraction/labels.go index 846d6a2e0..50fcf2f85 100644 --- a/pkg/kubeinteraction/labels.go +++ b/pkg/kubeinteraction/labels.go @@ -55,7 +55,6 @@ func AddLabelsAndAnnotations(event *info.Event, pipelineRun *tektonv1.PipelineRu keys.SourceBranch: event.HeadBranch, keys.Repository: repo.GetName(), keys.GitProvider: providerConfig.Name, - keys.State: StateStarted, keys.ControllerInfo: fmt.Sprintf(`{"name":"%s","configmap":"%s","secret":"%s", "gRepo": "%s"}`, paramsinfo.Controller.Name, paramsinfo.Controller.Configmap, paramsinfo.Controller.Secret, paramsinfo.Controller.GlobalRepository), } diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index f108861cd..c0384b9f7 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -176,14 +176,10 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi p.logger.Errorf("Error adding labels/annotations to PipelineRun '%s' in namespace '%s': %v", match.PipelineRun.GetName(), match.Repo.GetNamespace(), err) } - // if concurrency is defined then start the pipelineRun in pending state and - // state as queued + // if concurrency is defined then start the pipelineRun in pending state if match.Repo.Spec.ConcurrencyLimit != nil && *match.Repo.Spec.ConcurrencyLimit != 0 { // pending status match.PipelineRun.Spec.Status = tektonv1.PipelineRunSpecStatusPending - // pac state as queued - match.PipelineRun.Labels[keys.State] = kubeinteraction.StateQueued - match.PipelineRun.Annotations[keys.State] = kubeinteraction.StateQueued } // Create the actual pipelineRun @@ -240,26 +236,31 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi OriginalPipelineRunName: pr.GetAnnotations()[keys.OriginalPRName], } + // Patch the pipelineRun with the appropriate annotations and labels. + // Set the state so the watcher will continue with reconciling the pipelineRun + // The watcher reconciles only pipelineRuns that has the state annotation. patchAnnotations := map[string]string{} patchLabels := map[string]string{} whatPatching := "" // if pipelineRun is in pending state then report status as queued + // The pipelineRun can be pending because of PAC's concurrency limit or because of an external mutatingwebhook if pr.Spec.Status == tektonv1.PipelineRunSpecStatusPending { status.Status = queuedStatus if status.Text, err = mt.MakeTemplate(p.vcx.GetTemplate(provider.QueueingPipelineType)); err != nil { return nil, fmt.Errorf("cannot create message template: %w", err) } - // If the PipelineRun is in the "queued" state, add the appropriate label and annotation. - // These are later used by the watcher to determine whether the PipelineRun status - // should be reported back to the Git provider. We do add the `state` annotations and label when - // concurrency is enabled but this would happen when PipelineRun's status has been changed by - // the other controller and PaC is not aware of that change. whatPatching = "annotations.state and labels.state" patchAnnotations[keys.State] = kubeinteraction.StateQueued patchLabels[keys.State] = kubeinteraction.StateQueued } else { + // Mark that the start will be reported to the Git provider patchAnnotations[keys.SCMReportingPLRStarted] = "true" - whatPatching = fmt.Sprintf("annotation.%s", keys.SCMReportingPLRStarted) + patchAnnotations[keys.State] = kubeinteraction.StateStarted + patchLabels[keys.State] = kubeinteraction.StateStarted + whatPatching = fmt.Sprintf( + "annotation.%s and annotations.state and labels.state", + keys.SCMReportingPLRStarted, + ) } if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil { diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 3c752aed3..17ed9ba2c 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -718,7 +718,7 @@ func TestRun(t *testing.T) { secretName, ok := pr.GetAnnotations()[keys.GitAuthSecret] assert.Assert(t, ok, "Cannot find secret %s on annotations", secretName) } - if tt.concurrencyLimit > 0 || pr.Spec.Status == pipelinev1.PipelineRunSpecStatusPending { + if pr.Spec.Status == pipelinev1.PipelineRunSpecStatusPending { state, ok := pr.GetAnnotations()[keys.State] assert.Assert(t, ok, "State hasn't been set on PR", state) assert.Equal(t, state, kubeinteraction.StateQueued) From d34206bed85d7bf4f6ab92091bb08f881f4a5f3b Mon Sep 17 00:00:00 2001 From: Gal Ben Haim Date: Thu, 7 Aug 2025 15:43:05 +0300 Subject: [PATCH 6/6] fix: Skip reconciliation of stale PLRs Instead of running the reconcile loop on the fresh pipeline, skip it and let the next iterations to reconcile it. Signed-off-by: Gal Ben Haim --- pkg/reconciler/reconciler.go | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index b75f64095..58493f289 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -57,16 +57,15 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun logger.Debugf("reconciling pipelineRun %s/%s", pr.GetNamespace(), pr.GetName()) - // Get fresh PipelineRun from API server to avoid informer cache staleness - freshPR, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) + // make sure we have the latest pipelinerun to reconcile, since there is something updating at the same time + lpr, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) if err != nil { - logger.Errorf("failed to get fresh pipelineRun %s/%s: %v", pr.GetNamespace(), pr.GetName(), err) - return fmt.Errorf("cannot get fresh PipelineRun: %w", err) + return fmt.Errorf("cannot get pipelineRun: %w", err) } - if freshPR.GetResourceVersion() != pr.GetResourceVersion() { - logger.Debugf("using fresh pipelineRun data (cached version %s vs fresh version %s)", pr.GetResourceVersion(), freshPR.GetResourceVersion()) - pr = freshPR + if lpr.GetResourceVersion() != pr.GetResourceVersion() { + logger.Debugf("Skipping reconciliation, pipelineRun was updated (cached version %s vs fresh version %s)", pr.GetResourceVersion(), lpr.GetResourceVersion()) + return nil } // if pipelineRun is in completed or failed state then return @@ -116,16 +115,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, pr *tektonv1.PipelineRun return nil } - // make sure we have the latest pipelinerun to reconcile, since there is something updating at the same time - lpr, err := r.run.Clients.Tekton.TektonV1().PipelineRuns(pr.GetNamespace()).Get(ctx, pr.GetName(), metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("cannot get pipelineRun: %w", err) - } - - if lpr.GetResourceVersion() != pr.GetResourceVersion() { - return nil - } - // If we have a controllerInfo annotation, then we need to get the // configmap configuration for it //