feat: implement cancelAllPipelines for CI providers #205
Conversation
WalkthroughThis PR extends CI pipeline cancellation capabilities across multiple providers (Azure, GitHub, Jenkins, GitLab, Tekton) and the OpenShift Kubernetes client. It replaces the initial-pipeline handling with a comprehensive Changes
Sequence Diagram(s)sequenceDiagram
participant User as Test/Client
participant CI as CI Provider<br/>(e.g., AzureCI)
participant Svc as Service Layer<br/>(AzurePipelineService)
participant API as Azure API
User->>CI: cancelAllPipelines(options?)
activate CI
CI->>CI: normalizeOptions()
CI->>CI: fetchAllBuilds()<br/>(source + gitops)
CI->>CI: filterBuilds()<br/>(patterns, status,<br/>eventType, branch)
CI->>CI: cancelBuildsInBatches()<br/>(concurrency)
activate CI
loop Per-batch
CI->>Svc: cancelBuild(buildId)
activate Svc
Svc->>API: PATCH /builds/{id}/status
alt Success
API-->>Svc: 200 OK
Svc-->>CI: void
CI->>CI: Record: cancelled
else 404/403/400
API-->>Svc: Error
Svc-->>CI: Throw specific error
CI->>CI: Record: failed/skipped
end
deactivate Svc
end
deactivate CI
CI->>CI: Accumulate CancelResult<br/>(total, cancelled,<br/>failed, skipped, details)
CI-->>User: CancelResult
deactivate CI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
api-client
enhancement
xinredhat
force-pushed
the
cancel_pipelines
branch
6 times, most recently
from
b3f2aac to
dbbbf5f
Compare
Assisted-by: Claude Code
xinredhat
force-pushed
the
cancel_pipelines
branch
from
dbbbf5f to
f8c1055
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
src/api/github/services/github-actions.service.ts (2)
604-608: Add initial logging for consistency with service patterns.Other methods in this service log at the start of operations (e.g., lines 259, 314, 382) to aid debugging. Adding similar logging here improves observability.
Apply this diff:
async cancelWorkflowRun( owner: string, repo: string, runId: number ): Promise<void> { + console.log(`Cancelling workflow run #${runId} for ${owner}/${repo}`); + try {
634-653: Consider adding fallback error status check for robustness.Other error-handling blocks in this file check both
error.statusanderror.response?.status(see lines 219, 278, 334, 408). Adding the same fallback pattern here ensures consistent error handling across different error shapes from the Octokit library.Apply this diff to the error status checks:
// Handle specific error cases - if (error.status === 404) { + if (error.status === 404 || (error.response && error.response.status === 404)) { throw new GithubNotFoundError( 'workflow run', `${runId} in repository ${owner}/${repo}` ); } - if (error.status === 403) { + if (error.status === 403 || (error.response && error.response.status === 403)) { throw new GithubApiError( `Insufficient permissions to cancel workflow run ${runId}`, error.status ); } - if (error.status === 409) { + if (error.status === 409 || (error.response && error.response.status === 409)) { throw new GithubApiError( `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`, error.status ); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/api/azure/services/azure-pipeline.service.ts(1 hunks)src/api/github/services/github-actions.service.ts(1 hunks)src/api/jenkins/services/jenkins-build.service.ts(1 hunks)src/api/ocp/kubeClient.ts(1 hunks)src/api/tekton/services/tekton-pipelinerun.service.ts(1 hunks)src/api/tekton/tekton.client.ts(1 hunks)src/rhtap/core/integration/ci/baseCI.ts(2 hunks)src/rhtap/core/integration/ci/ciInterface.ts(2 hunks)src/rhtap/core/integration/ci/providers/azureCI.ts(2 hunks)src/rhtap/core/integration/ci/providers/githubActionsCI.ts(2 hunks)src/rhtap/core/integration/ci/providers/gitlabCI.ts(2 hunks)src/rhtap/core/integration/ci/providers/jenkinsCI.ts(3 hunks)src/rhtap/core/integration/ci/providers/tektonCI.ts(3 hunks)src/utils/test/common.ts(0 hunks)tests/api/git/github.test.ts(0 hunks)tests/tssc/full_workflow.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- tests/api/git/github.test.ts
- src/utils/test/common.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/api/**
⚙️ CodeRabbit configuration file
Focus on error handling, retry mechanisms, proper async/await usage, API client patterns, authentication security, and comprehensive logging for debugging. Ensure all API calls have appropriate timeouts and circuit breaker patterns.
Files:
src/api/github/services/github-actions.service.tssrc/api/azure/services/azure-pipeline.service.tssrc/api/ocp/kubeClient.tssrc/api/tekton/services/tekton-pipelinerun.service.tssrc/api/tekton/tekton.client.tssrc/api/jenkins/services/jenkins-build.service.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (README.md)
Test files should use the .test.ts naming convention
Files:
tests/tssc/full_workflow.test.ts
tests/**
⚙️ CodeRabbit configuration file
Focus on test coverage, maintainability, proper test structure, test isolation, mock usage, async test patterns, and test stability. Ensure tests are deterministic and properly clean up after themselves.
Files:
tests/tssc/full_workflow.test.ts
**/*.test.ts
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper assertions, test isolation, async pattern correctness, mock usage, and test maintainability. Review for potential flaky tests and race conditions.
Files:
tests/tssc/full_workflow.test.ts
src/rhtap/core/**
⚙️ CodeRabbit configuration file
Review component architecture, interface design, abstraction patterns, extensibility, integration reliability, and proper type definitions. Ensure the factory and strategy patterns are implemented correctly.
Files:
src/rhtap/core/integration/ci/baseCI.tssrc/rhtap/core/integration/ci/ciInterface.tssrc/rhtap/core/integration/ci/providers/githubActionsCI.tssrc/rhtap/core/integration/ci/providers/tektonCI.tssrc/rhtap/core/integration/ci/providers/jenkinsCI.tssrc/rhtap/core/integration/ci/providers/azureCI.tssrc/rhtap/core/integration/ci/providers/gitlabCI.ts
src/rhtap/core/integration/**
⚙️ CodeRabbit configuration file
Focus on integration reliability, error handling, factory pattern implementation, provider abstraction, configuration management, and testing of external dependencies. Validate connection pooling and resource management.
Files:
src/rhtap/core/integration/ci/baseCI.tssrc/rhtap/core/integration/ci/ciInterface.tssrc/rhtap/core/integration/ci/providers/githubActionsCI.tssrc/rhtap/core/integration/ci/providers/tektonCI.tssrc/rhtap/core/integration/ci/providers/jenkinsCI.tssrc/rhtap/core/integration/ci/providers/azureCI.tssrc/rhtap/core/integration/ci/providers/gitlabCI.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:42:15.469Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: tests/templates/import-templates.test.ts:138-168
Timestamp: 2025-08-19T06:42:15.469Z
Learning: In the TSSC E2E testing framework, test cleanup (like deleting Git repositories and catalog entities) is not handled via afterAll blocks in individual test files, following the pattern established in the full workflow suite.
Applied to files:
tests/tssc/full_workflow.test.ts
🧬 Code graph analysis (8)
src/api/github/services/github-actions.service.ts (1)
src/api/github/errors/github.errors.ts (2)
GithubNotFoundError(22-27)GithubApiError(65-70)
src/rhtap/core/integration/ci/baseCI.ts (1)
src/rhtap/core/integration/ci/ciInterface.ts (2)
CancelPipelineOptions(29-66)CancelResult(71-101)
src/rhtap/core/integration/ci/providers/githubActionsCI.ts (2)
src/rhtap/core/integration/ci/ciInterface.ts (4)
CancelPipelineOptions(29-66)CancelResult(71-101)PipelineCancelDetail(106-141)CancelError(146-171)src/api/github/types/github.types.ts (1)
WorkflowRun(49-65)
src/rhtap/core/integration/ci/providers/tektonCI.ts (1)
src/rhtap/core/integration/ci/ciInterface.ts (4)
CancelPipelineOptions(29-66)CancelResult(71-101)PipelineCancelDetail(106-141)CancelError(146-171)
src/rhtap/core/integration/ci/providers/jenkinsCI.ts (1)
src/rhtap/core/integration/ci/ciInterface.ts (5)
CancelPipelineOptions(29-66)CancelResult(71-101)PipelineCancelDetail(106-141)PipelineStatus(225-225)CancelError(146-171)
src/rhtap/core/integration/ci/providers/azureCI.ts (2)
src/rhtap/core/integration/ci/ciInterface.ts (4)
CancelPipelineOptions(29-66)CancelResult(71-101)PipelineCancelDetail(106-141)CancelError(146-171)src/api/azure/types/azure.types.ts (1)
AzureBuild(66-83)
src/rhtap/core/integration/ci/providers/gitlabCI.ts (1)
src/rhtap/core/integration/ci/ciInterface.ts (4)
CancelPipelineOptions(29-66)CancelResult(71-101)PipelineCancelDetail(106-141)CancelError(146-171)
src/api/jenkins/services/jenkins-build.service.ts (2)
src/api/jenkins/utils/jenkins.utils.ts (1)
JenkinsPathBuilder(7-52)src/api/jenkins/errors/jenkins.errors.ts (1)
JenkinsBuildNotFoundError(51-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-test-on-pull-request
🔇 Additional comments (1)
src/api/azure/services/azure-pipeline.service.ts (1)
171-204: Timeout configuration is properly implemented and safeguards this method.The
BaseHttpClientsets a sensible default timeout of 30 seconds (config.timeout ?? 30000), which is automatically applied to all HTTP requests including the PATCH call incancelBuild. This prevents hangs on slow or unresponsive endpoints and satisfies the coding guidelines for API clients. The timeout is configured at the HTTP client level, making it a reliable, framework-wide protection rather than something needing per-method configuration.
| await retry( | ||
| async () => { | ||
| await this.client.patch( | ||
| `${this.project}/_apis/build/builds/${buildId}?${this.getApiVersionParam()}`, | ||
| { status: 'cancelling' } | ||
| ); | ||
| }, | ||
| { | ||
| retries: 3, | ||
| minTimeout: 1000, | ||
| maxTimeout: 5000, | ||
| onRetry: (error: Error, attempt: number) => { | ||
| console.log(`[Azure] Retry ${attempt}/3 - Cancelling build ${buildId}: ${error.message}`); | ||
| }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
Non-retryable errors should not trigger retries.
The retry mechanism will attempt 3 retries for all errors, including HTTP 400, 403, and 404 responses. These status codes indicate permanent failures (bad request, forbidden, not found) that won't succeed on retry, wasting time and resources.
Refactor to check the error status and bail early for non-retryable errors:
await retry(
async () => {
- await this.client.patch(
+ try {
+ await this.client.patch(
`${this.project}/_apis/build/builds/${buildId}?${this.getApiVersionParam()}`,
{ status: 'cancelling' }
);
+ } catch (error: any) {
+ // Don't retry on permanent failures
+ if (error.response?.status && [400, 403, 404].includes(error.response.status)) {
+ throw new retry.AbortError(error);
+ }
+ throw error;
+ }
},
{
retries: 3,Committable suggestion skipped: line range outside the PR's diff.
| } catch (error: any) { | ||
| // Handle specific error cases | ||
| if (error.response?.status === 404) { | ||
| throw new Error(`Build ${buildId} not found`); | ||
| } | ||
| if (error.response?.status === 403) { | ||
| throw new Error(`Insufficient permissions to cancel build ${buildId}`); | ||
| } | ||
| if (error.response?.status === 400) { | ||
| throw new Error(`Build ${buildId} cannot be cancelled (already completed or not cancellable)`); | ||
| } | ||
| throw new Error(`Failed to cancel build ${buildId}: ${error.message}`); |
There was a problem hiding this comment.
Use safer error type checking and avoid any.
The catch block uses error: any and accesses error.response?.status without verifying the error is an axios error. If the error comes from network failure, timeout, or retry exhaustion, error.response may be undefined, leading to incorrect error messages or undefined behavior.
Apply this diff to use proper type guards:
- } catch (error: any) {
+ } catch (error: unknown) {
// Handle specific error cases
- if (error.response?.status === 404) {
+ if (axios.isAxiosError(error) && error.response?.status === 404) {
throw new Error(`Build ${buildId} not found`);
}
- if (error.response?.status === 403) {
+ if (axios.isAxiosError(error) && error.response?.status === 403) {
throw new Error(`Insufficient permissions to cancel build ${buildId}`);
}
- if (error.response?.status === 400) {
+ if (axios.isAxiosError(error) && error.response?.status === 400) {
throw new Error(`Build ${buildId} cannot be cancelled (already completed or not cancellable)`);
}
- throw new Error(`Failed to cancel build ${buildId}: ${error.message}`);
+ const message = error instanceof Error ? error.message : String(error);
+ throw new Error(`Failed to cancel build ${buildId}: ${message}`);
}🤖 Prompt for AI Agents
In src/api/azure/services/azure-pipeline.service.ts around lines 191 to 202, the
catch block types the error as any and directly accesses error.response?.status;
change the catch parameter to error: unknown and use a proper type guard (e.g.,
axios.isAxiosError(error) or check (error as AxiosError)?.response) before
reading response.status, handle cases where response is undefined
(network/timeout) by throwing an appropriate message using error?.message or
JSON.stringify(error) as fallback, and avoid casting blindy to any — ensure
non-Axios errors fall through to a generic failure error with the original
message included.
| await retry( | ||
| async () => { | ||
| await this.octokit.actions.cancelWorkflowRun({ | ||
| owner, | ||
| repo, | ||
| run_id: runId, | ||
| }); | ||
| }, | ||
| { | ||
| retries: this.maxRetries, | ||
| minTimeout: this.minTimeout, | ||
| maxTimeout: this.maxTimeout, | ||
| factor: this.factor, | ||
| onRetry: (error: Error, attempt: number) => { | ||
| console.log( | ||
| `[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}` | ||
| ); | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`); | ||
| } catch (error: any) { | ||
| // Handle specific error cases | ||
| if (error.status === 404) { | ||
| throw new GithubNotFoundError( | ||
| 'workflow run', | ||
| `${runId} in repository ${owner}/${repo}` | ||
| ); | ||
| } | ||
|
|
||
| if (error.status === 403) { | ||
| throw new GithubApiError( | ||
| `Insufficient permissions to cancel workflow run ${runId}`, | ||
| error.status | ||
| ); | ||
| } | ||
|
|
||
| if (error.status === 409) { | ||
| throw new GithubApiError( | ||
| `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`, | ||
| error.status | ||
| ); | ||
| } | ||
|
|
||
| // Re-throw with more context | ||
| throw new GithubApiError( | ||
| `Failed to cancel workflow run ${runId}: ${error.message}`, | ||
| error.status || 500, | ||
| error | ||
| ); | ||
| } |
There was a problem hiding this comment.
Use bail() to exit retry early for non-retryable errors.
The current implementation retries all errors, including 404 (not found), 403 (permissions), and 409 (already completed). These errors are non-retryable and should use the bail() function to exit early, as demonstrated in other methods (see lines 221, 280, 338). Retrying these errors wastes time and resources.
Apply this diff:
try {
await retry(
- async () => {
+ async (bail) => {
await this.octokit.rest.actions.cancelWorkflowRun({
owner,
repo,
run_id: runId,
});
+ },
+ {
+ retries: this.maxRetries,
+ minTimeout: this.minTimeout,
+ maxTimeout: this.maxTimeout,
+ factor: this.factor,
+ onRetry: (error: Error, attempt: number) => {
+ console.log(
+ `[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}`
+ );
+ },
+ }
+ );
+
+ console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`);
+ } catch (error: any) {
+ // Handle specific error cases - bail immediately for non-retryable errors
+ if (error.status === 404) {
+ throw new GithubNotFoundError(
+ 'workflow run',
+ `${runId} in repository ${owner}/${repo}`
+ );
+ }
+
+ if (error.status === 403) {
+ throw new GithubApiError(
+ `Insufficient permissions to cancel workflow run ${runId}`,
+ error.status
+ );
+ }
+
+ if (error.status === 409) {
+ throw new GithubApiError(
+ `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
+ error.status
+ );
+ }
+
+ // Re-throw with more context
+ throw new GithubApiError(
+ `Failed to cancel workflow run ${runId}: ${error.message}`,
+ error.status || 500,
+ error
+ );
+ }should be:
try {
await retry(
async (bail) => {
- await this.octokit.rest.actions.cancelWorkflowRun({
- owner,
- repo,
- run_id: runId,
- });
+ try {
+ await this.octokit.rest.actions.cancelWorkflowRun({
+ owner,
+ repo,
+ run_id: runId,
+ });
+ } catch (error: any) {
+ // Bail immediately for non-retryable errors
+ if (error.status === 404) {
+ bail(new GithubNotFoundError(
+ 'workflow run',
+ `${runId} in repository ${owner}/${repo}`,
+ error.status
+ ));
+ return;
+ }
+
+ if (error.status === 403) {
+ bail(new GithubApiError(
+ `Insufficient permissions to cancel workflow run ${runId}`,
+ error.status
+ ));
+ return;
+ }
+
+ if (error.status === 409) {
+ bail(new GithubApiError(
+ `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
+ error.status
+ ));
+ return;
+ }
+
+ // Throw for retryable errors
+ throw error;
+ }
},
{
retries: this.maxRetries,
minTimeout: this.minTimeout,
maxTimeout: this.maxTimeout,
factor: this.factor,
onRetry: (error: Error, attempt: number) => {
console.log(
`[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}`
);
},
}
);
console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`);
} catch (error: any) {
- // Handle specific error cases
- if (error.status === 404) {
- throw new GithubNotFoundError(
- 'workflow run',
- `${runId} in repository ${owner}/${repo}`
- );
- }
-
- if (error.status === 403) {
- throw new GithubApiError(
- `Insufficient permissions to cancel workflow run ${runId}`,
- error.status
- );
- }
-
- if (error.status === 409) {
- throw new GithubApiError(
- `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
- error.status
- );
- }
-
- // Re-throw with more context
+ // Add context to unknown errors
throw new GithubApiError(
`Failed to cancel workflow run ${runId}: ${error.message}`,
error.status || 500,
error
);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/api/github/services/github-actions.service.ts around lines 610-661, the
retry wrapper currently retries all errors including non-retryable 404/403/409;
modify the async function passed to retry to detect these statuses and call
bail(...) with the appropriate GithubNotFoundError or GithubApiError (matching
the existing error messages) so retry aborts immediately for those cases, and
only let other errors throw to trigger retry; keep the existing outer catch as a
fallback for unexpected errors.
| try { | ||
| await retry( | ||
| async () => { | ||
| await this.octokit.actions.cancelWorkflowRun({ |
There was a problem hiding this comment.
Fix the API path to include .rest.
All other methods in this service use this.octokit.rest.actions.* pattern (see lines 93, 100, 265, 321, 389, 517), but this line uses this.octokit.actions.* without the .rest segment. This inconsistency will likely cause a runtime error.
Apply this diff:
- await this.octokit.actions.cancelWorkflowRun({
+ await this.octokit.rest.actions.cancelWorkflowRun({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.octokit.actions.cancelWorkflowRun({ | |
| await this.octokit.rest.actions.cancelWorkflowRun({ |
🤖 Prompt for AI Agents
In src/api/github/services/github-actions.service.ts around line 612, the call
uses this.octokit.actions.cancelWorkflowRun which is inconsistent with the rest
of the file and will fail at runtime; change it to use
this.octokit.rest.actions.cancelWorkflowRun so it matches the other Octokit REST
calls (keep the same arguments and error handling).
| } catch (error) { | ||
| // If build is not found or already stopped, throw specific error | ||
| throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName); | ||
| } |
There was a problem hiding this comment.
Preserve non-404 failures from stopBuild
Catching every failure and rethrowing JenkinsBuildNotFoundError makes a 403/500 look like a simple “build not found,” which is painful for diagnosing flaky cancellations in our E2E runs. Please only map actual 404s to JenkinsBuildNotFoundError and let other errors bubble so the caller can see the real cause.
- } catch (error) {
- // If build is not found or already stopped, throw specific error
- throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
- }
+ } catch (error) {
+ const status =
+ (error as { statusCode?: number })?.statusCode ??
+ (error as { response?: { status?: number } })?.response?.status;
+
+ if (status === 404) {
+ throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
+ }
+
+ throw error;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| // If build is not found or already stopped, throw specific error | |
| throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName); | |
| } | |
| } catch (error) { | |
| const status = | |
| (error as { statusCode?: number })?.statusCode ?? | |
| (error as { response?: { status?: number } })?.response?.status; | |
| if (status === 404) { | |
| throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In src/api/jenkins/services/jenkins-build.service.ts around lines 181 to 184,
the catch block unconditionally converts every error from stopBuild into
JenkinsBuildNotFoundError; change it to detect a real 404 and only throw
JenkinsBuildNotFoundError for that case (e.g. check error.response?.status ===
404 or error.status === 404 depending on the HTTP client) and for any other
status or unexpected error rethrow the original error so the caller can observe
the true failure and stack.
| private normalizeOptions( | ||
| options?: CancelPipelineOptions | ||
| ): Required<Omit<CancelPipelineOptions, 'eventType' | 'branch'>> & Pick<CancelPipelineOptions, 'eventType' | 'branch'> { | ||
| return { | ||
| excludePatterns: options?.excludePatterns || [], | ||
| includeCompleted: options?.includeCompleted || false, | ||
| eventType: options?.eventType, | ||
| branch: options?.branch, | ||
| concurrency: options?.concurrency || 10, | ||
| dryRun: options?.dryRun || false, | ||
| }; |
There was a problem hiding this comment.
Clamp concurrency to keep cancellation reliable.
If someone passes a negative concurrency, chunkArray(..., options.concurrency) spins forever, hanging the cancel flow and any tests that rely on it. Clamping to at least 1 keeps the new API safe without surprising callers.
- concurrency: options?.concurrency || 10,
+ concurrency: Math.max(1, options?.concurrency ?? 10),🤖 Prompt for AI Agents
In src/rhtap/core/integration/ci/providers/gitlabCI.ts around lines 348 to 358,
the normalizeOptions function currently passes negative concurrency through
which can cause chunkArray(..., concurrency) to spin forever; clamp the
concurrency value to a minimum of 1 when building the returned object (e.g., use
Math.max(1, options?.concurrency ?? 10) or similar) so concurrency is never
below 1 while keeping the default of 10 and preserving other fields.
| private normalizeOptions( | ||
| options?: CancelPipelineOptions | ||
| ): Required<Omit<CancelPipelineOptions, 'eventType' | 'branch'>> & Pick<CancelPipelineOptions, 'eventType' | 'branch'> { | ||
| return { | ||
| excludePatterns: options?.excludePatterns || [], | ||
| includeCompleted: options?.includeCompleted || false, | ||
| eventType: options?.eventType, | ||
| branch: options?.branch, | ||
| concurrency: options?.concurrency || 10, | ||
| dryRun: options?.dryRun || false, | ||
| }; |
There was a problem hiding this comment.
Prevent negative concurrency from stalling cancellations.
Here too, a negative concurrency propagates into chunkArray, resulting in a non-terminating loop and stuck cleanup. Let’s clamp to at least 1 so Jenkins-based runs don’t hang.
- concurrency: options?.concurrency || 10,
+ concurrency: Math.max(1, options?.concurrency ?? 10),🤖 Prompt for AI Agents
In src/rhtap/core/integration/ci/providers/jenkinsCI.ts around lines 643 to 653,
normalizeOptions currently passes a possibly negative concurrency through to
chunkArray which can cause an infinite loop; clamp the computed concurrency to a
minimum of 1 (e.g. compute the defaulted value then apply Math.max(1, ...)) so
concurrency is never less than 1 before returning the options.
| // 4. Apply filters | ||
| const pipelineRunsToCancel = this.filterPipelineRuns(allPipelineRuns, opts); | ||
|
|
||
| console.log(`[Tekton] ${pipelineRunsToCancel.length} PipelineRuns match filters`); | ||
| console.log(`[Tekton] ${allPipelineRuns.length - pipelineRunsToCancel.length} PipelineRuns filtered out`); | ||
|
|
||
| // 5. Cancel PipelineRuns in batches | ||
| await this.cancelPipelineRunsInBatches(pipelineRunsToCancel, opts, result); | ||
|
|
There was a problem hiding this comment.
Update skipped counter to match filtered PipelineRuns.
Just like the GitHub provider, we never increment result.skipped for runs dropped by filters. That misleads our cancellation telemetry and test assertions. Capture the delta right after filtering:
- const pipelineRunsToCancel = this.filterPipelineRuns(allPipelineRuns, opts);
+ const pipelineRunsToCancel = this.filterPipelineRuns(allPipelineRuns, opts);
+ result.skipped += allPipelineRuns.length - pipelineRunsToCancel.length;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 4. Apply filters | |
| const pipelineRunsToCancel = this.filterPipelineRuns(allPipelineRuns, opts); | |
| console.log(`[Tekton] ${pipelineRunsToCancel.length} PipelineRuns match filters`); | |
| console.log(`[Tekton] ${allPipelineRuns.length - pipelineRunsToCancel.length} PipelineRuns filtered out`); | |
| // 5. Cancel PipelineRuns in batches | |
| await this.cancelPipelineRunsInBatches(pipelineRunsToCancel, opts, result); | |
| // 4. Apply filters | |
| const pipelineRunsToCancel = this.filterPipelineRuns(allPipelineRuns, opts); | |
| result.skipped += allPipelineRuns.length - pipelineRunsToCancel.length; | |
| console.log(`[Tekton] ${pipelineRunsToCancel.length} PipelineRuns match filters`); | |
| console.log(`[Tekton] ${allPipelineRuns.length - pipelineRunsToCancel.length} PipelineRuns filtered out`); | |
| // 5. Cancel PipelineRuns in batches | |
| await this.cancelPipelineRunsInBatches(pipelineRunsToCancel, opts, result); |
🤖 Prompt for AI Agents
In src/rhtap/core/integration/ci/providers/tektonCI.ts around lines 447 to 455,
after applying filters you fail to increment result.skipped for the PipelineRuns
dropped by filtering; compute the number filtered (allPipelineRuns.length -
pipelineRunsToCancel.length) and add that delta to result.skipped immediately
after filtering (before logging/cancelling) so telemetry/tests reflect the
skipped count correctly.
| private matchesEventType(pipelineRun: any, eventType: EventType): boolean { | ||
| const tektonEventType = pipelineRun.metadata?.labels?.['pipelinesascode.tekton.dev/event-type']; | ||
|
|
||
| if (!tektonEventType) { | ||
| return true; // If no event type label, allow all | ||
| } | ||
|
|
||
| switch (eventType) { | ||
| case EventType.PUSH: | ||
| return tektonEventType === 'push'; | ||
| case EventType.PULL_REQUEST: | ||
| return tektonEventType === 'pull_request'; | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Honor explicit event-type filters.
When callers request a specific EventType, we must not cancel runs whose Tekton label is missing; returning true here silently widens the scope and breaks targeted clean-up tests. Please treat the absence of pipelinesascode.tekton.dev/event-type as a mismatch (skip) unless includeCompleted or other options explicitly override it.
🤖 Prompt for AI Agents
In src/rhtap/core/integration/ci/providers/tektonCI.ts around lines 608-623, the
matchesEventType function currently treats a missing
pipelinesascode.tekton.dev/event-type label as a match (returning true); change
this so a missing label is treated as a mismatch (return false) so explicit
EventType filters are honored. If there is a legitimate case to allow missing
labels, add an explicit override flag (e.g. includeCompleted or
allowMissingEventType) to the function signature and use it to allow the old
behavior only when explicitly set; update call sites and tests accordingly.
| // It is possible to trigger multiple pipelines when a new component is created and make some changes | ||
| // to the both source and gitops repos. These pipelines are not needed for the test and should be cancelled. | ||
| await ci.cancelAllPipelines(); | ||
| console.log('All pipelines have been cancelled!'); | ||
| console.log('Component creation is complete!'); |
There was a problem hiding this comment.
Assert cancelAllPipelines result for reliability.
We’re swallowing the CancelResult, so a failed cancellation would still let this workflow test pass, masking real regressions in the new cancellation path. Let’s assert the failure count so the suite stays trustworthy.
- await ci.cancelAllPipelines();
- console.log('All pipelines have been cancelled!');
+ const cancelResult = await ci.cancelAllPipelines();
+ expect(cancelResult.failed).toBe(0);
+ console.log(`All pipelines cancelled: ${cancelResult.cancelled}/${cancelResult.total}`);
console.log('Component creation is complete!');🤖 Prompt for AI Agents
In tests/tssc/full_workflow.test.ts around lines 71 to 75, the call to await
ci.cancelAllPipelines() ignores its CancelResult so cancellation failures are
swallowed; capture the returned result into a variable and add an assertion that
result.failed === 0 (or equivalent test assertion) so the test fails when any
pipeline cancellation did not succeed, optionally including the result in the
assertion message for debugging.
|
@xinredhat: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-2ll28Test results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
2 participants
Assisted-by: Claude Code
Summary by CodeRabbit
Release Notes