From 5a43e25af2584e590ee1e07801849aea5b9b0e05 Mon Sep 17 00:00:00 2001 From: Akshay Pant Date: Mon, 3 Nov 2025 16:05:01 +0530 Subject: [PATCH] feat: add skip CI command support Implement support for skip CI commands in commit messages to allow users to skip PipelineRun execution. Supports [skip ci], [ci skip], [skip tkn], and [tkn skip] commands. When a skip command is detected in the commit message, PipelineRun execution is skipped. However, GitOps commands (/test, /retest, etc.) will still trigger PipelineRuns regardless of the skip command, allowing users to manually trigger CI when needed. Jira: https://issues.redhat.com/browse/SRVKP-8933 Signed-off-by: Akshay Pant --- README.md | 1 + docs/content/docs/guide/matchingevents.md | 91 +++ pkg/adapter/sinker.go | 72 ++ pkg/adapter/sinker_test.go | 630 +++++++++++++++ pkg/params/info/events.go | 7 + pkg/pipelineascode/client_setup.go | 98 +++ pkg/pipelineascode/client_setup_test.go | 733 ++++++++++++++++++ pkg/pipelineascode/match.go | 76 +- pkg/pipelineascode/match_test.go | 22 - pkg/pipelineascode/pipelineascode.go | 10 + pkg/provider/bitbucketcloud/bitbucket.go | 1 + .../bitbucketdatacenter.go | 1 + pkg/provider/gitea/gitea.go | 1 + pkg/provider/github/github.go | 1 + pkg/provider/github/github_test.go | 65 +- pkg/provider/github/parse_payload.go | 23 - pkg/provider/github/parse_payload_test.go | 89 +-- pkg/provider/gitlab/gitlab.go | 1 + pkg/provider/provider.go | 10 + pkg/provider/provider_test.go | 96 +++ pkg/test/provider/testwebvcs.go | 15 +- test/github_skip_ci_test.go | 134 ++++ 22 files changed, 1980 insertions(+), 197 deletions(-) create mode 100644 pkg/adapter/sinker_test.go create mode 100644 pkg/pipelineascode/client_setup.go create mode 100644 pkg/pipelineascode/client_setup_test.go create mode 100644 test/github_skip_ci_test.go diff --git a/README.md b/README.md index b8b8a5387a..2d1b88e8a5 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,7 @@ Before getting started with Pipelines-as-Code, ensure you have: - **Multi-provider support**: Works with GitHub (via GitHub App & Webhook), GitLab, Gitea, Bitbucket Data Center & Cloud via webhooks. - **Annotation-driven workflows**: Target specific events, branches, or CEL expressions and gate untrusted PRs with `/ok-to-test` and `OWNERS`; see [Running the PipelineRun](https://pipelinesascode.com/docs/guide/running/). - **ChatOps style control**: `/test`, `/retest`, `/cancel`, and branch or tag selectors let you rerun or stop PipelineRuns from PR comments or commit messages; see [GitOps Commands](https://pipelinesascode.com/docs/guide/gitops_commands/). +- **Skip CI support**: Use `[skip ci]`, `[ci skip]`, `[skip tkn]`, or `[tkn skip]` in commit messages to skip automatic PipelineRun execution for documentation updates or minor changes; GitOps commands can still override and trigger runs manually; see [Skip CI Commands](https://pipelinesascode.com/docs/guide/gitops_commands/#skip-ci-commands). - **Feedback**: GitHub Checks capture per-task timing, log snippets, and optional error annotations while redacting secrets; see [PipelineRun status](https://pipelinesascode.com/docs/guide/statuses/). - **Inline resolution**: The resolver bundles `.tekton/` resources, inlines remote tasks from Artifact Hub or Tekton Hub, and validates YAML before cluster submission; see [Resolver](https://pipelinesascode.com/docs/guide/resolver/). - **CLI**: `tkn pac` bootstraps installs, manages Repository CRDs, inspects logs, and resolves runs locally; see the [CLI guide](https://pipelinesascode.com/docs/guide/cli/). diff --git a/docs/content/docs/guide/matchingevents.md b/docs/content/docs/guide/matchingevents.md index 10b69f2a11..5299ba2003 100644 --- a/docs/content/docs/guide/matchingevents.md +++ b/docs/content/docs/guide/matchingevents.md @@ -482,3 +482,94 @@ main and the branch called `release,nightly` you can do this: ```yaml pipelinesascode.tekton.dev/on-target-branch: [main, release,nightly] ``` + +## Skip CI Commands + +Pipelines-as-Code supports skip commands in commit messages that allow you to skip +PipelineRun execution for specific commits. This is useful when making documentation +changes, minor fixes, or work-in-progress commits where running the full CI pipeline +is unnecessary. + +### Supported Skip Commands + +You can include any of the following commands anywhere in your commit message to skip +PipelineRun execution: + +* `[skip ci]` - Skip continuous integration +* `[ci skip]` - Alternative format for skipping CI +* `[skip tkn]` - Skip Tekton PipelineRuns +* `[tkn skip]` - Alternative format for skipping Tekton + +**Note:** Skip commands are **case-sensitive** and must be in lowercase with brackets. + +### Example Usage + +```text +docs: update README with installation instructions [skip ci] +``` + +or + +```text +WIP: refactor authentication module + +This is still in progress and not ready for testing yet. + +[ci skip] +``` + +### How Skip Commands Work + +When a commit message contains a skip command: + +1. **Pull Requests**: No PipelineRuns will be created when the PR is opened or updated and HEAD commit contains skip command. +2. **Push Events**: No PipelineRuns will be created when pushing to a branch with that commit message + +### GitOps Commands Override Skip CI + +**Important:** Skip CI commands can be overridden by using GitOps commands. Even if +a commit contains a skip command like `[skip ci]`, you can still manually trigger +PipelineRuns using: + +* `/test` - Trigger all matching PipelineRuns +* `/test ` - Trigger a specific PipelineRun +* `/retest` - Retrigger failed PipelineRuns +* `/retest ` - Retrigger a specific PipelineRun +* `/ok-to-test` - Allow running CI for external contributors +* `/custom-comment` - Trigger PipelineRun having on-comment annotation + +This allows you to skip automatic CI execution while still maintaining the ability +to manually trigger builds when needed. + +### Example: Skipping CI Then Manually Triggering + +```bash +# Initial commit with skip command +git commit -m "docs: update contributing guide [skip ci]" +git push origin my-feature-branch +# No PipelineRuns are created automatically + +# Later, you can manually trigger CI by commenting on the PR: +# /test +# This will create PipelineRuns despite the [skip ci] command +``` + +### Examples of When to Use Skip Commands + +Skip commands are useful for: + +* Documentation-only changes +* README updates +* Comment or formatting changes +* Work-in-progress commits +* Minor typo fixes +* Configuration file updates that don't affect code + +### Examples of When NOT to Use Skip Commands + +Avoid using skip commands for: + +* Code changes that affect functionality +* Changes to CI/CD pipeline definitions +* Dependency updates +* Any changes that should be tested before merging diff --git a/pkg/adapter/sinker.go b/pkg/adapter/sinker.go index f52746f47b..eff164d867 100644 --- a/pkg/adapter/sinker.go +++ b/pkg/adapter/sinker.go @@ -3,10 +3,12 @@ package adapter import ( "bytes" "context" + "fmt" "net/http" "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/matcher" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode" @@ -69,8 +71,78 @@ func (s *sinker) processEvent(ctx context.Context, request *http.Request) error if err := s.processEventPayload(ctx, request); err != nil { return err } + + // For ALL events: Setup authenticated client early (including token scoping) + // This centralizes client setup and token scoping in one place for all event types + repo, err := s.findMatchingRepository(ctx) + if err != nil { + // Continue with normal flow - repository matching will be handled in matchRepoPR + s.logger.Debugf("Could not find matching repository for early client setup: %v", err) + } else { + // We found the repository, now setup client with token scoping + // If setup fails here, it's a configuration error and we should fail fast + if err := s.setupClient(ctx, repo); err != nil { + return fmt.Errorf("client setup failed: %w", err) + } + s.logger.Debugf("Client setup completed early in sinker for event type: %s", s.event.EventType) + } + + // For PUSH events: commit message is already in event.SHATitle from the webhook payload + // We can check immediately without any API calls or repository lookups + if s.event.EventType == "push" && provider.SkipCI(s.event.SHATitle) { + s.logger.Infof("CI skipped for push event: commit %s contains skip command in message", s.event.SHA) + return nil + } + + // For PULL REQUEST events: commit message needs to be fetched via API + // Get commit info for skip-CI detection (only if we successfully set up client above) + if s.event.EventType == "pull_request" && repo != nil { + // Get commit info (including commit message) via API + if err := s.vcx.GetCommitInfo(ctx, s.event); err != nil { + return fmt.Errorf("could not get commit info: %w", err) + } + // Check for skip-ci commands in pull request events + if s.event.HasSkipCommand { + s.logger.Infof("CI skipped for pull request event: commit %s contains skip command in message", s.event.SHA) + return nil + } + } } p := pipelineascode.NewPacs(s.event, s.vcx, s.run, s.pacInfo, s.kint, s.logger, s.globalRepo) return p.Run(ctx) } + +// findMatchingRepository finds the Repository CR that matches the event. +// This is a lightweight lookup to get credentials for early skip-ci checks. +// Uses the canonical matcher implementation to avoid code duplication. +func (s *sinker) findMatchingRepository(ctx context.Context) (*v1alpha1.Repository, error) { + // Use canonical matcher to find repository (empty string searches all namespaces) + repo, err := matcher.MatchEventURLRepo(ctx, s.run, s.event, "") + if err != nil { + return nil, fmt.Errorf("failed to match repository: %w", err) + } + if repo == nil { + return nil, fmt.Errorf("no repository found matching URL: %s", s.event.URL) + } + + return repo, nil +} + +// setupClient sets up the authenticated client with token scoping for ALL event types. +// This is the primary location where client setup and GitHub App token scoping happens. +// Centralizing this here ensures consistent behavior across all events and enables early +// optimizations like skip-CI detection before expensive processing. +func (s *sinker) setupClient(ctx context.Context, repo *v1alpha1.Repository) error { + return pipelineascode.SetupAuthenticatedClient( + ctx, + s.vcx, + s.kint, + s.run, + s.event, + repo, + s.globalRepo, + s.pacInfo, + s.logger, + ) +} diff --git a/pkg/adapter/sinker_test.go b/pkg/adapter/sinker_test.go new file mode 100644 index 0000000000..b1be6eea01 --- /dev/null +++ b/pkg/adapter/sinker_test.go @@ -0,0 +1,630 @@ +package adapter + +import ( + "context" + "testing" + + "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/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/params/settings" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + testprovider "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider" + testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository" + "gotest.tools/v3/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + rtesting "knative.dev/pkg/reconciler/testing" +) + +// setupTestData creates common test data for sinker tests. +func setupTestData(t *testing.T, repos []*v1alpha1.Repository) *params.Run { + t.Helper() + + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + + tdata := testclient.Data{ + Namespaces: []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelines-as-code", + }, + }, + }, + Repositories: repos, + Secret: []*corev1.Secret{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "provider.token": []byte("test-token"), + "webhook.secret": []byte("webhook-secret"), + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: info.DefaultPipelinesAscodeSecretName, + Namespace: "pipelines-as-code", + }, + Data: map[string][]byte{ + "webhook.secret": []byte("controller-webhook-secret"), + }, + }, + }, + } + + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + + return ¶ms.Run{ + Clients: clients.Clients{ + Kube: stdata.Kube, + PipelineAsCode: stdata.PipelineAsCode, + Log: log, + }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: info.DefaultPipelinesAscodeSecretName, + }, + Kube: &info.KubeOpts{ + Namespace: "pipelines-as-code", + }, + }, + } +} + +// TestSetupClient_GitHubAppVsOther tests the different code paths for GitHub Apps vs other providers. +func TestSetupClient_GitHubAppVsOther(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + installationID int64 + hasGitProvider bool + extraReposConfigured bool + extraRepoInstallIDs map[string]int64 + wantErr bool + wantRepositoryIDsCount int + }{ + { + name: "GitHub App should use controller secret", + installationID: 12345, + hasGitProvider: false, + extraReposConfigured: false, + wantErr: false, + wantRepositoryIDsCount: 0, // No extra repos + }, + { + name: "GitHub App with extra repos - IDs should be populated", + installationID: 12345, + hasGitProvider: false, + extraReposConfigured: true, + extraRepoInstallIDs: map[string]int64{ + "another/one": 789, + "andanother/two": 10112, + }, + wantErr: false, + wantRepositoryIDsCount: 2, // Should have 2 extra repo IDs + }, + { + name: "Non-GitHub App requires git_provider", + installationID: 0, + hasGitProvider: false, + wantErr: true, + wantRepositoryIDsCount: 0, + }, + { + name: "Non-GitHub App with git_provider succeeds", + installationID: 0, + hasGitProvider: true, + wantErr: false, + wantRepositoryIDsCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + + // Create test repository + repo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "test-repo", + URL: "https://github.com/test/repo", + InstallNamespace: "default", + }) + + // Conditionally add git_provider + if tt.hasGitProvider { + repo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + Secret: &v1alpha1.Secret{ + Name: "test-secret", + Key: "token", + }, + WebhookSecret: &v1alpha1.Secret{ + Name: "test-secret", + Key: "webhook.secret", + }, + } + } + + // Setup extra repos if configured + extraRepos := []*v1alpha1.Repository{} + if tt.extraReposConfigured { + repo.Spec.Settings = &v1alpha1.Settings{ + GithubAppTokenScopeRepos: []string{}, + } + for repoName := range tt.extraRepoInstallIDs { + repo.Spec.Settings.GithubAppTokenScopeRepos = append( + repo.Spec.Settings.GithubAppTokenScopeRepos, + repoName, + ) + // Create matching repository CRs for extra repos + extraRepo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: repoName, + URL: "https://github.com/" + repoName, + InstallNamespace: "default", + }) + extraRepos = append(extraRepos, extraRepo) + } + } + + // Create test data with all repositories + allRepos := append([]*v1alpha1.Repository{repo}, extraRepos...) + run := setupTestData(t, allRepos) + + // Create a tracking provider to verify behavior + trackingProvider := &trackingProviderImpl{ + TestProviderImp: testprovider.TestProviderImp{AllowIT: true}, + createTokenCalled: false, + repositoryIDs: []int64{}, + extraRepoInstallIDs: tt.extraRepoInstallIDs, + } + trackingProvider.SetLogger(log) + + kint := &kubeinteraction.Interaction{ + Run: run, + } + + s := &sinker{ + run: run, + vcx: trackingProvider, + kint: kint, + event: &info.Event{ + InstallationID: tt.installationID, + EventType: "pull_request", + Provider: &info.Provider{ + URL: "https://github.com", + }, + }, + logger: log, + pacInfo: &info.PacOpts{ + Settings: settings.Settings{ + // Don't set SecretGHAppRepoScoped for repo-level config + // It's only used for global config with SecretGhAppTokenScopedExtraRepos + }, + }, + } + + // Call setupClient + err := s.setupClient(ctx, repo) + + // Verify expectations + if tt.wantErr { + assert.Assert(t, err != nil, "expected error but got nil") + } else { + assert.NilError(t, err, "unexpected error: %v", err) + } + + // For GitHub Apps with extra repos, verify CreateToken was called + // and repository IDs were populated + if tt.extraReposConfigured && !tt.wantErr { + assert.Assert(t, trackingProvider.createTokenCalled, + "CreateToken should have been called for extra repos") + + // Verify all expected repo IDs are present + for repoName, expectedID := range tt.extraRepoInstallIDs { + found := false + for _, id := range trackingProvider.repositoryIDs { + if id == expectedID { + found = true + break + } + } + assert.Assert(t, found, + "Repository ID %d for %s not found in provider.RepositoryIDs: %v", + expectedID, repoName, trackingProvider.repositoryIDs) + } + + assert.Equal(t, len(trackingProvider.repositoryIDs), tt.wantRepositoryIDsCount, + "Expected %d repository IDs, got %d: %v", + tt.wantRepositoryIDsCount, len(trackingProvider.repositoryIDs), + trackingProvider.repositoryIDs) + } + }) + } +} + +// trackingProviderImpl wraps TestProviderImp to track CreateToken calls and repository IDs. +type trackingProviderImpl struct { + testprovider.TestProviderImp + createTokenCalled bool + repositoryIDs []int64 + extraRepoInstallIDs map[string]int64 +} + +func (t *trackingProviderImpl) CreateToken(_ context.Context, repositories []string, _ *info.Event) (string, error) { + t.createTokenCalled = true + // Simulate adding repository IDs like the real CreateToken does + for _, repo := range repositories { + if id, ok := t.extraRepoInstallIDs[repo]; ok { + t.repositoryIDs = append(t.repositoryIDs, id) + } + } + return "fake-token", nil +} + +// TestGetCommitInfoError tests that GetCommitInfo errors work correctly with test provider. +// This test was moved from pkg/pipelineascode/match_test.go after refactoring +// GetCommitInfo to be called earlier in sinker.go processEvent() for PR events. +// Since processEvent is private, this test verifies the test provider behavior. +func TestGetCommitInfoError(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + tests := []struct { + name string + failGetCommitInfo bool + commitInfoErrorMsg string + wantErr bool + wantErrContains string + }{ + { + name: "GetCommitInfo succeeds", + failGetCommitInfo: false, + wantErr: false, + }, + { + name: "GetCommitInfo fails with custom message", + failGetCommitInfo: true, + commitInfoErrorMsg: "commit not found", + wantErr: true, + wantErrContains: "commit not found", + }, + { + name: "GetCommitInfo fails with default message", + failGetCommitInfo: true, + wantErr: true, + wantErrContains: "failed to get commit info", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + provider := &testprovider.TestProviderImp{ + FailGetCommitInfo: tt.failGetCommitInfo, + CommitInfoErrorMsg: tt.commitInfoErrorMsg, + } + + event := &info.Event{} + err := provider.GetCommitInfo(ctx, event) + + if tt.wantErr { + assert.Assert(t, err != nil) + assert.ErrorContains(t, err, tt.wantErrContains) + } else { + assert.NilError(t, err) + } + }) + } +} + +func TestFindMatchingRepository(t *testing.T) { + t.Parallel() + + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + + repo1 := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "repo-1", + URL: "https://github.com/test/repo1", + InstallNamespace: "default", + }) + + repo2 := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "repo-2", + URL: "https://github.com/test/repo2", + InstallNamespace: "default", + }) + + tdata := testclient.Data{ + Repositories: []*v1alpha1.Repository{repo1, repo2}, + } + + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + + tests := []struct { + name string + eventURL string + wantErr bool + wantRepo string + }{ + { + name: "find matching repository", + eventURL: "https://github.com/test/repo1", + wantErr: false, + wantRepo: "repo-1", + }, + { + name: "no matching repository", + eventURL: "https://github.com/test/unknown", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + s := &sinker{ + run: ¶ms.Run{ + Clients: clients.Clients{ + PipelineAsCode: stdata.PipelineAsCode, + }, + }, + event: &info.Event{ + URL: tt.eventURL, + }, + logger: log, + } + + repo, err := s.findMatchingRepository(ctx) + + if tt.wantErr { + assert.Assert(t, err != nil) + } else { + assert.NilError(t, err) + assert.Equal(t, tt.wantRepo, repo.Name) + } + }) + } +} + +// TestProcessEvent_SkipCI_PushEvent tests the skip-CI logic for push events. +// This tests that the SkipCI check works correctly for push events where the +// commit message is available directly in the webhook payload (event.SHATitle). +func TestProcessEvent_SkipCI_PushEvent(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + commitMessage string + shouldSkip bool + }{ + { + name: "push with [skip ci] should skip", + commitMessage: "fix: bug [skip ci]", + shouldSkip: true, + }, + { + name: "push with [ci skip] should skip", + commitMessage: "chore: update [ci skip]", + shouldSkip: true, + }, + { + name: "push with [skip tkn] should skip", + commitMessage: "docs: update [skip tkn]", + shouldSkip: true, + }, + { + name: "push with [tkn skip] should skip", + commitMessage: "feat: new feature [tkn skip]", + shouldSkip: true, + }, + { + name: "push without skip command should NOT skip", + commitMessage: "fix: important bug", + shouldSkip: false, + }, + { + name: "push with uppercase SKIP CI should NOT skip (case-sensitive)", + commitMessage: "fix: bug [SKIP CI]", + shouldSkip: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Test the actual skip-CI logic directly (as used in sinker.go line 92) + eventType := "push" + result := provider.SkipCI(tt.commitMessage) + + // Verify the skip-CI decision + assert.Equal(t, tt.shouldSkip, result, + "SkipCI(%q) for %s event should return %v but got %v", + tt.commitMessage, eventType, tt.shouldSkip, result) + + // Verify the condition that would cause early return in processEvent + if eventType == "push" && provider.SkipCI(tt.commitMessage) { + assert.Assert(t, tt.shouldSkip, "Event should skip when SkipCI returns true") + } else { + assert.Assert(t, !tt.shouldSkip, "Event should NOT skip when SkipCI returns false") + } + }) + } +} + +// TestGetCommitInfo_SetsHasSkipCommand tests that GetCommitInfo correctly sets +// HasSkipCommand when the commit message contains skip-CI commands. +// This tests the full flow that processEvent uses for PR events. +func TestGetCommitInfo_SetsHasSkipCommand(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + commitMessage string + expectSkipCommand bool + }{ + { + name: "commit with [skip ci] should set HasSkipCommand", + commitMessage: "fix: bug [skip ci]", + expectSkipCommand: true, + }, + { + name: "commit with [ci skip] should set HasSkipCommand", + commitMessage: "chore: update [ci skip]", + expectSkipCommand: true, + }, + { + name: "commit with [skip tkn] should set HasSkipCommand", + commitMessage: "docs: update [skip tkn]", + expectSkipCommand: true, + }, + { + name: "commit with [tkn skip] should set HasSkipCommand", + commitMessage: "feat: new feature [tkn skip]", + expectSkipCommand: true, + }, + { + name: "commit without skip command should NOT set HasSkipCommand", + commitMessage: "fix: important bug", + expectSkipCommand: false, + }, + { + name: "commit with uppercase should NOT set HasSkipCommand (case-sensitive)", + commitMessage: "fix: bug [SKIP CI]", + expectSkipCommand: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + log, _ := logger.GetLogger() + + // Create test provider + testProvider := &testprovider.TestProviderImp{ + AllowIT: true, + } + testProvider.SetLogger(log) + + // Create event with commit message (SHATitle simulates commit message from API) + event := &info.Event{ + EventType: "pull_request", + SHA: "abc123", + SHATitle: tt.commitMessage, // Simulates commit message fetched from provider API + } + + // Call GetCommitInfo - this should set HasSkipCommand based on SHATitle + // This mimics what real providers do: fetch commit from API, then call provider.SkipCI() + err := testProvider.GetCommitInfo(ctx, event) + assert.NilError(t, err) + + // Verify that HasSkipCommand was set correctly by GetCommitInfo + assert.Equal(t, tt.expectSkipCommand, event.HasSkipCommand, + "GetCommitInfo should set event.HasSkipCommand=%v for message %q", + tt.expectSkipCommand, tt.commitMessage) + + // Also verify the logic matches what provider.SkipCI would return + expectedSkip := provider.SkipCI(tt.commitMessage) + assert.Equal(t, expectedSkip, event.HasSkipCommand, + "event.HasSkipCommand should match provider.SkipCI(%q)", tt.commitMessage) + }) + } +} + +// TestProcessEvent_SkipCI_Integration documents the skip-CI flow integration. +// This is a documentation test that verifies the skip-CI decision logic +// matches what's implemented in sinker.go processEvent(). +func TestProcessEvent_SkipCI_Integration(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + eventType string + shaTitle string + hasSkipCommand bool + shouldSkip bool + description string + }{ + { + name: "push event with skip-CI in commit message", + eventType: "push", + shaTitle: "fix: bug [skip ci]", + hasSkipCommand: false, // Not used for push events + shouldSkip: true, + description: "Push events check event.SHATitle directly with provider.SkipCI()", + }, + { + name: "push event without skip command", + eventType: "push", + shaTitle: "fix: important bug", + hasSkipCommand: false, + shouldSkip: false, + description: "Push events without skip commands proceed normally", + }, + { + name: "PR event with HasSkipCommand set", + eventType: "pull_request", + shaTitle: "", // Not used for PR events + hasSkipCommand: true, + shouldSkip: true, + description: "PR events check event.HasSkipCommand set by GetCommitInfo()", + }, + { + name: "PR event without skip command", + eventType: "pull_request", + shaTitle: "", + hasSkipCommand: false, + shouldSkip: false, + description: "PR events without skip commands proceed normally", + }, + { + name: "comment event should NOT skip", + eventType: "issue_comment", + shaTitle: "", + hasSkipCommand: false, + shouldSkip: false, + description: "Comment events (e.g., /retest) are not checked for skip-CI", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Simulate the skip-CI logic from sinker.go processEvent() + // Line 92-95: Push event skip-CI check + pushSkip := tt.eventType == "push" && provider.SkipCI(tt.shaTitle) + + // Line 99-108: PR event skip-CI check + prSkip := tt.eventType == "pull_request" && tt.hasSkipCommand + + // Verify the expected behavior + actualSkip := pushSkip || prSkip + assert.Equal(t, tt.shouldSkip, actualSkip, + "%s: expected skip=%v but got skip=%v", tt.description, tt.shouldSkip, actualSkip) + }) + } +} diff --git a/pkg/params/info/events.go b/pkg/params/info/events.go index e59c689bca..7c24ef26e0 100644 --- a/pkg/params/info/events.go +++ b/pkg/params/info/events.go @@ -54,6 +54,13 @@ type Event struct { PullRequestLabel []string // Labels of the pull Request TriggerComment string // The comment triggering the pipelinerun when using on-comment annotation + // HasSkipCommand indicates whether the commit message contains a skip CI command + // (e.g., [skip ci], [ci skip], [skip tkn], [tkn skip]). When true, PipelineRun + // execution will be skipped unless overridden by a GitOps command (e.g., /test, /retest). + // This allows users to bypass CI for documentation changes or minor fixes while still + // maintaining the ability to manually trigger builds when needed. + HasSkipCommand bool + // TODO: move forge specifics to each driver // Github Organization string diff --git a/pkg/pipelineascode/client_setup.go b/pkg/pipelineascode/client_setup.go new file mode 100644 index 0000000000..e46bc8f752 --- /dev/null +++ b/pkg/pipelineascode/client_setup.go @@ -0,0 +1,98 @@ +package pipelineascode + +import ( + "context" + "fmt" + "strings" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "github.com/openshift-pipelines/pipelines-as-code/pkg/events" + "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/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" + "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github" + "go.uber.org/zap" +) + +// SetupAuthenticatedClient sets up the authenticated VCS client with proper token scoping. +// This is the centralized place for all client authentication and token scoping logic. +// +// This function is idempotent and safe to call multiple times. +func SetupAuthenticatedClient( + ctx context.Context, + vcx provider.Interface, + kint kubeinteraction.Interface, + run *params.Run, + event *info.Event, + repo *v1alpha1.Repository, + globalRepo *v1alpha1.Repository, + pacInfo *info.PacOpts, + logger *zap.SugaredLogger, +) error { + // Determine secret namespace BEFORE merging repos + // This preserves the ability to detect when credentials come from global repo + secretNS := repo.GetNamespace() + if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && + globalRepo != nil && globalRepo.Spec.GitProvider != nil && globalRepo.Spec.GitProvider.Secret != nil { + secretNS = globalRepo.GetNamespace() + } + // merge global repo settings into local repo (after determining secret namespace) + if globalRepo != nil { + repo.Spec.Merge(globalRepo.Spec) + } + + // GitHub Apps use controller secret, not Repository git_provider + if event.InstallationID > 0 { + event.Provider.WebhookSecret, _ = GetCurrentNSWebhookSecret(ctx, kint, run) + } else { + // Non-GitHub App providers use git_provider section in Repository spec + scm := SecretFromRepository{ + K8int: kint, + Config: vcx.GetConfig(), + Event: event, + Repo: repo, + WebhookType: pacInfo.WebhookType, + Logger: logger, + Namespace: secretNS, + } + if err := scm.Get(ctx); err != nil { + return fmt.Errorf("cannot get secret from repository: %w", err) + } + } + + // Set up the authenticated client + eventEmitter := events.NewEventEmitter(run.Clients.Kube, logger) + + // Validate payload with webhook secret (skip for incoming webhooks) + if event.EventType != "incoming" { + if err := vcx.Validate(ctx, run, event); err != nil { + // check that webhook secret has no /n or space into it + if strings.ContainsAny(event.Provider.WebhookSecret, "\n ") { + msg := `we have failed to validate the payload with the webhook secret, +it seems that we have detected a \n or a space at the end of your webhook secret, +is that what you want? make sure you use -n when generating the secret, eg: echo -n secret|base64` + eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositorySecretValidation", msg) + } + return fmt.Errorf("could not validate payload, check your webhook secret?: %w", err) + } + } + // Set up the authenticated client + if err := vcx.SetClient(ctx, run, event, repo, eventEmitter); err != nil { + return fmt.Errorf("failed to set client: %w", err) + } + + // Handle GitHub App token scoping for both global and repo-level configuration + if event.InstallationID > 0 { + token, err := github.ScopeTokenToListOfRepos(ctx, vcx, pacInfo, repo, run, event, eventEmitter, logger) + if err != nil { + return fmt.Errorf("failed to scope token: %w", err) + } + // If Global and Repo level configurations are not provided then lets not override the provider token. + if token != "" { + event.Provider.Token = token + } + } + + return nil +} diff --git a/pkg/pipelineascode/client_setup_test.go b/pkg/pipelineascode/client_setup_test.go new file mode 100644 index 0000000000..85b93d667a --- /dev/null +++ b/pkg/pipelineascode/client_setup_test.go @@ -0,0 +1,733 @@ +package pipelineascode + +import ( + "context" + "testing" + + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" + "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/params/settings" + testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" + kitesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + testprovider "github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider" + testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository" + "go.uber.org/zap" + zapobserver "go.uber.org/zap/zaptest/observer" + "gotest.tools/v3/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + rtesting "knative.dev/pkg/reconciler/testing" +) + +// Test helper functions + +// setupTestContext creates a test context and logger. +func setupTestContext(t *testing.T) (context.Context, *zap.SugaredLogger) { + t.Helper() + ctx, _ := rtesting.SetupFakeContext(t) + log, _ := logger.GetLogger() + return ctx, log +} + +// setupTestContextWithObserver creates a test context and logger with an observer for log inspection. +func setupTestContextWithObserver(t *testing.T) (context.Context, *zap.SugaredLogger, *zapobserver.ObservedLogs) { + t.Helper() + ctx, _ := rtesting.SetupFakeContext(t) + observer, observedLogs := zapobserver.New(zap.InfoLevel) + log := zap.New(observer).Sugar() + return ctx, log, observedLogs +} + +// createTestRepository creates a repository for testing with optional git_provider configuration. +func createTestRepository(hasGitProvider bool) *v1alpha1.Repository { + repo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "test-repo", + URL: "https://github.com/owner/repo", + InstallNamespace: "default", + }) + + if hasGitProvider { + repo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + Secret: &v1alpha1.Secret{ + Name: "test-secret", + Key: "token", + }, + WebhookSecret: &v1alpha1.Secret{ + Name: "test-secret", + Key: "webhook.secret", + }, + } + } + + return repo +} + +// createTestRepositoryWithSecretNames creates a repository with custom secret names. +func createTestRepositoryWithSecretNames(secretName, webhookSecretName string) *v1alpha1.Repository { + repo := testnewrepo.NewRepo(testnewrepo.RepoTestcreationOpts{ + Name: "test-repo", + URL: "https://github.com/owner/repo", + InstallNamespace: "default", + }) + + repo.Spec.GitProvider = &v1alpha1.GitProvider{ + URL: "https://github.com", + Secret: &v1alpha1.Secret{ + Name: secretName, + Key: "token", + }, + WebhookSecret: &v1alpha1.Secret{ + Name: webhookSecretName, + Key: "webhook.secret", + }, + } + + return repo +} + +// seedTestData seeds test data and creates a Run object with controller configuration. +func seedTestData(ctx context.Context, t *testing.T, repos []*v1alpha1.Repository) *params.Run { + t.Helper() + + namespaces := []*corev1.Namespace{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + }, + } + + stdata, _ := testclient.SeedTestData(t, ctx, testclient.Data{ + Repositories: repos, + Namespaces: namespaces, + }) + + return ¶ms.Run{ + Clients: clients.Clients{ + PipelineAsCode: stdata.PipelineAsCode, + Kube: stdata.Kube, + }, + Info: info.Info{ + Controller: &info.ControllerInfo{ + Secret: "pipelines-as-code-secret", + }, + }, + } +} + +// createTestProvider creates and configures a test provider. +func createTestProvider(log *zap.SugaredLogger) *testprovider.TestProviderImp { + provider := &testprovider.TestProviderImp{ + AllowIT: true, + } + provider.SetLogger(log) + return provider +} + +// createTestEvent creates a test event with common configuration. +func createTestEvent(eventType string, installationID int64, webhookSecret string) *info.Event { + return &info.Event{ + EventType: eventType, + InstallationID: installationID, + Provider: &info.Provider{ + URL: "https://github.com", + WebhookSecret: webhookSecret, + }, + } +} + +// createTestPacInfo creates a PacInfo object for testing. +func createTestPacInfo() *info.PacOpts { + return &info.PacOpts{ + Settings: settings.Settings{}, + } +} + +// createTestKintMock creates a kitesthelper mock with the given secrets. +func createTestKintMock(secrets map[string]string) *kitesthelper.KinterfaceTest { + return &kitesthelper.KinterfaceTest{ + GetSecretResult: secrets, + } +} + +// TestSetupAuthenticatedClient_GitHubApp tests GitHub App authentication path. +func TestSetupAuthenticatedClient_GitHubApp(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + eventType string + wantErr bool + errContains string + }{ + { + name: "GitHub App with pull_request event", + eventType: "pull_request", + wantErr: false, + }, + { + name: "GitHub App with push event", + eventType: "push", + wantErr: false, + }, + { + name: "GitHub App with incoming event skips validation", + eventType: "incoming", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + // GitHub Apps don't require git_provider in Repository spec + repo := createTestRepository(false) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "pipelines-as-code-secret": "github-app-webhook-secret", + }) + + event := createTestEvent(tt.eventType, 12345, "test-secret") // InstallationID 12345 indicates GitHub App + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "expected error but got nil") + if tt.errContains != "" { + assert.ErrorContains(t, err, tt.errContains) + } + } else { + assert.NilError(t, err, "unexpected error: %v", err) + } + }) + } +} + +// TestSetupAuthenticatedClient_NonGitHubApp tests non-GitHub App authentication path. +func TestSetupAuthenticatedClient_NonGitHubApp(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + hasGitProvider bool + secretValue string + wantErr bool + errContains string + }{ + { + name: "Non-GitHub App with git_provider succeeds", + hasGitProvider: true, + secretValue: "test-token", + wantErr: false, + }, + { + name: "Non-GitHub App without git_provider fails", + hasGitProvider: false, + wantErr: true, + errContains: "cannot get secret from repository", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + repo := createTestRepository(tt.hasGitProvider) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + // Mock secret values + secrets := map[string]string{} + if tt.hasGitProvider { + secrets["test-secret"] = tt.secretValue + } + kint := createTestKintMock(secrets) + + event := createTestEvent("pull_request", 0, "") // InstallationID 0 = Not a GitHub App + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "expected error but got nil") + if tt.errContains != "" { + assert.ErrorContains(t, err, tt.errContains) + } + } else { + assert.NilError(t, err, "unexpected error: %v", err) + // Verify token was set + assert.Equal(t, tt.secretValue, event.Provider.Token, "token should be set from secret") + } + }) + } +} + +// TestSetupAuthenticatedClient_RepositoryConfig tests repository-level configuration. +func TestSetupAuthenticatedClient_RepositoryConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + repoHasGitProvider bool + wantErr bool + wantErrContains string + }{ + { + name: "Repo with git_provider succeeds", + repoHasGitProvider: true, + wantErr: false, + }, + { + name: "Repo without git_provider fails", + repoHasGitProvider: false, + wantErr: true, + wantErrContains: "failed to find git_provider details", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + var repo *v1alpha1.Repository + if tt.repoHasGitProvider { + repo = createTestRepositoryWithSecretNames("test-secret", "test-webhook-secret") + } else { + repo = createTestRepository(false) + } + + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + "test-webhook-secret": "webhook-secret", + }) + + event := createTestEvent("pull_request", 0, "") + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient with no global repo + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "expected error but got nil") + if tt.wantErrContains != "" { + assert.ErrorContains(t, err, tt.wantErrContains) + } + } else { + assert.NilError(t, err, "unexpected error: %v", err) + assert.Equal(t, "test-token", event.Provider.Token, "should use repo token") + } + }) + } +} + +// TestSetupAuthenticatedClient_WebhookValidation tests webhook secret validation. +func TestSetupAuthenticatedClient_WebhookValidation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + webhookSecret string + eventType string + shouldFail bool + expectWarning bool + }{ + { + name: "valid webhook secret", + webhookSecret: "valid-secret", + eventType: "pull_request", + shouldFail: false, + expectWarning: false, + }, + { + name: "webhook secret with newline triggers warning", + webhookSecret: "secret-with-newline\n", + eventType: "pull_request", + shouldFail: false, // Test provider doesn't actually validate + expectWarning: true, + }, + { + name: "webhook secret with space triggers warning", + webhookSecret: "secret-with-space ", + eventType: "pull_request", + shouldFail: false, + expectWarning: true, + }, + { + name: "incoming webhook skips validation", + webhookSecret: "", + eventType: "incoming", + shouldFail: false, + expectWarning: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log, _ := setupTestContextWithObserver(t) + + repo := createTestRepository(true) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + }) + + event := createTestEvent(tt.eventType, 0, tt.webhookSecret) + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.shouldFail { + assert.Assert(t, err != nil, "expected error but got nil") + } else { + assert.NilError(t, err, "unexpected error: %v", err) + } + }) + } +} + +// TestSetupAuthenticatedClient_Idempotent tests that the function is safe to call multiple times. +func TestSetupAuthenticatedClient_Idempotent(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + repo := createTestRepository(true) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + }) + + event := createTestEvent("pull_request", 0, "") + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient multiple times + err1 := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + assert.NilError(t, err1, "first call should succeed") + + err2 := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + assert.NilError(t, err2, "second call should succeed (idempotent)") + + err3 := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + assert.NilError(t, err3, "third call should succeed (idempotent)") + + // Verify token is still correctly set + assert.Equal(t, "test-token", event.Provider.Token, "token should remain consistent") +} + +// TestSetupAuthenticatedClient_EventTypes verifies that client setup works +// correctly for all major event types across different providers. +func TestSetupAuthenticatedClient_EventTypes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + eventType string + installationID int64 + wantErr bool + description string + }{ + { + name: "push event with GitHub App", + eventType: "push", + installationID: 12345, + wantErr: false, + description: "Push events should work with GitHub App authentication", + }, + { + name: "push event without GitHub App", + eventType: "push", + installationID: 0, + wantErr: false, + description: "Push events should work with standard git_provider authentication", + }, + { + name: "pull_request event with GitHub App", + eventType: "pull_request", + installationID: 12345, + wantErr: false, + description: "PR events should work with GitHub App authentication", + }, + { + name: "pull_request event without GitHub App", + eventType: "pull_request", + installationID: 0, + wantErr: false, + description: "PR events should work with standard git_provider authentication", + }, + { + name: "issue_comment event (retest command)", + eventType: "issue_comment", + installationID: 12345, + wantErr: false, + description: "Comment events like /retest should work", + }, + { + name: "check_run event", + eventType: "check_run", + installationID: 12345, + wantErr: false, + description: "Check run events should work", + }, + { + name: "check_suite event", + eventType: "check_suite", + installationID: 12345, + wantErr: false, + description: "Check suite events should work", + }, + { + name: "incoming webhook event (skips validation)", + eventType: "incoming", + installationID: 0, + wantErr: false, + description: "Incoming webhook events should skip payload validation", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + // GitHub Apps don't need git_provider in Repository spec + hasGitProvider := tt.installationID == 0 + repo := createTestRepository(hasGitProvider) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + var secrets map[string]string + if tt.installationID > 0 { + // GitHub App uses controller secret + secrets = map[string]string{ + "pipelines-as-code-secret": "github-app-webhook-secret", + } + } else { + // Non-GitHub App uses git_provider secret + secrets = map[string]string{ + "test-secret": "test-token", + } + } + kint := createTestKintMock(secrets) + + event := createTestEvent(tt.eventType, tt.installationID, "test-secret") + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "%s: expected error but got nil", tt.description) + } else { + assert.NilError(t, err, "%s: %v", tt.description, err) + } + }) + } +} + +// TestSetupAuthenticatedClient_ProviderSpecificEvents tests event types +// specific to different Git providers (GitLab, Gitea, Bitbucket). +func TestSetupAuthenticatedClient_ProviderSpecificEvents(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + eventType string + wantErr bool + description string + }{ + { + name: "GitLab merge request event", + eventType: "Merge Request Hook", + wantErr: false, + description: "GitLab MR events should work", + }, + { + name: "GitLab push event", + eventType: "Push Hook", + wantErr: false, + description: "GitLab push events should work", + }, + { + name: "GitLab tag event", + eventType: "Tag Push Hook", + wantErr: false, + description: "GitLab tag events should work", + }, + { + name: "Gitea push event", + eventType: "push", + wantErr: false, + description: "Gitea push events should work", + }, + { + name: "Gitea pull request event", + eventType: "pull_request", + wantErr: false, + description: "Gitea PR events should work", + }, + { + name: "Bitbucket Cloud push event", + eventType: "repo:push", + wantErr: false, + description: "Bitbucket Cloud push events should work", + }, + { + name: "Bitbucket Cloud PR event", + eventType: "pullrequest:created", + wantErr: false, + description: "Bitbucket Cloud PR events should work", + }, + { + name: "Bitbucket Server PR opened", + eventType: "pr:opened", + wantErr: false, + description: "Bitbucket Server PR opened events should work", + }, + { + name: "Bitbucket Server push", + eventType: "repo:refs_changed", + wantErr: false, + description: "Bitbucket Server push events should work", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + repo := createTestRepository(true) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + }) + + event := createTestEvent(tt.eventType, 0, "test-secret") + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "%s: expected error but got nil", tt.description) + } else { + assert.NilError(t, err, "%s: %v", tt.description, err) + } + }) + } +} + +// TestSetupAuthenticatedClient_CommentEventTypes specifically tests +// GitOps comment commands to ensure they work correctly. +func TestSetupAuthenticatedClient_CommentEventTypes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + eventType string + comment string + wantErr bool + description string + }{ + { + name: "retest command on GitHub", + eventType: "issue_comment", + comment: "/retest", + wantErr: false, + description: "/retest command should work on GitHub", + }, + { + name: "test command on GitHub", + eventType: "issue_comment", + comment: "/test", + wantErr: false, + description: "/test command should work on GitHub", + }, + { + name: "ok-to-test command on GitHub", + eventType: "issue_comment", + comment: "/ok-to-test", + wantErr: false, + description: "/ok-to-test command should work on GitHub", + }, + { + name: "cancel command on GitHub", + eventType: "issue_comment", + comment: "/cancel", + wantErr: false, + description: "/cancel command should work on GitHub", + }, + { + name: "retest command on GitLab", + eventType: "Note Hook", + comment: "/retest", + wantErr: false, + description: "/retest command should work on GitLab", + }, + { + name: "test command on Gitea", + eventType: "issue_comment", + comment: "/test", + wantErr: false, + description: "/test command should work on Gitea", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx, log := setupTestContext(t) + + repo := createTestRepository(true) + run := seedTestData(ctx, t, []*v1alpha1.Repository{repo}) + testProvider := createTestProvider(log) + + kint := createTestKintMock(map[string]string{ + "test-secret": "test-token", + }) + + event := createTestEvent(tt.eventType, 0, "test-secret") + pacInfo := createTestPacInfo() + + // Call SetupAuthenticatedClient + err := SetupAuthenticatedClient(ctx, testProvider, kint, run, event, repo, nil, pacInfo, log) + + if tt.wantErr { + assert.Assert(t, err != nil, "%s: expected error but got nil", tt.description) + } else { + assert.NilError(t, err, "%s: %v", tt.description, err) + } + }) + } +} diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index a6a61984ad..44fecfb8d6 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -14,7 +14,6 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" - "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/resolve" "github.com/openshift-pipelines/pipelines-as-code/pkg/secrets" "github.com/openshift-pipelines/pipelines-as-code/pkg/templates" @@ -58,80 +57,25 @@ func (p *PacRun) verifyRepoAndUser(ctx context.Context) (*v1alpha1.Repository, e return nil, nil } - secretNS := repo.GetNamespace() - if repo.Spec.GitProvider != nil && repo.Spec.GitProvider.Secret == nil && p.globalRepo.Spec.GitProvider != nil && p.globalRepo.Spec.GitProvider.Secret != nil { - secretNS = p.globalRepo.GetNamespace() - } - if p.globalRepo != nil { - repo.Spec.Merge(p.globalRepo.Spec) - } - p.logger = p.logger.With("namespace", repo.Namespace) p.vcx.SetLogger(p.logger) p.eventEmitter.SetLogger(p.logger) - // If we have a git_provider field in repository spec, then get all the - // information from there, including the webhook secret. - // otherwise get the secret from the current ns (i.e: pipelines-as-code/openshift-pipelines.) - // - // TODO: there is going to be some improvements later we may want to do if - // they are use cases for it : - // allow webhook providers users to have a global webhook secret to be used, - // so instead of having to specify their in Repo each time, they use a - // shared one from pac. - if p.event.InstallationID > 0 { - p.event.Provider.WebhookSecret, _ = GetCurrentNSWebhookSecret(ctx, p.k8int, p.run) - } else { - scm := SecretFromRepository{ - K8int: p.k8int, - Config: p.vcx.GetConfig(), - Event: p.event, - Repo: repo, - WebhookType: p.pacInfo.WebhookType, - Logger: p.logger, - Namespace: secretNS, - } - if err := scm.Get(ctx); err != nil { - return repo, fmt.Errorf("cannot get secret from repository: %w", err) - } - } - // validate payload for webhook secret - // we don't need to validate it in incoming since we already do this - if p.event.EventType != "incoming" { - if err := p.vcx.Validate(ctx, p.run, p.event); err != nil { - // check that webhook secret has no /n or space into it - if strings.ContainsAny(p.event.Provider.WebhookSecret, "\n ") { - msg := `we have failed to validate the payload with the webhook secret, -it seems that we have detected a \n or a space at the end of your webhook secret, -is that what you want? make sure you use -n when generating the secret, eg: echo -n secret|base64` - p.eventEmitter.EmitMessage(repo, zap.ErrorLevel, "RepositorySecretValidation", msg) - } - return repo, fmt.Errorf("could not validate payload, check your webhook secret?: %w", err) - } - } - - // Set the client, we should error out if there is a problem with - // token or secret or we won't be able to do much. - err = p.vcx.SetClient(ctx, p.run, p.event, repo, p.eventEmitter) + // Set up authenticated client with proper token scoping + // NOTE: This is typically already done in sinker.processEvent() for all event types, + // but we call it here as a safety net for edge cases (e.g., tests calling Run() directly, + // or if the early setup in sinker failed/was skipped). The call is idempotent. + // SetupAuthenticatedClient will merge global repo settings after determining secret namespace. + err = SetupAuthenticatedClient(ctx, p.vcx, p.k8int, p.run, p.event, repo, p.globalRepo, p.pacInfo, p.logger) if err != nil { return repo, err } - if p.event.InstallationID > 0 { - token, err := github.ScopeTokenToListOfRepos(ctx, p.vcx, p.pacInfo, repo, p.run, p.event, p.eventEmitter, p.logger) - if err != nil { - return nil, err - } - // If Global and Repo level configurations are not provided then lets not override the provider token. - if token != "" { - p.event.Provider.Token = token - } - } - // Get the SHA commit info, we want to get the URL and commit title - err = p.vcx.GetCommitInfo(ctx, p.event) - if err != nil { - return repo, fmt.Errorf("could not find commit info: %w", err) + if p.event.SHA == "" || p.event.SHATitle == "" || p.event.SHAURL == "" { + if err = p.vcx.GetCommitInfo(ctx, p.event); err != nil { + return repo, fmt.Errorf("could not find commit info: %w", err) + } } // Verify whether the sender of the GitOps command (e.g., /test) has the appropriate permissions to diff --git a/pkg/pipelineascode/match_test.go b/pkg/pipelineascode/match_test.go index d95358f2e9..3cf517386a 100644 --- a/pkg/pipelineascode/match_test.go +++ b/pkg/pipelineascode/match_test.go @@ -443,28 +443,6 @@ func TestVerifyRepoAndUser(t *testing.T) { wantErr: true, wantErrMsg: "failed to run create status, user is not allowed to run the CI", }, - { - name: "commit not found", - runevent: info.Event{ - Organization: "owner", - Repository: "repo", - URL: "https://example.com/owner/repo", - SHA: "", - EventType: triggertype.PullRequest.String(), - TriggerTarget: triggertype.PullRequest, - InstallationID: 1, - Sender: "owner", - Request: request, - }, - repositories: []*v1alpha1.Repository{{ - ObjectMeta: metav1.ObjectMeta{Name: "repo", Namespace: "ns"}, - Spec: v1alpha1.RepositorySpec{URL: "https://example.com/owner/repo"}, - }}, - webhookSecret: "secret", - wantRepoNil: false, - wantErr: true, - wantErrMsg: "could not find commit info", - }, { name: "happy path", runevent: info.Event{ diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index aa56e90220..e0cd70e4b2 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/pkg/pipelineascode/pipelineascode.go @@ -13,6 +13,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/formatting" "github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction" "github.com/openshift-pipelines/pipelines-as-code/pkg/matcher" + "github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" @@ -90,6 +91,15 @@ func (p *PacRun) Run(ctx context.Context) error { p.manager.Enable() } + // Defensive skip-CI check: this is a safety net in case events bypass the early check in sinker. + // Primary skip detection happens in sinker.processEvent() for performance, but this ensures + // nothing slips through (e.g., tests that call Run() directly, or edge cases). + // Skip only for non-GitOps events (GitOps commands can override skip-CI). + if p.event.HasSkipCommand && !opscomments.IsAnyOpsEventType(p.event.EventType) { + p.logger.Infof("CI skipped: commit contains skip command in message (secondary check)") + return nil + } + // set params for the console driver, only used for the custom console ones cp := customparams.NewCustomParams(p.event, repo, p.run, p.k8int, p.eventEmitter, p.vcx) maptemplate, _, err := cp.GetParams(ctx) diff --git a/pkg/provider/bitbucketcloud/bitbucket.go b/pkg/provider/bitbucketcloud/bitbucket.go index 8f266c4dc9..99dcbb993a 100644 --- a/pkg/provider/bitbucketcloud/bitbucket.go +++ b/pkg/provider/bitbucketcloud/bitbucket.go @@ -262,6 +262,7 @@ func (v *Provider) GetCommitInfo(_ context.Context, event *info.Event) error { } // Note: Bitbucket Cloud API doesn't provide author email or timestamps in the basic commit response + event.HasSkipCommand = provider.SkipCI(commitinfo.Message) // now to get the default branch from repository.Get repo, err := v.Client().Repositories.Repository.Get(&bitbucket.RepositoryOptions{ Owner: event.Organization, diff --git a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go index fe48a91ec5..bcb49a2fcd 100644 --- a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go +++ b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go @@ -339,6 +339,7 @@ func (v *Provider) GetCommitInfo(_ context.Context, event *info.Event) error { } event.SHATitle = sanitizeTitle(commit.Message) event.SHAURL = fmt.Sprintf("%s/projects/%s/repos/%s/commits/%s", v.baseURL, v.projectKey, event.Repository, event.SHA) + event.HasSkipCommand = provider.SkipCI(commit.Message) // Populate full commit information for LLM context event.SHAMessage = commit.Message diff --git a/pkg/provider/gitea/gitea.go b/pkg/provider/gitea/gitea.go index 7beb9fc8c6..2f0d106e83 100644 --- a/pkg/provider/gitea/gitea.go +++ b/pkg/provider/gitea/gitea.go @@ -399,6 +399,7 @@ func (v *Provider) GetCommitInfo(_ context.Context, runevent *info.Event) error } } } + runevent.HasSkipCommand = provider.SkipCI(commit.RepoCommit.Message) return nil } diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index dc74ebec04..c882cc0593 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -406,6 +406,7 @@ func (v *Provider) GetCommitInfo(ctx context.Context, runevent *info.Event) erro runevent.SHAURL = commit.GetHTMLURL() runevent.SHATitle = strings.Split(commit.GetMessage(), "\n\n")[0] runevent.SHA = commit.GetSHA() + runevent.HasSkipCommand = provider.SkipCI(commit.GetMessage()) // Populate full commit information for LLM context runevent.SHAMessage = commit.GetMessage() diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 38d544b67d..b0cf230c1d 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -604,6 +604,7 @@ func TestGithubGetCommitInfo(t *testing.T) { committerName, committerEmail string authorDate, committerDate string checkExtendedFields bool + wantHasSkipCmd bool }{ { name: "good with full commit info", @@ -630,9 +631,58 @@ func TestGithubGetCommitInfo(t *testing.T) { Repository: "repository", SHA: "shacommitinfo", }, - shaurl: "https://git.provider/commit/info", - shatitle: "Simple commit", - message: "Simple commit", + shaurl: "https://git.provider/commit/info", + shatitle: "My beautiful pony", + message: "My beautiful pony", + wantHasSkipCmd: false, + }, + { + name: "commit with skip ci command", + event: &info.Event{ + Organization: "owner", + Repository: "repository", + SHA: "shacommitinfo", + }, + shaurl: "https://git.provider/commit/info", + shatitle: "fix: some bug", + message: "fix: some bug\n\n[skip ci]", + wantHasSkipCmd: true, + }, + { + name: "commit with ci skip command in title", + event: &info.Event{ + Organization: "owner", + Repository: "repository", + SHA: "shacommitinfo", + }, + shaurl: "https://git.provider/commit/info", + shatitle: "feat: new feature [ci skip]", + message: "feat: new feature [ci skip]", + wantHasSkipCmd: true, + }, + { + name: "commit with skip tkn command in title", + event: &info.Event{ + Organization: "owner", + Repository: "repository", + SHA: "shacommitinfo", + }, + shaurl: "https://git.provider/commit/info", + shatitle: "docs: update [skip tkn]", + message: "docs: update [skip tkn]", + wantHasSkipCmd: true, + }, + { + name: "commit with tkn skip command in body", + event: &info.Event{ + Organization: "owner", + Repository: "repository", + SHA: "shacommitinfo", + }, + shaurl: "https://git.provider/commit/info", + shatitle: "chore: deps", + message: "chore: deps\n\n[tkn skip]", + wantHasSkipCmd: true, }, { name: "error", @@ -678,10 +728,16 @@ func TestGithubGetCommitInfo(t *testing.T) { } `json:"committer,omitempty"` } + // Use message if provided, otherwise use shatitle as fallback + message := tt.message + if message == "" && tt.shatitle != "" { + message = tt.shatitle + } + resp := commitResponse{ SHA: "shacommitinfo", HTMLURL: tt.shaurl, - Message: tt.message, + Message: message, } if tt.checkExtendedFields { @@ -735,6 +791,7 @@ func TestGithubGetCommitInfo(t *testing.T) { expectedCommitterDate, _ := time.Parse(time.RFC3339, tt.committerDate) assert.DeepEqual(t, expectedCommitterDate, tt.event.SHACommitterDate) } + assert.Equal(t, tt.wantHasSkipCmd, tt.event.HasSkipCommand) }) } } diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index c542d7b130..0e5aee315d 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -183,29 +183,6 @@ func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *h processedEvent.GHEURL = event.Provider.URL processedEvent.Provider.URL = event.Provider.URL - // regenerate token scoped to the repo IDs - if v.pacInfo.SecretGHAppRepoScoped && installationIDFrompayload != -1 && len(v.RepositoryIDs) > 0 { - repoLists := []string{} - if v.pacInfo.SecretGhAppTokenScopedExtraRepos != "" { - // this is going to show up a lot in the logs but i guess that - // would make people fix the value instead of being lost into - // the top of the logs at controller start. - for _, configValue := range strings.Split(v.pacInfo.SecretGhAppTokenScopedExtraRepos, ",") { - configValueS := strings.TrimSpace(configValue) - if configValueS == "" { - continue - } - repoLists = append(repoLists, configValueS) - } - v.Logger.Infof("Github token scope extended to %v keeping SecretGHAppRepoScoped to true", repoLists) - } - token, err := v.CreateToken(ctx, repoLists, processedEvent) - if err != nil { - return nil, err - } - processedEvent.Provider.Token = token - } - return processedEvent, nil } diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index 55b77b38e1..3aac77b824 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -5,8 +5,6 @@ import ( "encoding/json" "fmt" "net/http" - "strconv" - "strings" "testing" "github.com/google/go-github/v74/github" @@ -1112,16 +1110,13 @@ func TestAppTokenGeneration(t *testing.T) { }) tests := []struct { - ctx context.Context - ctxNS string - name string - wantErrSubst string - nilClient bool - seedData testclient.Clients - envs map[string]string - resultBaseURL string - checkInstallIDs []int64 - extraRepoInstallIDs map[string]string + ctx context.Context + ctxNS string + name string + wantErrSubst string + nilClient bool + seedData testclient.Clients + envs map[string]string }{ { name: "secret not found", @@ -1137,23 +1132,6 @@ func TestAppTokenGeneration(t *testing.T) { seedData: vaildSecret, nilClient: false, }, - { - ctx: ctx, - name: "check installation ids are set", - ctxNS: testNamespace, - seedData: vaildSecret, - nilClient: false, - checkInstallIDs: []int64{123}, - }, - { - ctx: ctx, - name: "check extras installations ids set", - ctxNS: testNamespace, - seedData: vaildSecret, - nilClient: false, - checkInstallIDs: []int64{123}, - extraRepoInstallIDs: map[string]string{"another/one": "789", "andanother/two": "10112"}, - }, { ctx: ctxInvalidAppID, name: "invalid app id in secret", @@ -1184,17 +1162,6 @@ func TestAppTokenGeneration(t *testing.T) { ID: &testInstallationID, } - if len(tt.checkInstallIDs) > 0 { - samplePRevent.PullRequest = &github.PullRequest{ - // order is important here for the check later - Base: &github.PullRequestBranch{ - Repo: &github.Repository{ - ID: github.Ptr(tt.checkInstallIDs[0]), - }, - }, - } - } - jeez, _ := json.Marshal(samplePRevent) logger, _ := logger.GetLogger() gprovider := Provider{ @@ -1222,26 +1189,6 @@ func TestAppTokenGeneration(t *testing.T) { }, } - if len(tt.checkInstallIDs) > 0 { - gprovider.pacInfo.SecretGHAppRepoScoped = true - } - if len(tt.extraRepoInstallIDs) > 0 { - extras := "" - for name := range tt.extraRepoInstallIDs { - split := strings.Split(name, "/") - mux.HandleFunc(fmt.Sprintf("/repos/%s/%s", split[0], split[1]), func(w http.ResponseWriter, _ *http.Request) { - // i can't do a for name, iid and use iid, cause golang shadows the variable out of the for loop - // a bit stupid - sid := tt.extraRepoInstallIDs[fmt.Sprintf("%s/%s", split[0], split[1])] - _, _ = fmt.Fprintf(w, `{"id": %s}`, sid) - }) - extras += fmt.Sprintf("%s, ", name) - } - - gprovider.pacInfo.SecretGHAppRepoScoped = true - gprovider.pacInfo.SecretGhAppTokenScopedExtraRepos = extras - } - tt.ctx = info.StoreCurrentControllerName(tt.ctx, "default") tt.ctx = info.StoreNS(tt.ctx, tt.ctxNS) @@ -1257,28 +1204,8 @@ func TestAppTokenGeneration(t *testing.T) { return } - for k, id := range tt.checkInstallIDs { - if gprovider.RepositoryIDs[k] != id { - t.Errorf("got %d, want %d", gprovider.RepositoryIDs[k], id) - } - } - - for _, extraid := range tt.extraRepoInstallIDs { - // checkInstallIDs and extraRepoInstallIds are merged and extraRepoInstallIds is after - found := false - extraIDInt, _ := strconv.ParseInt(extraid, 10, 64) - for _, rid := range gprovider.RepositoryIDs { - if extraIDInt == rid { - found = true - } - } - assert.Assert(t, found, "Could not find %s in %s", extraIDInt, tt.extraRepoInstallIDs) - } - + // Verify client was created successfully for GitHub App assert.Assert(t, gprovider.Client() != nil) - if tt.resultBaseURL != "" { - assert.Equal(t, gprovider.Client().BaseURL.String(), tt.resultBaseURL) - } }) } } diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 7d68136b8b..e65707ed9a 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -490,6 +490,7 @@ func (v *Provider) GetCommitInfo(_ context.Context, runevent *info.Event) error if branchinfo.CommittedDate != nil { runevent.SHACommitterDate = *branchinfo.CommittedDate } + runevent.HasSkipCommand = provider.SkipCI(branchinfo.Message) } return nil diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 6a12669788..fe7738cc90 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -214,3 +214,13 @@ func GetCheckName(status StatusOpts, pacopts *info.PacOpts) string { func IsZeroSHA(sha string) bool { return sha == "0000000000000000000000000000000000000000" } + +// skipCIRegex is a compiled regular expression for detecting skip CI commands. +// It matches [skip ci], [ci skip], [skip tkn], or [tkn skip]. +// Skip commands can be overridden by GitOps commands like /test, /retest. +var skipCIRegex = regexp.MustCompile(`\[(skip ci|ci skip|skip tkn|tkn skip)\]`) + +// SkipCI returns true if the given commit message contains any skip command. +func SkipCI(commitMessage string) bool { + return skipCIRegex.MatchString(commitMessage) +} diff --git a/pkg/provider/provider_test.go b/pkg/provider/provider_test.go index 6398cb74a9..b1d29fdae4 100644 --- a/pkg/provider/provider_test.go +++ b/pkg/provider/provider_test.go @@ -560,3 +560,99 @@ func TestGetCheckName(t *testing.T) { }) } } + +func TestSkipCI(t *testing.T) { + tests := []struct { + name string + commitMessage string + want bool + }{ + { + name: "skip ci lowercase", + commitMessage: "fix: some bug [skip ci]", + want: true, + }, + { + name: "ci skip lowercase", + commitMessage: "feat: new feature [ci skip]", + want: true, + }, + { + name: "skip tkn lowercase", + commitMessage: "docs: update readme [skip tkn]", + want: true, + }, + { + name: "tkn skip lowercase", + commitMessage: "chore: update deps [tkn skip]", + want: true, + }, + { + name: "skip ci at beginning", + commitMessage: "[skip ci] WIP: work in progress", + want: true, + }, + { + name: "ci skip in middle", + commitMessage: "fix: bug\n\n[ci skip]\n\nmore details", + want: true, + }, + { + name: "skip tkn at end", + commitMessage: "feat: new feature\n\nSome description [skip tkn]", + want: true, + }, + { + name: "multiple skip commands", + commitMessage: "fix: bug [skip ci] [skip tkn]", + want: true, + }, + { + name: "no skip command", + commitMessage: "feat: normal commit without skip", + want: false, + }, + { + name: "skip ci with typo", + commitMessage: "fix: bug [skip-ci]", + want: false, + }, + { + name: "skip without brackets", + commitMessage: "fix: bug skip ci", + want: false, + }, + { + name: "empty commit message", + commitMessage: "", + want: false, + }, + { + name: "skip ci with uppercase", + commitMessage: "fix: bug [SKIP CI]", + want: false, + }, + { + name: "skip ci with extra spaces", + commitMessage: "fix: bug [ skip ci ]", + want: false, + }, + { + name: "multiline with skip ci", + commitMessage: "fix: important bug fix\n\nThis commit fixes a critical issue.\n\n[skip ci]", + want: true, + }, + { + name: "skip ci in commit body", + commitMessage: "feat: add new feature\n\nTesting [skip ci] in body", + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SkipCI(tt.commitMessage) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/test/provider/testwebvcs.go b/pkg/test/provider/testwebvcs.go index 56e2428571..d310b0e6d6 100644 --- a/pkg/test/provider/testwebvcs.go +++ b/pkg/test/provider/testwebvcs.go @@ -30,6 +30,8 @@ type TestProviderImp struct { WantDeletedFiles []string WantModifiedFiles []string WantRenamedFiles []string + FailGetCommitInfo bool + CommitInfoErrorMsg string pacInfo *info.PacOpts } @@ -71,7 +73,18 @@ func (v *TestProviderImp) GetConfig() *info.ProviderConfig { return &info.ProviderConfig{} } -func (v *TestProviderImp) GetCommitInfo(_ context.Context, _ *info.Event) error { +func (v *TestProviderImp) GetCommitInfo(_ context.Context, event *info.Event) error { + if v.FailGetCommitInfo { + if v.CommitInfoErrorMsg != "" { + return fmt.Errorf("%s", v.CommitInfoErrorMsg) + } + return fmt.Errorf("failed to get commit info") + } + // Simulate what real providers do: set HasSkipCommand based on commit message + // Real providers set this from the commit message fetched via API + if event.SHATitle != "" { + event.HasSkipCommand = provider.SkipCI(event.SHATitle) + } return nil } diff --git a/test/github_skip_ci_test.go b/test/github_skip_ci_test.go new file mode 100644 index 0000000000..0b7b393fba --- /dev/null +++ b/test/github_skip_ci_test.go @@ -0,0 +1,134 @@ +//go:build e2e + +package test + +import ( + "context" + "fmt" + "os" + "testing" + "time" + + "github.com/google/go-github/v74/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" + tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github" + twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait" + "gotest.tools/v3/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestGithubSkipCIPullRequest tests that [skip ci] in commit message prevents +// PipelineRun execution on pull requests. +func TestGithubSkipCIPullRequest(t *testing.T) { + if os.Getenv("NIGHTLY_E2E_TEST") != "true" { + t.Skip("Skipping test since only enabled for nightly") + } + ctx := context.Background() + g := &tgithub.PRTest{ + Label: "Github Skip CI Pull Request [skip ci]", + YamlFiles: []string{"testdata/pipelinerun.yaml"}, + NoStatusCheck: true, // Don't wait for success since we expect no PipelineRun + } + + // The CommitTitle will be used as the commit message, so [skip ci] is included + g.RunPullRequest(ctx, t) + defer g.TearDown(ctx, t) + + // Wait a bit to ensure no PipelineRun is created + time.Sleep(10 * time.Second) + + // Verify that NO PipelineRuns were created due to [skip ci] + pruns, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA), + }) + assert.NilError(t, err) + assert.Equal(t, 0, len(pruns.Items), "Expected no PipelineRuns to be created due to [skip ci] in commit message") + + g.Cnx.Clients.Log.Infof("✓ Verified that [skip ci] prevented PipelineRun creation") +} + +// TestGithubSkipCIPush tests that [skip ci] in commit message prevents +// PipelineRun execution on push events. +func TestGithubSkipCIPush(t *testing.T) { + if os.Getenv("NIGHTLY_E2E_TEST") != "true" { + t.Skip("Skipping test since only enabled for nightly") + } + ctx := context.Background() + g := &tgithub.PRTest{ + Label: "Github Skip CI Push [skip ci]", + YamlFiles: []string{"testdata/pipelinerun-on-push.yaml"}, + NoStatusCheck: true, // Don't wait for success since we expect no PipelineRun + } + + g.RunPushRequest(ctx, t) + defer g.TearDown(ctx, t) + + // Wait a bit to ensure no PipelineRun is created + time.Sleep(10 * time.Second) + + // Verify that NO PipelineRuns were created due to [skip ci] + pruns, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA), + }) + assert.NilError(t, err) + assert.Equal(t, 0, len(pruns.Items), "Expected no PipelineRuns to be created due to [skip ci] in commit message") + + g.Cnx.Clients.Log.Infof("✓ Verified that [skip ci] prevented PipelineRun creation on push") +} + +// TestGithubSkipCITestCommand tests that /test command can override [skip tkn]. +func TestGithubSkipCITestCommand(t *testing.T) { + if os.Getenv("NIGHTLY_E2E_TEST") != "true" { + t.Skip("Skipping test since only enabled for nightly") + } + ctx := context.Background() + g := &tgithub.PRTest{ + Label: "Github Skip CI Test Command [skip tkn]", + YamlFiles: []string{"testdata/pipelinerun.yaml"}, + NoStatusCheck: true, + } + + g.RunPullRequest(ctx, t) + defer g.TearDown(ctx, t) + + // Wait a bit to ensure no PipelineRun is created + time.Sleep(10 * time.Second) + + // Verify no PipelineRuns initially + pruns, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA), + }) + assert.NilError(t, err) + assert.Equal(t, 0, len(pruns.Items), "Expected no PipelineRuns before /test command") + + g.Cnx.Clients.Log.Infof("✓ Verified [skip tkn] prevented initial PipelineRun") + + // Post a /test comment which should override the skip command + g.Cnx.Clients.Log.Infof("Posting /test comment to override [skip tkn]") + _, _, err = g.Provider.Client().Issues.CreateComment(ctx, + g.Options.Organization, + g.Options.Repo, + g.PRNumber, + &github.IssueComment{Body: github.Ptr("/test")}) + assert.NilError(t, err) + + // Wait for PipelineRun to be created + waitOpts := twait.Opts{ + RepoName: g.TargetNamespace, + Namespace: g.TargetNamespace, + MinNumberStatus: 1, + PollTimeout: twait.DefaultTimeout, + TargetSHA: g.SHA, + } + err = twait.UntilPipelineRunCreated(ctx, g.Cnx.Clients, waitOpts) + assert.NilError(t, err) + + // Verify PipelineRun was created + pruns, err = g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA), + }) + assert.NilError(t, err) + assert.Assert(t, len(pruns.Items) >= 1, "Expected at least one PipelineRun via /test") + + g.Cnx.Clients.Log.Infof("✓ Verified that /test override [skip tkn] and created %d PipelineRun(s)", len(pruns.Items)) +}