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 a25138a7f8..5656088590 100644 --- a/docs/content/docs/guide/matchingevents.md +++ b/docs/content/docs/guide/matchingevents.md @@ -486,3 +486,97 @@ 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. A neutral status check will be displayed on the PR indicating that CI was skipped. +2. **Push Events**: No PipelineRuns will be created when pushing to a branch with that commit message. A neutral status check will be displayed on the commit. + +**Note:** A neutral status check is created on your git provider to provide visibility that the commit was acknowledged but CI was intentionally skipped. This helps distinguish between commits that were ignored due to skip commands versus commits where CI hasn't run. + +### 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 +# A neutral status check is displayed on the commit/PR + +# 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 9effed3fb7..1db6a72539 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" @@ -76,8 +78,98 @@ func (s *sinker) processEvent(ctx context.Context, request *http.Request) error if s.event == nil { return nil } + + // 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: %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 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 s.createSkipCIStatus(ctx) + } + + // 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 s.createSkipCIStatus(ctx) + } + } } 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, + ) +} + +// createSkipCIStatus creates a neutral status check on the git provider when CI is skipped. +func (s *sinker) createSkipCIStatus(ctx context.Context) error { + statusOpts := provider.StatusOpts{ + Status: "completed", + Conclusion: "neutral", + Title: "CI Skipped", + Summary: fmt.Sprintf("%s - CI has been skipped", s.pacInfo.ApplicationName), + Text: "Commit contains a skip CI command. Use /test or /retest to manually trigger CI if needed.", + DetailsURL: s.run.Clients.ConsoleUI().URL(), + } + + if err := s.vcx.CreateStatus(ctx, s.event, statusOpts); err != nil { + s.logger.Warnf("Failed to create skip-CI status: %v", err) + // Don't return error - skip-CI should succeed even if status creation fails + return nil + } + + return nil +} 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 f4886c764d..ba706ba76f 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 30ab712799..b2071d9a22 100644 --- a/pkg/pipelineascode/match_test.go +++ b/pkg/pipelineascode/match_test.go @@ -488,28 +488,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 23da8005e5..190d8467c4 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 ce3fde8310..763198347c 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -187,29 +187,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 9c9a6ff282..924628239c 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" @@ -1118,16 +1116,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", @@ -1143,23 +1138,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", @@ -1190,17 +1168,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{ @@ -1228,26 +1195,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) @@ -1263,28 +1210,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 66c224f11f..145ac18626 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -491,6 +491,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..4e36a7a26a --- /dev/null +++ b/test/github_skip_ci_test.go @@ -0,0 +1,144 @@ +//go:build e2e + +package test + +import ( + "context" + "fmt" + "regexp" + "testing" + "time" + + "github.com/google/go-github/v74/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" + "github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx" + 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" +) + +// verifySkipCI is a helper function that verifies no PipelineRuns were created +// and that controller logs mention the skip command detection for a given event type. +func verifySkipCI(ctx context.Context, t *testing.T, g *tgithub.PRTest, eventType string) { + t.Helper() + + // 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") + + // Setup context with controller namespace info before checking logs + ctx, err = cctx.GetControllerCtxInfo(ctx, g.Cnx) + assert.NilError(t, err) + + // Verify controller logs mention skip command detection + numLines := int64(100) + skipLogRegex := regexp.MustCompile(fmt.Sprintf("CI skipped for %s event.*contains skip command in message", eventType)) + err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *skipLogRegex, 10, "controller", &numLines) + assert.NilError(t, err, "Expected controller logs to mention CI skip due to skip command") + + g.Cnx.Clients.Log.Infof("✓ Verified controller logs mention skip command detection") +} + +// TestGithubSkipCIPullRequest tests that [skip ci] in commit message prevents +// PipelineRun execution on pull requests. +func TestGithubSkipCIPullRequest(t *testing.T) { + 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) + + verifySkipCI(ctx, t, g, "pull request") +} + +// TestGithubSkipCIPush tests that [skip ci] in commit message prevents +// PipelineRun execution on push events. +func TestGithubSkipCIPush(t *testing.T) { + 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) + + verifySkipCI(ctx, t, g, "push") +} + +// TestGithubSkipCITestCommand tests that /test command can override [skip tkn]. +func TestGithubSkipCITestCommand(t *testing.T) { + 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") + + // Setup context with controller namespace info before checking logs + ctx, err = cctx.GetControllerCtxInfo(ctx, g.Cnx) + assert.NilError(t, err) + + // Verify controller logs mention skip command detection + numLines := int64(100) + skipLogRegex := regexp.MustCompile("CI skipped for pull request event.*contains skip command in message") + err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *skipLogRegex, 10, "controller", &numLines) + assert.NilError(t, err, "Expected controller logs to mention CI skip due to skip command") + + g.Cnx.Clients.Log.Infof("✓ Verified controller logs mention skip command detection") + + // 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)) +}