RHTAP-2547 Add import test #114
Conversation
WalkthroughAdds import-focused Playwright E2E test and testplan, updates npm test scripts and Playwright projects, and introduces repository/file existence and deletion APIs with retry/backoff across GitHub/GitLab/Bitbucket providers and related clients/services; also adds DeveloperHub entity deletion with retries and timeouts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI
participant PW as Playwright
participant API as TSSC API
participant Prov as Git Provider
participant Git as Git Host API
participant DH as DeveloperHub
participant CD as ArgoCD
CI->>PW: npm run test:import
PW->>API: create component (template)
API-->>PW: repo URL + commit SHA
PW->>CD: poll provisioning & sync
CD-->>PW: synced
PW->>Prov: checkIfRepositoryExists / checkIfFileExistsInRepository
Prov->>Git: GET repo / contents
Git-->>Prov: 200 / 404
Prov-->>PW: exists / not-found
PW->>Prov: deleteFolderInRepository / deleteFileInRepository
Prov->>Git: list & delete (recursive)
Git-->>Prov: 2xx
Prov-->>PW: delete success
PW->>DH: deleteEntitiesBySelector(selector)
DH->>DH: GET /catalog/entities (retry)
DH->>DH: DELETE /by-uid/{id} (per-entity retry)
DH-->>PW: aggregated result
PW->>API: create import-task
API-->>PW: imported component created
PW->>CD: wait for sync
CD-->>PW: synced
PW->>Prov: verify imported repo & catalog-info.yaml
Prov->>Git: GET repo/contents
Git-->>Prov: 200
Prov-->>PW: verified
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Points needing extra attention:
Constructive suggestions (testing reliability, security, maintainability):
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)src/rhtap/core/**⚙️ CodeRabbit configuration file
Files:
src/rhtap/core/integration/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📚 Learning: 2025-08-19T06:47:38.630ZApplied to files:
📚 Learning: 2025-07-31T07:43:28.355ZApplied to files:
🔇 Additional comments (5)
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 review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
api-client
configuration
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
package.json(1 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)tests/templates/import-templates.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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-repository.service.tssrc/api/rhdh/developerhub.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/templates/import-templates.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/templates/import-templates.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/git/providers/github.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/git/providers/github.ts
🧠 Learnings (1)
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
package.json
⏰ 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 (2)
src/rhtap/core/integration/git/providers/github.ts (1)
697-707: Repository existence check is correct and keeps unexpected errors visible.Catching 404 as non-existence and rethrowing others is the right tradeoff for reliability and debuggability.
tests/templates/import-templates.test.ts (1)
36-41: Template presence smoke-check is acceptable for now.Minimal assertion keeps suite fast; later we can query the catalog directly for determinism.
jkopriva
force-pushed
the
RHTAP-2547
branch
3 times, most recently
from
cdbd0fc to
de9f3a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (4)
src/api/rhdh/developerhub.ts (2)
208-211: Clarify log message ('entities' vs 'components' and not 'description').Message currently mentions “components” and “description,” which doesn’t match selector semantics. Slightly confusing during triage.
Apply this diff:
- console.log(`No components found in catalog with the description containing "${selector}".`); + console.log(`No entities found in catalog matching selector "${selector}".`);
195-206: Filter out entities without UIDs to prevent DELETE /by-uid/undefined.The current filter doesn’t check for metadata.uid presence. Attempting to delete with undefined UID will fail and reduce cleanup reliability.
Apply this diff:
- const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + const allEntities: any[] = Array.isArray(response.data) ? response.data : []; + const filteredEntities = allEntities + .filter((entity: any) => + (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || + (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || + (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) + ) + .filter((entity: any) => Boolean(entity?.metadata?.uid));src/rhtap/core/integration/git/providers/github.ts (2)
717-727: Distinguish files vs directories when checking file existence.getContent can return a directory listing (array). Current implementation treats any successful response as a file, leading to false positives.
Apply this diff:
- public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { - try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; - } catch (error: any) { - if (error.status === 404) { - return false; - } - throw error; - } - } + public async checkIfFileExistsInRepository( + owner: string, + repoName: string, + filePath: string, + branch: string = 'main' + ): Promise<boolean> { + try { + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(content)) { + return false; // directory + } + return (content as any).type === 'file' || 'content' in (content as any); + } catch (error: any) { + if (error.status === 404) { + return false; + } + throw error; + } + }This avoids misreporting directories as files and improves test assertions.
738-755: Guard explicitly for file type before accessing SHAAlthough all current callers only pass file paths (and the existing
!('sha' in fileContent)check will reject directory responses), adding an explicit type check makes the intent crystal-clear and future-proofs against accidental directory deletes.Suggested update in
src/rhtap/core/integration/git/providers/github.ts:- if (!fileContent || !('sha' in fileContent)) { - throw new Error(`File ${filePath} not found or is not a file`); - } + if (Array.isArray(fileContent) || fileContent.type !== 'file' || !fileContent.sha) { + throw new Error(`Path ${filePath} is not a file`); + }
- Catches directory-array responses up front
- Improves readability by checking
type === 'file'instead of relying solely onshapresence- Makes error messages clearer when someone later tries to delete a folder by mistake
With this small refactor, your cleanup logic stays rock-solid and self-documenting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
playwright.config.ts
⚙️ CodeRabbit Configuration File
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.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/templates/import_templates.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/templates/import_templates.test.ts
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-repository.service.tssrc/api/rhdh/developerhub.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/git/providers/github.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/git/providers/github.ts
🧠 Learnings (2)
📚 Learning: 2025-08-19T06:47:38.581Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.581Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/github.ts
📚 Learning: 2025-08-18T07:39:58.810Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.810Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
package.json
🧬 Code Graph Analysis (1)
tests/templates/import_templates.test.ts (4)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(14-253)src/rhtap/core/integration/git/gitInterface.ts (1)
Git(12-116)src/playwright/testItem.ts (1)
TestItem(6-120)
🔇 Additional comments (12)
playwright.config.ts (3)
19-19: Good feature flagging for import tests.ENABLE_IMPORT_TESTS gating is clear and aligns with the new script. This helps keep CI focused and reproducible.
77-86: Make import tests reliably serial; avoid parallelism-induced flakiness and rate limits.Import flows hit external systems (GitHub, Backstage, Argo). Running multiple import projects concurrently can cause cross-test interference and GH rate limiting. Serialize import tests when ENABLE_IMPORT_TESTS is true.
- Option A (minimal): reduce global workers when import tests are enabled.
- Option B: gate workers by env (e.g., IMPORT_TEST_WORKERS=1) and default to 1 for imports.
I can draft the config change if you’d like.
[ suggest_essential_refactor ]
[ offer_assistance ]
88-93: Project aggregation looks correct.Ordering import projects after auth/e2e/ui is sensible and keeps the suite structure coherent.
package.json (2)
19-19: No behavioral change to test:tssc.Looks consistent with existing patterns. Nothing to action here.
20-20: ✅test:importScript Validated and Ready
- Confirmed
tests/templates/import_templates.test.tsexists and is specifically targeted.- Flags (
ENABLE_E2E_TESTS=false,ENABLE_UI_TESTS=false,ENABLE_IMPORT_TESTS=true) properly isolate the import suite.- Preceding
npm run generate-configensures environment consistency and guards against missing configuration.This mirrors the established E2E pattern, keeps runs deterministic, and maintains testing reliability and security. Great work!
src/api/github/services/github-repository.service.ts (2)
226-251: deleteFile implementation is correct and aligned with Octokit usage.
- Parameters match GitHub API expectations.
- Default branch to 'main' aids ergonomics.
- Logging is adequate for test diagnostics.
No changes requested.
259-273: Repository deletion flow is straightforward and consistent with existing error handling.Behavior matches other service methods; keeping uniform error propagation aids maintainability and predictable test failures.
src/rhtap/core/integration/git/providers/github.ts (2)
697-707: Repository existence check logic is sound.404 -> false, rethrow others is appropriate for robust diagnostics while keeping the happy-path fast.
797-806: Deletion method is consistent with service behavior; keep as-is.Uniform error propagation across deletion methods (per maintainer preference) keeps behavior predictable during test cleanup.
tests/templates/import_templates.test.ts (3)
61-69: LGTM: deterministic wait for scaffolding completionUses the framework’s waiter and guards against missing component. No concerns.
194-211: Re-check provider API use here as well (organization/owner)Ensure consistency with the earlier verification on repository/file checks for the imported component.
Run the earlier script and update this block to use the confirmed owner accessor (likely
git.getRepoOwner()), mirroring any changes applied above.
206-212: Add robust afterAll cleanup for repos and Backstage entitiesAvoid leaving stray repos/entities between runs. Cleanup should be best-effort and non-fatal.
console.log(`Imported repository ${importedComponentName} and catalog-info.yaml verified in GitHub`); }); -}); + + test.afterAll(async () => { + const targets = [importedComponent, component].filter(Boolean) as Component[]; + for (const c of targets) { + const name = c.getName(); + const gh = c.getGit() as GithubProvider; + const owner = typeof (gh as any).getOrganization === 'function' + ? (gh as any).getOrganization() + : gh.getRepoOwner(); + // Best-effort GitHub repo cleanup + try { + await gh.deleteRepository(owner, name); + console.log(`Cleanup: deleted repository ${owner}/${name}`); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + console.warn(`Cleanup: failed to delete repository ${owner}/${name}: ${msg}`); + } + // Best-effort Backstage entity cleanup + try { + const dh = c.getDeveloperHub(); + const selector = `kind=Component,metadata.name=${name}`; + await dh.deleteEntitiesBySelector(selector); + console.log(`Cleanup: deleted Backstage entities for ${name}`); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + console.warn(`Cleanup: failed to delete Backstage entities for ${name}: ${msg}`); + } + } + }); +});⛔ Skipped due to learnings
Learnt from: jkopriva PR: redhat-appstudio/tssc-test#114 File: tests/templates/import-templates.test.ts:138-168 Timestamp: 2025-08-19T06:42:15.437Z 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.
| const test = createBasicFixture(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate the suite behind ENABLE_IMPORT_TESTS to avoid destructive runs by default
Prevent accidental GitHub/Backstage mutations on local or unrelated CI runs.
Apply this diff right after the fixture is created:
const test = createBasicFixture();
+// Skip the entire suite unless explicitly enabled
+if (process.env.ENABLE_IMPORT_TESTS !== 'true') {
+ test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to enable this suite.');
+}📝 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.
| const test = createBasicFixture(); | |
| const test = createBasicFixture(); | |
| // Skip the entire suite unless explicitly enabled | |
| if (process.env.ENABLE_IMPORT_TESTS !== 'true') { | |
| test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to enable this suite.'); | |
| } |
🤖 Prompt for AI Agents
In tests/templates/import_templates.test.ts around lines 18-19, immediately
after the const test = createBasicFixture(); line add a runtime guard that
checks the ENABLE_IMPORT_TESTS environment variable and, if it is not set to
"true", skips the import test suite (by returning early or invoking the test
framework's skip mechanism) and emits a short warning; this prevents the
destructive import tests from running by default in local or unrelated CI
environments.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playwright.config.ts (1)
104-121: Set CI retries at the config level and keep artifact reporting stable.A single retry in CI materially improves E2E stability without masking persistent issues.
export default defineConfig({ testDir: './tests', testMatch: '**/*.test.ts', workers: DEFAULT_WORKERS, timeout: DEFAULT_TIMEOUT, fullyParallel: false, // This should allow immediate execution when dependencies are met + retries: process.env.CI ? 1 : 0, projects: allProjects.length ? allProjects : [{ name: 'default' }], @@ reporter: [ ['html', { open: 'never', outputFolder: 'playwright-report' }], ['list'], ['junit', { outputFile: 'test-results/junit.xml' }], ],
♻️ Duplicate comments (18)
playwright.config.ts (1)
42-46: Fix project testMatch patterns (they are relative to testDir, drop leading 'tests/').With testDir set to './tests', using 'tests/...' prevents Playwright from discovering your import/UI/E2E tests under their subfolders. This will silently skip tests, including the new import suite.
Apply this diff to all project overrides:
@@ - testMatch: 'tests/tssc/**/*.test.ts', + testMatch: 'tssc/**/*.test.ts', @@ - testMatch: 'tests/ui/**/*.test.ts', + testMatch: 'ui/**/*.test.ts', @@ - testMatch: 'tests/templates/**/*.test.ts', + testMatch: 'templates/**/*.test.ts',#!/bin/bash set -euo pipefail echo "Configured testDir:" rg -nP '^\s*testDir\s*:' playwright.config.ts echo -e "\nProject-level testMatch values:" rg -nC2 "testMatch:" playwright.config.ts echo -e "\nList import tests under tests/templates:" fd -t f -E node_modules '.+\.test\.ts$' tests/templates | sed 's|^| - |'Also applies to: 54-61, 83-89
src/api/rhdh/developerhub.ts (3)
208-212: Fix misleading log message and return value when nothing matches.The message mentions “description” and “components,” but you match by name/target across multiple kinds.
- if (filteredEntities.length === 0) { - console.log(`No components found in catalog with the description containing "${selector}".`); - return false; - } + if (filteredEntities.length === 0) { + console.log(`No entities found in catalog matching selector "${selector}".`); + return false; + }
213-216: Add lightweight retries to mitigate eventual consistency and transient 5xx.Backstage catalog operations occasionally lag; retry each delete a couple of times.
- // Delete all filtered entities - const results = await Promise.all( - filteredEntities.map((entity: any) => this.unregisterEntityByUid(entity.metadata.uid)) - ); + // Delete all filtered entities with lightweight retries + const results = await Promise.all( + filteredEntities.map((entity: any) => + retry(async () => this.unregisterEntityByUid(entity.metadata.uid), { + retries: 2, + minTimeout: 1000, + maxTimeout: 2000, + }) + ) + );
199-206: Avoid over-deletes and undefined UID calls; filter robustly and handle non-array data.Using includes() on names/targets is coarse and you don’t guard for missing metadata.uid, which can lead to DELETE /by-uid/undefined. Also ensure response.data is an array.
- // Get all entities and filter them based on the selector - const response = await this.axios.get(`${this.url}/api/catalog/entities`); - - const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + // Get all entities and filter them based on the selector (substring match) + const response = await this.axios.get(`${this.url}/api/catalog/entities`); + const allEntities: any[] = Array.isArray(response.data) ? response.data : []; + const filteredEntities = allEntities + .filter((entity: any) => + (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || + (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || + (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) + ) + // Only delete entities that have a UID + .filter((entity: any) => Boolean(entity?.metadata?.uid));tests/templates/import_templates.test.ts (12)
18-19: Gate the entire suite behind ENABLE_IMPORT_TESTS to prevent destructive runs by default.const test = createBasicFixture(); +// Skip destructive import tests unless explicitly enabled +if (process.env.ENABLE_IMPORT_TESTS !== 'true') { + test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to enable this suite.'); +}
13-13: Remove unused Git import to avoid cross-test shared state.-import { Git } from '../../src/rhtap/core/integration/git';
30-35: Avoid top-level mutable git and make template configurable via env; configure suite-level timeout/retries.test.describe.serial('Import Template Tests', () => { - let component: Component; - let importedComponent: Component; - let git: Git; - const templateName = 'go'; // You can change this to test different templates + let component: Component; + let importedComponent: Component; + const templateName = process.env.TEMPLATE_NAME ?? 'go'; + + test.describe.configure({ + timeout: 45 * 60_000, // 45 minutes for end-to-end provisioning/sync + retries: process.env.CI ? 1 : 0, + });
36-41: Drop the placeholder “template exists” test — it adds no signal.- test(`verifies if ${templateName} template exists in the catalog`, async ({ testItem }) => { - // This would require implementing a method to get golden path templates - // For now, we'll assume the template exists if we can create a component - expect(templateName).toBeDefined(); - console.log(`Template ${templateName} is available for testing`); - });
55-57: Sanitize error logging to avoid leaking tokens/headers.- } catch (error) { - console.error(`❌ Failed to create component: ${error}`); - throw error; - } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create component: ${message}`); + throw error; + }
71-84: Mark long ArgoCD wait as slow to improve reporter fidelity.test(`waits for ${templateName} argocd to be synced in the cluster`, async () => { + test.slow(); if (!component) { throw new Error('Component was not created successfully'); }
91-100: Use provider-agnostic owner API and avoid casting to GithubProvider.- const git = component.getGit() as GithubProvider; + const git = component.getGit(); const componentName = component.getName(); @@ - const repositoryExists = await git.checkIfRepositoryExists(git.getOrganization(), componentName); + const repositoryExists = await git.checkIfRepositoryExists(git.getRepoOwner(), componentName); @@ - const catalogFileExists = await git.checkIfFileExistsInRepository(git.getOrganization(), componentName, 'catalog-info.yaml'); + const catalogFileExists = await git.checkIfFileExistsInRepository(git.getRepoOwner(), componentName, 'catalog-info.yaml');
110-120: Harden deletion step: tolerate missing .tekton, check file exists before delete, fix log message.- const git = component.getGit() as GithubProvider; + const git = component.getGit(); const componentName = component.getName(); - // Delete .tekton folder - await git.deleteFolderInRepository(git.getOrganization(), componentName, '.tekton'); + const owner = git.getRepoOwner(); + + // Best-effort delete .tekton folder (may be absent) + try { + await git.deleteFolderInRepository(owner, componentName, '.tekton'); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + console.warn(`.tekton delete skipped/failed for ${componentName}: ${msg}`); + } // Delete catalog-info.yaml file - await git.deleteFileInRepository(git.getOrganization(), componentName, 'catalog-info.yaml'); + const hasCatalog = await git.checkIfFileExistsInRepository(owner, componentName, 'catalog-info.yaml'); + if (hasCatalog) { + await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); + } else { + console.warn(`catalog-info.yaml not found in ${componentName}, skipping delete`); + } - console.log(`Deleted .tekton, gitops folders and catalog-info.yaml from ${componentName}`); + console.log(`Deleted .tekton folder and catalog-info.yaml from ${componentName}`);
122-134: Pass an explicit Backstage selector for deterministic cleanup.- // Delete entities from Developer Hub - const deleted = await developerHub.deleteEntitiesBySelector(componentName); + // Delete entities from Developer Hub (explicit selector) + const selector = `kind=Component,metadata.name=${componentName}`; + const deleted = await developerHub.deleteEntitiesBySelector(selector); expect(deleted).toBeTruthy();Note: If you adopt this, consider teaching deleteEntitiesBySelector to parse/handle explicit selectors; or keep the current method and pass the bare name string. See related API comment in src/api/rhdh/developerhub.ts.
163-165: Sanitize error logging for imported component creation as well.- } catch (error) { - console.error(`❌ Failed to create imported component: ${error}`); - throw error; - } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create imported component: ${message}`); + throw error; + }
178-192: Mark the imported ArgoCD wait as slow.test(`waits for imported component argocd to be synced in the cluster`, async () => { + test.slow(); if (!importedComponent) { throw new Error('Imported component was not created successfully'); }
199-208: Use provider-agnostic owner for imported repo checks.- const git = importedComponent.getGit() as GithubProvider; + const git = importedComponent.getGit(); const importedComponentName = importedComponent.getName(); @@ - const repositoryExists = await git.checkIfRepositoryExists(git.getOrganization(), importedComponentName); + const repositoryExists = await git.checkIfRepositoryExists(git.getRepoOwner(), importedComponentName); @@ - const catalogFileExists = await git.checkIfFileExistsInRepository(git.getOrganization(), importedComponentName, 'catalog-info.yaml'); + const catalogFileExists = await git.checkIfFileExistsInRepository(git.getRepoOwner(), importedComponentName, 'catalog-info.yaml');src/rhtap/core/integration/git/providers/github.ts (2)
717-727: Avoid false positives: treat directory responses as non-files.getContent returns an array for directories. The current implementation will return true for directories, which can mask test setup mistakes (e.g., asserting catalog-info.yaml exists when only the folder is present). Please gate on “file” responses.
Apply this diff:
- public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { - try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; - } catch (error: any) { - if (error.status === 404) { - return false; - } - throw error; - } - } + public async checkIfFileExistsInRepository( + owner: string, + repoName: string, + filePath: string, + branch: string = 'main' + ): Promise<boolean> { + try { + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + // Directory -> array; File -> object with type === 'file' (or has 'content') + if (Array.isArray(content)) { + return false; + } + return (content as any).type === 'file' || 'content' in (content as any); + } catch (error: any) { + if (error.status === 404) { + return false; + } + throw error; + } + }Follow-up: add an assertion in tests to verify that providing a directory path returns false, improving E2E reliability for import-template checks.
766-789: Optional: replace recursive per-file commits with a single batched commit to avoid rate limits.Current approach creates one commit per file, which is slow and brittle under CI concurrency. Building a single tree that omits the folder and committing once materially improves reliability and performance.
If/when convenient, consider implementing folder removal via your GithubClient’s git-data APIs (createTree/createCommit/updateRef) and wiring a single call here (keeping your existing error semantics intact).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🧰 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/rhdh/developerhub.tssrc/api/github/services/github-repository.service.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.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/git/providers/github.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/git/providers/github.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (2)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/github.ts
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
playwright.config.tspackage.json
🧬 Code graph analysis (2)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
tests/templates/import_templates.test.ts (3)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(14-253)src/playwright/testItem.ts (1)
TestItem(6-120)
🔇 Additional comments (5)
package.json (1)
19-21: Import test script wiring looks good.Runs generate-config and sets flags appropriately to isolate import tests.
src/api/rhdh/developerhub.ts (1)
237-251: Delete helper looks correct; accepting any 2xx avoids false negatives.The rename and 2xx success handling improve clarity and robustness.
src/api/github/services/github-repository.service.ts (1)
259-273: Repository deletion implementation is straightforward and consistent.Clear logs and uniform error propagation via GithubApiError; no issues spotted.
src/rhtap/core/integration/git/providers/github.ts (2)
697-707: Repo existence check: behavior is correct and consistent.Returns true on success, false on 404, rethrows others — aligns with the integration error-handling pattern and is appropriate for E2E guards.
797-806: Repository deletion flow is clear and consistent.Straightforward call + logging, rethrow on error — matches the class’s error-handling pattern used elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (23)
playwright.config.ts (1)
81-90: Fix import project testMatch; add a small retry budget (optional).
- testMatch is resolved relative to testDir ('./tests'), so the leading 'tests/' causes discovery misses.
- A light retry helps dampen transient GitHub/Backstage hiccups during imports.
Apply this diff:
importProjects = projectConfigs.map(config => ({ name: `import-${config.name}`, - testMatch: 'tests/templates/**/*.test.ts', + testMatch: 'templates/**/*.test.ts', use: { testItem: config.testItem, }, + retries: process.env.CI ? 1 : 0, }));If the import flow needs an authenticated session, declare a dependency on your auth setup project:
importProjects = projectConfigs.map(config => ({ name: `import-${config.name}`, testMatch: 'templates/**/*.test.ts', use: { testItem: config.testItem }, + // dependencies: ['auth-setup'], retries: process.env.CI ? 1 : 0, }));package.json (1)
20-20: Drop the explicit file path; rely on project testMatch once config is fixed.
After updating testMatch in playwright.config.ts, this script can run the import suite without hardcoding the file path.Apply this diff:
- "test:import": "npm run generate-config && ENABLE_E2E_TESTS=false ENABLE_UI_TESTS=false ENABLE_IMPORT_TESTS=true playwright test tests/templates/import_templates.test.ts" + "test:import": "npm run generate-config && ENABLE_E2E_TESTS=false ENABLE_UI_TESTS=false ENABLE_IMPORT_TESTS=true playwright test"src/api/rhdh/developerhub.ts (4)
225-227: Sanitize error logs to avoid leaking sensitive details.
Log only the message string.Apply this diff:
- console.error(`Error deleting entities with selector ${selector}:`, error); + const msg = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entities with selector ${selector}: ${msg}`);
208-211: Fix misleading log message.
The log mentions “description” but filtering is by name/target.Apply this diff:
- console.log(`No components found in catalog with the description containing "${selector}".`); + console.log(`No entities found in catalog matching selector "${selector}".`);
213-216: Retry deletes to handle transient 409/429/5xx.
Parallel deletes can race with catalog writes. Add a small retry around each delete.Apply this diff:
- const results = await Promise.all( - filteredEntities.map((entity: any) => this.unregisterEntityByUid(entity.metadata.uid)) - ); + const results = await Promise.all( + filteredEntities.map((entity: any) => + retry(async () => this.unregisterEntityByUid(entity.metadata.uid), { retries: 2, minTimeout: 300, maxTimeout: 500 }) + ) + );
200-216: Guard against non-array payloads, avoid undefined UIDs, and make matching safer.
- response.data may not be an array; guard it.
- Filter out entities lacking metadata.uid to avoid DELETE .../by-uid/undefined.
- Using includes() on names can over-delete; use exact match for names (keep includes for Location target if desired).
Apply this diff:
- const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + const all: any[] = Array.isArray(response.data) ? response.data : []; + const sel = selector?.toLowerCase?.() ?? ''; + const filteredEntities = all + .filter((entity: any) => + (entity.kind === 'Component' && entity.metadata?.name?.toLowerCase?.() === sel) || + (entity.kind === 'Resource' && entity.metadata?.name?.toLowerCase?.() === sel) || + (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) + ) + .filter((entity: any) => Boolean(entity?.metadata?.uid));src/api/github/services/github-repository.service.ts (1)
226-251: Validate inputs early for clearer failures and safer deletes.
Fail fast with actionable messages if required params are missing; keeps errors deterministic in tests.Apply this diff:
public async deleteFile( owner: string, repo: string, path: string, message: string, sha: string, branch: string = 'main' ): Promise<void> { - try { + // Basic guards + if (!owner || !repo) { + throw new GithubApiError('Owner and repo are required for deleteFile'); + } + if (!path) { + throw new GithubApiError('Path is required for deleteFile'); + } + if (!sha) { + throw new GithubApiError(`SHA is required to delete ${path} from ${owner}/${repo}`); + } + if (!message?.trim()) { + throw new GithubApiError('Commit message is required for deleteFile'); + } + try { console.log(`Deleting file ${path} from ${owner}/${repo}`);src/rhtap/core/integration/git/providers/github.ts (3)
717-727: Avoid false positives: treat directories as non-files.
getContent returns arrays for directories; only return true for actual files.Apply this diff:
public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(content)) { + return false; + } + return (content as any)?.type === 'file' || 'content' in (content as any); } catch (error: any) { if (error.status === 404) { return false; } throw error; } }
738-755: Validate target is a file before deleting; improve error message.
When content is a directory array, sha isn’t present; make the failure explicit.Apply this diff:
- const fileContent = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - - if (!fileContent || !('sha' in fileContent)) { - throw new Error(`File ${filePath} not found or is not a file`); - } + const fileContent = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(fileContent) || !fileContent || !('sha' in fileContent)) { + throw new Error(`Path ${filePath} is not a file or does not exist`); + } // Delete the file using the GitHub API - await this.githubClient.repository.deleteFile(owner, repoName, filePath, commitMessage, fileContent.sha, branch); + await this.githubClient.repository.deleteFile(owner, repoName, filePath, commitMessage, (fileContent as any).sha, branch);
766-789: Make folder deletion idempotent; treat 404 as already deleted.
Improves cleanup reliability in CI where retries are common. Optional batching to a single commit can follow later.Apply this diff:
public async deleteFolderInRepository(owner: string, repoName: string, folderPath: string, branch: string = 'main', commitMessage: string = 'Delete folder'): Promise<void> { try { // Get the contents of the folder - const folderContents = await this.githubClient.repository.getContent(owner, repoName, folderPath, branch); + const folderContents = await this.githubClient.repository.getContent(owner, repoName, folderPath, branch); if (!Array.isArray(folderContents)) { throw new Error(`Path ${folderPath} is not a folder`); } @@ console.log(`Successfully deleted folder ${folderPath} from ${owner}/${repoName}`); } catch (error: any) { - console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); - throw error; + if (error?.status === 404) { + console.log(`Folder ${folderPath} not found in ${owner}/${repoName}; treating as already deleted.`); + return; + } + console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error?.message ?? String(error)}`); + throw error; } }Optional follow-up: batch deletions into a single commit for large folders to reduce rate-limit risk.
tests/templates/import_templates.test.ts (13)
18-20: Guard the suite behind ENABLE_IMPORT_TESTS to prevent destructive runs by defaultThese tests mutate GitHub and Backstage. They should be opt-in to avoid accidental data loss in local/CI.
Apply right after creating the fixture:
const test = createBasicFixture(); +// Skip the entire suite unless explicitly enabled +if (process.env.ENABLE_IMPORT_TESTS !== 'true') { + test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to enable this suite.'); +}
30-30: Configure generous timeouts and CI retries for long-running E2EArgo sync and cluster ops can be slow; make the suite resilient and bounded.
test.describe.serial('Import Template Tests', () => { + test.describe.configure({ + timeout: 45 * 60_000, // 45 minutes for end-to-end provisioning/sync + retries: process.env.CI ? 1 : 0, + });
11-14: Drop provider-specific and unused imports to reduce coupling and leakage riskThe GithubProvider and Git imports are unnecessary if you use the provider-agnostic Git interface returned by component.getGit() where needed. This also avoids accidental reliance on GitHub-only methods.
-import { GithubProvider } from '../../src/rhtap/core/integration/git/providers/github'; import { expect } from '@playwright/test'; -import { Git } from '../../src/rhtap/core/integration/git';
31-35: Eliminate shared mutable git state and parameterize template via env
- Top-level mutable git encourages cross-test state leakage; use local consts per test.
- Make the template configurable for flexibility across environments.
let component: Component; let importedComponent: Component; - let git: Git; - const templateName = 'go'; // You can change this to test different templates + const templateName = process.env.TEMPLATE_NAME ?? 'go'; // override with TEMPLATE_NAME
36-41: Remove placeholder “template exists” test or replace with a real catalog checkAsserting toBeDefined() on a constant provides no signal and can mask issues.
Minimal fix: delete the test.
- test(`verifies if ${templateName} template exists in the catalog`, async ({ testItem }) => { - // This would require implementing a method to get golden path templates - // For now, we'll assume the template exists if we can create a component - expect(templateName).toBeDefined(); - console.log(`Template ${templateName} is available for testing`); - });If you want a real check, I can wire a DeveloperHub.listGoldenPathTemplates() helper and assert presence.
55-57: Sanitize error logging to avoid leaking tokens/headersLogging raw Error objects can include provider responses with secrets.
- } catch (error) { - console.error(`❌ Failed to create component: ${error}`); - throw error; - } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create component: ${message}`); + throw error; + }
71-84: Mark long ArgoCD waits as slow and avoid shared git mutationImproves reporter fidelity and removes reliance on a shared top-level git variable.
test(`waits for ${templateName} argocd to be synced in the cluster`, async () => { + test.slow(); if (!component) { throw new Error('Component was not created successfully'); } - git = component.getGit(); + const git = component.getGit(); const commitSha = await git.getGitOpsRepoCommitSha(); const cd = component.getCD();
86-103: Use provider-agnostic API (getRepoOwner) and avoid GithubProvider castKeeps tests portable across providers and trims type-casts.
- const git = component.getGit() as GithubProvider; + const git = component.getGit(); const componentName = component.getName(); // Check if repository exists in GitHub - const repositoryExists = await git.checkIfRepositoryExists(git.getOrganization(), componentName); + const repositoryExists = await git.checkIfRepositoryExists(git.getRepoOwner(), componentName); expect(repositoryExists).toBe(true); // Check if catalog-info.yaml exists - const catalogFileExists = await git.checkIfFileExistsInRepository(git.getOrganization(), componentName, 'catalog-info.yaml'); + const catalogFileExists = await git.checkIfFileExistsInRepository(git.getRepoOwner(), componentName, 'catalog-info.yaml'); expect(catalogFileExists).toBe(true);
105-120: Harden deletion step, tolerate absent items, and fix misleading log message
- .tekton may not exist; don’t fail the suite.
- Check for catalog-info.yaml before deleting.
- Log should not claim gitops folder deletion here.
- const git = component.getGit() as GithubProvider; + const git = component.getGit(); const componentName = component.getName(); - // Delete .tekton folder - await git.deleteFolderInRepository(git.getOrganization(), componentName, '.tekton'); - - // Delete catalog-info.yaml file - await git.deleteFileInRepository(git.getOrganization(), componentName, 'catalog-info.yaml'); - - console.log(`Deleted .tekton, gitops folders and catalog-info.yaml from ${componentName}`); + const owner = git.getRepoOwner(); + + // Best-effort delete .tekton folder + try { + await git.deleteFolderInRepository(owner, componentName, '.tekton'); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + console.warn(`.tekton delete skipped/failed for ${componentName}: ${msg}`); + } + + // Delete catalog-info.yaml only if present + const hasCatalog = await git.checkIfFileExistsInRepository(owner, componentName, 'catalog-info.yaml'); + if (hasCatalog) { + await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); + } else { + console.warn(`catalog-info.yaml not found in ${componentName}, skipping delete`); + } + + console.log(`Deleted .tekton folder and catalog-info.yaml from ${componentName}`);
122-134: Pass an explicit Backstage entity selector (kind + name) for deterministic cleanupA bare name risks matching nothing or too much.
const developerHub = component.getDeveloperHub(); const componentName = component.getName(); // Delete entities from Developer Hub - const deleted = await developerHub.deleteEntitiesBySelector(componentName); + const selector = `kind=Component,metadata.name=${componentName}`; + const deleted = await developerHub.deleteEntitiesBySelector(selector); expect(deleted).toBeTruthy();
162-165: Sanitize error logging during import flowMirror earlier sanitization to reduce secret leakage.
- } catch (error) { - console.error(`❌ Failed to create imported component: ${error}`); - throw error; - } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create imported component: ${message}`); + throw error; + }
178-187: Mark imported ArgoCD sync as slow and use local git constConsistency with the earlier wait and better readability.
test(`waits for imported component argocd to be synced in the cluster`, async () => { + test.slow(); if (!importedComponent) { throw new Error('Imported component was not created successfully'); } // Wait for ArgoCD application to be healthy - git = importedComponent.getGit(); + const git = importedComponent.getGit(); const commitSha = await git.getGitOpsRepoCommitSha(); const cd = importedComponent.getCD();
199-209: Use provider-agnostic API for imported repo verificationAvoid casts and prefer getRepoOwner().
- const git = importedComponent.getGit() as GithubProvider; + const git = importedComponent.getGit(); const importedComponentName = importedComponent.getName(); // Check if imported repository exists in GitHub - const repositoryExists = await git.checkIfRepositoryExists(git.getOrganization(), importedComponentName); + const repositoryExists = await git.checkIfRepositoryExists(git.getRepoOwner(), importedComponentName); expect(repositoryExists).toBe(true); // Check if catalog-info.yaml exists in imported repository - const catalogFileExists = await git.checkIfFileExistsInRepository(git.getOrganization(), importedComponentName, 'catalog-info.yaml'); + const catalogFileExists = await git.checkIfFileExistsInRepository(git.getRepoOwner(), importedComponentName, 'catalog-info.yaml'); expect(catalogFileExists).toBe(true);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
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-repository.service.tssrc/api/rhdh/developerhub.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/git/providers/github.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/git/providers/github.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (2)
📚 Learning: 2025-08-18T07:39:58.821Z
Learnt from: CR
PR: redhat-appstudio/tssc-test#0
File: README.md:0-0
Timestamp: 2025-08-18T07:39:58.821Z
Learning: Applies to tests/ui/ui.test.ts : Keep the main UI automation test file at tests/ui/ui.test.ts
Applied to files:
playwright.config.tspackage.json
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (2)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
tests/templates/import_templates.test.ts (3)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(14-253)src/playwright/testItem.ts (1)
TestItem(6-120)
⏰ 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 (6)
playwright.config.ts (2)
19-19: Good toggle for import tests; default-off is sane for CI.
Keeps the suite opt-in and avoids surprise runtime. No action needed.
95-96: Project ordering is fine; confirm import doesn’t require auth/e2e dependencies.
Since fullyParallel=false, dependencies are honored when declared. If imports rely on prior auth/e2e, wire them explicitly via dependencies as above; otherwise, current ordering is OK.Would you like me to wire dependencies for imports based on your intended flow?
package.json (1)
19-19: test:tssc looks consistent with other E2E scripts.
Includes generate-config and flags; good for reliability and reproducibility.src/rhtap/core/integration/git/providers/github.ts (2)
697-707: Repository existence check reads well.
Straightforward 404→false behavior is appropriate and consistent.
797-806: Repository delete aligns with project preference to throw on errors.
Consistent error semantics across provider methods. No changes from me.tests/templates/import_templates.test.ts (1)
105-134: Add deterministic cleanup to prevent resource drift (optional but recommended)Follow-through cleanup reduces flakes between runs and avoids orphaned repos/entities.
- After verifying the imported component, consider deleting the imported Backstage entity and (optionally) the imported repo if your provider supports it.
- Also consider asserting that catalog-info.yaml is absent after the deletion step to validate the import preconditions.
Example additions (outside current ranges):
// After the deletion test, assert absence before import: test('verifies catalog-info.yaml is absent before import', async () => { const git = component.getGit(); const owner = git.getRepoOwner(); const name = component.getName(); const exists = await git.checkIfFileExistsInRepository(owner, name, 'catalog-info.yaml'); expect(exists).toBe(false); }); // Global cleanup test.afterAll(async () => { try { if (importedComponent) { const git = importedComponent.getGit(); const owner = git.getRepoOwner(); const repo = importedComponent.getName(); // Best-effort entity cleanup await importedComponent.getDeveloperHub().deleteEntitiesBySelector(`kind=Component,metadata.name=${repo}`); // Best-effort repo cleanup if provider supports it if (typeof (git as any).deleteRepository === 'function') { await (git as any).deleteRepository(owner, repo); } } } catch (e) { const msg = e instanceof Error ? e.message : String(e); console.warn(`Cleanup skipped/failed: ${msg}`); } });If you want, I can open a follow-up PR wiring a provider-agnostic deleteRepository into the Git abstraction.
Also applies to: 194-211
⛔ Skipped due to learnings
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.
jkopriva
force-pushed
the
RHTAP-2547
branch
2 times, most recently
from
8ccc96f to
a7406e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
playwright.config.ts (1)
82-90: Fix import project testMatch so the suite actually runsWith
testDir: './tests', Playwright resolves each project’stestMatchrelative to that directory. Keeping the leadingtests/means none of the import specs will be discovered, so the new projects never execute. Please drop the redundant segment (and align other project overrides the same way).- testMatch: 'tests/templates/**/*.test.ts', + testMatch: 'templates/**/*.test.ts',src/api/rhdh/developerhub.ts (1)
225-227: Scrub sensitive data from deletion error logs
console.error(..., error)will dump full Axios responses (cookies, tokens). Convert to a safe string before logging.- } catch (error) { - console.error(`Error deleting entities with selector ${selector}:`, error); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entities with selector ${selector}: ${message}`); return false; }tests/templates/import_templates.test.ts (2)
183-193: Also sanitize imported-component error loggingMirror the sanitized logging here so imported component failures don’t leak secrets:
- } catch (error) { - console.error(`❌ Failed to create imported component: ${error}`); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create imported component: ${message}`); throw error; }
50-60: Sanitize error logging in test catch blocks
console.error(... ${error})will stringify tokens/headers from provider failures, violating our “no secrets in test logs” guideline. Log only a safe message (e.g.,error instanceof Error ? error.message : String(error)) and keep the original error for rethrow. Apply the same treatment to the imported-component catch below.- } catch (error) { - console.error(`❌ Failed to create component: ${error}`); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create component: ${message}`); throw error; }src/rhtap/core/integration/git/providers/github.ts (1)
717-727: Distinguish files from directories in existence check.
getContentreturns an array for directories and an object for files. The current implementation returnstruefor both, which can cause false positives when checking file existence.Apply this diff to validate the result is a file:
public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + // Array indicates directory; file has 'type' === 'file' or 'content' field + if (Array.isArray(content)) { + return false; + } + return (content as any).type === 'file' || 'content' in (content as any); } catch (error: any) { if (error.status === 404) { return false; } throw error; } }
🧹 Nitpick comments (3)
tests/templates/import_templates.test.ts (2)
32-90: Skip destructive suite unless explicitly enabledThese tests create and tear down real resources. Without a runtime guard, anyone invoking the file directly (or forgetting to export ENABLE_IMPORT_TESTS) will mutate live systems unintentionally. Add a top-level skip conditioned on
process.env.ENABLE_IMPORT_TESTS === 'true'so local and unrelated CI runs stay safe. Example:const test = createBasicFixture(); +if (process.env.ENABLE_IMPORT_TESTS !== 'true') { + test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to run this suite.'); +}
38-43: Drop the no-op “template exists” testAsserting
expect(templateName).toBeDefined()never fails and doesn’t validate the catalog. Please remove this placeholder or replace it with a real catalog lookup so the suite remains signal-rich.src/rhtap/core/integration/git/providers/github.ts (1)
738-755: Optional: Improve error message for directory paths.While the SHA check will fail for directories (arrays), an explicit array check would provide a clearer error message without changing the error handling semantics.
Apply this diff:
const fileContent = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(fileContent)) { + throw new Error(`Path ${filePath} is a directory, not a file`); + } + if (!fileContent || !('sha' in fileContent)) { throw new Error(`File ${filePath} not found or is not a file`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (8)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (7)
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.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/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/github.tssrc/rhtap/core/integration/git/providers/gitlab.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/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/github.tssrc/rhtap/core/integration/git/providers/gitlab.ts
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/rhdh/developerhub.tssrc/api/github/services/github-repository.service.ts
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (2)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
tests/templates/import_templates.test.ts (3)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/playwright/testItem.ts (1)
TestItem(6-120)
⏰ 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 (5)
src/rhtap/core/integration/git/providers/github.ts (3)
697-707: LGTM: Existence check correctly handles 404.The method correctly returns
falsefor 404 (not found) and rethrows other errors. This pattern is appropriate for existence checks.
766-789: LGTM: Recursive deletion logic is correct.The implementation correctly handles recursive deletion of folder contents. The one-commit-per-file approach was previously noted as a performance consideration for follow-up batching optimization.
797-806: LGTM: Deletion follows consistent error handling.The implementation correctly deletes the repository and maintains consistent error handling throughout the provider class, per the established pattern.
Based on learnings.
src/api/github/services/github-repository.service.ts (2)
226-251: LGTM: File deletion implements consistent error handling.The method correctly wraps the Octokit API call with logging and error handling that matches the service's established patterns.
259-273: LGTM: Repository deletion follows service patterns.The implementation is correct and maintains consistency with the service's error handling and logging approach.
Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/api/rhdh/developerhub.ts (2)
202-216: Tighten selector filtering to avoid collateral deletions.Using
includeshere makes the cleanup path non-deterministic; any entity whose name merely contains the selector will be purged, and entities lacking a UID will still be sent to/by-uid/undefined. Both undermine test reliability and can wipe the wrong catalog entries. Switch to exact matches (or a consciously stricter predicate) and drop anything without a UID before invoking the DELETE call.- const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + const allEntities: any[] = Array.isArray(response.data) ? response.data : []; + const filteredEntities = allEntities + .filter((entity: any) => Boolean(entity?.metadata?.uid)) + .filter( + (entity: any) => + (entity.kind === 'Component' && entity.metadata?.name === selector) || + (entity.kind === 'Resource' && entity.metadata?.name === selector) || + (entity.kind === 'Location' && entity.spec?.target === selector) + );
225-227: Sanitize error logging to keep credentials out of test logs.The raw axios error object can contain headers (incl. Authorization) and full request payloads. Please log only a safe message string and leave the original error as
.causeif you need to troubleshoot offline.- } catch (error) { - console.error(`Error deleting entities with selector ${selector}:`, error); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entities with selector ${selector}: ${message}`); return false; } … - } catch (error) { - console.error(`Error deleting entity with UID ${id}:`, error); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entity with UID ${id}: ${message}`); return false; }Also applies to: 247-249
tests/templates/import_templates.test.ts (2)
57-59: Sanitize error logging to prevent leaking sensitive data.Logging raw error objects can expose tokens and headers from HTTP errors. Extract and log only the error message.
Apply this diff:
} catch (error) { - console.error(`❌ Failed to create component: ${error}`); + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create component: ${message}`); throw error; }As per coding guidelines: "Do not print secrets in logs within tests".
190-192: Sanitize error logging to prevent leaking sensitive data.Logging raw error objects can expose tokens and headers. Extract and log only the error message.
Apply this diff:
} catch (error) { - console.error(`❌ Failed to create imported component: ${error}`); + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create imported component: ${message}`); throw error; }As per coding guidelines: "Do not print secrets in logs within tests".
🧹 Nitpick comments (8)
tests/templates/import_templates.test.ts (8)
20-36: Add test suite safeguards to prevent accidental runs and improve reliability.The suite lacks several protections that would improve stability:
- No
ENABLE_IMPORT_TESTSguard to prevent accidental destructive operations- No generous timeout configuration for slow E2E operations
- Hardcoded template name instead of env-driven configuration
- Top-level mutable state (
git,component,importedComponent) can leak between test rerunsApply these changes:
const test = createBasicFixture(); + +// Skip the entire suite unless explicitly enabled +if (process.env.ENABLE_IMPORT_TESTS !== 'true') { + test.skip(true, 'Import tests disabled. Set ENABLE_IMPORT_TESTS=true to enable.'); +} test.describe.serial('Import Template Tests', () => { + test.describe.configure({ + timeout: 45 * 60_000, // 45 minutes for E2E provisioning/sync + retries: process.env.CI ? 1 : 0, + }); + let component: Component; let importedComponent: Component; - let git: Git; - const templateName = 'go'; + const templateName = process.env.TEMPLATE_NAME ?? 'go';
38-43: Remove placeholder test or implement real template validation.Asserting that a hardcoded constant
toBeDefined()provides no test signal. Either remove this test entirely or implement a real catalog query to verify the template exists in Developer Hub.Minimal fix—remove the test:
- test(`verifies if ${templateName} template exists in the catalog`, async () => { - // This would require implementing a method to get golden path templates - // For now, we'll assume the template exists if we can create a component - expect(templateName).toBeDefined(); - console.log(`Template ${templateName} is available for testing`); - });
73-86: Consider marking long ArgoCD waits as slow.Optional: calling
test.slow()at the start helps test reporters and CI systems identify expected slow tests, improving triage visibility.test(`waits for ${templateName} argocd to be synced in the cluster`, async () => { + test.slow(); if (!component) {
88-118: Use provider-agnosticgetRepoOwner()instead of provider-specific methods.The test currently uses
instanceofchecks and provider-specific methods (getOrganization(),getGroup(),getWorkspace()). Usinggit.getRepoOwner()simplifies the code and makes it provider-agnostic.Apply this diff:
const git = component.getGit(); const componentName = component.getName(); - const gitType = git.getGitType(); - - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner(); // Check if repository exists const repositoryExists = await git.checkIfRepositoryExists(owner, componentName); expect(repositoryExists).toBe(true); // Check if catalog-info.yaml exists const catalogFileExists = await git.checkIfFileExistsInRepository(owner, componentName, 'catalog-info.yaml'); expect(catalogFileExists).toBe(true); - console.log(`Repository ${componentName} and catalog-info.yaml verified in ${gitType}`); + console.log(`Repository ${componentName} and catalog-info.yaml verified in ${git.getGitType()}`);This same pattern should be applied to other tests at lines 120-148 and 222-252.
120-148: Simplify owner resolution and fix misleading log message.
- Use
git.getRepoOwner()instead of provider-specific instanceof checks- Log message mentions "gitops folders" but the code doesn't delete gitops—update to match actual deletions
Apply this diff:
const git = component.getGit(); const componentName = component.getName(); - const gitType = git.getGitType(); - - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner(); // Delete .tekton folder await git.deleteFolderInRepository(owner, componentName, '.tekton'); // Delete catalog-info.yaml file await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); - console.log(`Deleted .tekton, gitops folders and catalog-info.yaml from ${componentName} in ${gitType}`); + console.log(`Deleted .tekton folder and catalog-info.yaml from ${componentName}`);
150-162: Use qualified Backstage selector for deterministic entity deletion.Passing the bare component name may not match entities reliably. Construct a qualified selector with
kindandmetadata.nameto ensure precise targeting.Apply this diff:
const developerHub = component.getDeveloperHub(); const componentName = component.getName(); - // Delete entities from Developer Hub - const deleted = await developerHub.deleteEntitiesBySelector(componentName); + // Delete entities from Developer Hub with qualified selector + const selector = `kind=Component,metadata.name=${componentName}`; + const deleted = await developerHub.deleteEntitiesBySelector(selector); expect(deleted).toBeTruthy(); console.log(`Deleted entities for ${componentName} from Developer Hub`);
206-220: Consider marking long ArgoCD waits as slow.Optional: calling
test.slow()helps identify expected slow tests for better CI triage.test(`waits for imported component argocd to be synced in the cluster`, async () => { + test.slow(); if (!importedComponent) {
222-252: Use provider-agnosticgetRepoOwner()instead of provider-specific methods.This is the third occurrence of the same pattern. Simplify by using
git.getRepoOwner().Apply this diff:
const git = importedComponent.getGit(); const importedComponentName = importedComponent.getName(); - const gitType = git.getGitType(); - - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner(); // Check if imported repository exists const repositoryExists = await git.checkIfRepositoryExists(owner, importedComponentName); expect(repositoryExists).toBe(true); // Check if catalog-info.yaml exists in imported repository const catalogFileExists = await git.checkIfFileExistsInRepository(owner, importedComponentName, 'catalog-info.yaml'); expect(catalogFileExists).toBe(true); - console.log(`Imported repository ${importedComponentName} and catalog-info.yaml verified in ${gitType}`); + console.log(`Imported repository ${importedComponentName} and catalog-info.yaml verified in ${git.getGitType()}`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (8)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/rhtap/core/integration/git/providers/bitbucket.ts
- playwright.config.ts
- src/api/github/services/github-repository.service.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/git/providers/github.tssrc/rhtap/core/integration/git/providers/gitlab.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/git/providers/github.tssrc/rhtap/core/integration/git/providers/gitlab.ts
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
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/rhdh/developerhub.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (1)
tests/templates/import_templates.test.ts (7)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/rhtap/core/integration/git/gitInterface.ts (1)
Git(12-118)src/rhtap/core/integration/git/providers/github.ts (1)
GithubProvider(11-807)src/rhtap/core/integration/git/providers/gitlab.ts (1)
GitlabProvider(15-1000)src/rhtap/core/integration/git/providers/bitbucket.ts (1)
BitbucketProvider(16-1058)src/playwright/testItem.ts (1)
TestItem(6-120)
🔇 Additional comments (9)
src/rhtap/core/integration/git/providers/gitlab.ts (4)
902-918: LGTM!The repository existence check correctly uses GitLab's project API and appropriately returns
falsefor 404/not-found responses while propagating other errors.
920-939: LGTM!File existence check correctly retrieves the project ID before checking file content, and appropriately handles 404s by returning
false.
941-962: LGTM!File deletion correctly retrieves the project ID and uses GitLab's delete API with appropriate error logging.
964-999: LGTM!Folder deletion correctly uses GitLab's repository tree API to recursively delete files and subdirectories. The one-commit-per-file pattern is consistent with the GitHub provider implementation.
src/rhtap/core/integration/git/providers/github.ts (5)
691-707: LGTM!Repository existence check correctly uses GitHub's repository API and appropriately returns
falsefor 404 responses while propagating other errors.
709-727: LGTM!File existence check returns
truewhen content is retrievable andfalsefor 404s. Implementation is consistent with the provider's design patterns.
729-755: LGTM!File deletion correctly retrieves the file SHA before deletion and includes appropriate error handling with detailed diagnostics.
757-789: LGTM!Folder deletion correctly uses GitHub's content API to recursively delete files and subdirectories. Error handling is consistent with the maintainer's preferred pattern of uniform error propagation. Based on learnings.
791-806: LGTM!Repository deletion correctly uses GitHub's API with uniform error handling. The consistent error propagation approach aligns with the maintainer's design preference. Based on learnings.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
tests/templates/import_templates.test.ts (2)
93-117: Use provider-agnosticgetRepoOwner()to reduce driftDuplicating owner lookup across providers makes maintenance harder and forces casts. The shared Git interface already exposes
getRepoOwner(). Use that instead of branching on provider type.- const git = component.getGit(); - const componentName = component.getName(); - const gitType = git.getGitType(); - - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const git = component.getGit(); + const componentName = component.getName(); + const gitType = git.getGitType(); + const owner = git.getRepoOwner(); @@ - const git = importedComponent.getGit(); - const importedComponentName = importedComponent.getName(); - const gitType = git.getGitType(); - - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const git = importedComponent.getGit(); + const importedComponentName = importedComponent.getName(); + const gitType = git.getGitType(); + const owner = git.getRepoOwner();Also applies to: 227-249
20-33: Gate the destructive import suite behind ENABLE_IMPORT_TESTSThese tests create/delete real repos and Backstage entities. Without an opt-in guard they’ll fire on every
testrun, which is dangerous for local dev and unrelated CI jobs. Please skip the entire describe unlessENABLE_IMPORT_TESTS=true.-const test = createBasicFixture(); +const test = createBasicFixture(); +const describeImport = + process.env.ENABLE_IMPORT_TESTS === 'true' ? test.describe.serial : test.describe.skip; -test.describe.serial('Import Template Tests', () => { +describeImport('Import Template Tests', () => {src/api/rhdh/developerhub.ts (1)
199-216: Lock selector matching to exact entities before deletingUsing
.includes(selector)and skipping any UID guard meansdeleteEntitiesBySelector('foo')can wipe unrelated catalog entries (e.g., components namedfoo-imported) and may call/by-uid/undefined. That’s a reliability and safety regression for cleanup flows. Please restrict matching to exact identifiers and only issue DELETEs when a UID is present.- const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + const allEntities: any[] = Array.isArray(response.data) ? response.data : []; + const filteredEntities = allEntities + .filter((entity: any) => Boolean(entity?.metadata?.uid)) + .filter( + (entity: any) => + ((entity.kind === 'Component' || entity.kind === 'Resource') && + entity.metadata?.name === selector) || + (entity.kind === 'Location' && entity.spec?.target === selector) + );src/rhtap/core/integration/git/providers/github.ts (1)
717-727: File vs directory check still missing.Past review identified that
getContentreturns an array for directories and an object for files, but the current implementation treats any successful response as a file. This causes false positives when the path points to a directory.Apply this diff to distinguish files from directories:
public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + // getContent returns an array for directories, an object for files + if (Array.isArray(content)) { + return false; + } + // Verify it's actually a file by checking for the 'sha' or 'content' property + return 'sha' in content || 'content' in content; } catch (error: any) { if (error.status === 404) { return false; } throw error; } }
🧹 Nitpick comments (1)
src/rhtap/core/integration/git/providers/github.ts (1)
738-755: Consider adding retry logic for transient failures.While the deletion logic is correct, file deletions can fail due to rate limits (429), concurrent modifications (409), or transient server errors (5xx). If the underlying
githubClientdoesn't handle retries, consider adding exponential backoff here to improve E2E test reliability.Based on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (8)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- playwright.config.ts
- package.json
🧰 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-repository.service.tssrc/api/rhdh/developerhub.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/git/providers/github.tssrc/rhtap/core/integration/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/gitlab.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/git/providers/github.tssrc/rhtap/core/integration/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/gitlab.ts
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (2)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
tests/templates/import_templates.test.ts (7)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/rhtap/core/integration/git/gitInterface.ts (1)
Git(12-118)src/rhtap/core/integration/git/providers/github.ts (1)
GithubProvider(11-807)src/rhtap/core/integration/git/providers/gitlab.ts (1)
GitlabProvider(15-1000)src/rhtap/core/integration/git/providers/bitbucket.ts (1)
BitbucketProvider(16-1058)src/playwright/testItem.ts (1)
TestItem(6-120)
🔇 Additional comments (2)
src/rhtap/core/integration/git/providers/github.ts (2)
697-707: LGTM! Clean existence check pattern.The repository existence check correctly handles the 404 case to return false while propagating other errors.
797-806: Implementation aligns with error handling consistency preference.The repository deletion correctly follows the uniform error handling pattern established across other methods in this class.
Based on learnings
| public async deleteFileInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { | ||
| try { | ||
| // Delete the file using Bitbucket API | ||
| await this.bitbucketClient.repositories.deleteFile(owner, repoName, filePath, branch, commitMessage); | ||
|
|
||
| console.log(`Successfully deleted file ${filePath} from ${owner}/${repoName}`); | ||
| } catch (error: any) { | ||
| console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Deletes a folder from a repository by deleting all files in the folder | ||
| * @param owner The repository owner (workspace) | ||
| * @param repoName The repository name | ||
| * @param folderPath The path to the folder to delete | ||
| * @param branch The branch to delete from (default: 'main') | ||
| * @param commitMessage The commit message for the deletion | ||
| * @returns Promise<void> | ||
| */ | ||
| public async deleteFolderInRepository(owner: string, repoName: string, folderPath: string, branch: string = 'main', commitMessage: string = 'Delete folder'): Promise<void> { | ||
| try { | ||
| // Get the contents of the folder | ||
| const folderContents = await this.bitbucketClient.repositories.getDirectoryContent(owner, repoName, folderPath, branch); | ||
|
|
||
| if (!Array.isArray(folderContents)) { | ||
| throw new Error(`Path ${folderPath} is not a folder`); | ||
| } | ||
|
|
||
| // Delete each file in the folder | ||
| for (const item of folderContents) { | ||
| if (item.type === 'file') { | ||
| await this.deleteFileInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | ||
| } else if (item.type === 'directory') { | ||
| // Recursively delete subdirectories | ||
| await this.deleteFolderInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | ||
| } | ||
| } | ||
|
|
||
| console.log(`Successfully deleted folder ${folderPath} from ${owner}/${repoName}`); | ||
| } catch (error: any) { | ||
| console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Handle missing Bitbucket files/folders gracefully during cleanup
deleteFileInRepository and deleteFolderInRepository rethrow on 404, so rerunning the import flow after partial cleanup will fail immediately. For these destructive E2E tests, treat “not found” as success and only throw on unexpected errors.
- console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`);
- throw error;
+ if (error.status === 404 || error.message?.includes('not found')) {
+ console.warn(`File ${filePath} already absent in ${owner}/${repoName}, skipping delete`);
+ return;
+ }
+ console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`);
+ throw error;
@@
- console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`);
- throw error;
+ if (error.status === 404 || error.message?.includes('not found')) {
+ console.warn(`Folder ${folderPath} already absent in ${owner}/${repoName}, skipping delete`);
+ return;
+ }
+ console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`);
+ 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.
| public async deleteFileInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { | |
| try { | |
| // Delete the file using Bitbucket API | |
| await this.bitbucketClient.repositories.deleteFile(owner, repoName, filePath, branch, commitMessage); | |
| console.log(`Successfully deleted file ${filePath} from ${owner}/${repoName}`); | |
| } catch (error: any) { | |
| console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`); | |
| throw error; | |
| } | |
| } | |
| /** | |
| * Deletes a folder from a repository by deleting all files in the folder | |
| * @param owner The repository owner (workspace) | |
| * @param repoName The repository name | |
| * @param folderPath The path to the folder to delete | |
| * @param branch The branch to delete from (default: 'main') | |
| * @param commitMessage The commit message for the deletion | |
| * @returns Promise<void> | |
| */ | |
| public async deleteFolderInRepository(owner: string, repoName: string, folderPath: string, branch: string = 'main', commitMessage: string = 'Delete folder'): Promise<void> { | |
| try { | |
| // Get the contents of the folder | |
| const folderContents = await this.bitbucketClient.repositories.getDirectoryContent(owner, repoName, folderPath, branch); | |
| if (!Array.isArray(folderContents)) { | |
| throw new Error(`Path ${folderPath} is not a folder`); | |
| } | |
| // Delete each file in the folder | |
| for (const item of folderContents) { | |
| if (item.type === 'file') { | |
| await this.deleteFileInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | |
| } else if (item.type === 'directory') { | |
| // Recursively delete subdirectories | |
| await this.deleteFolderInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | |
| } | |
| } | |
| console.log(`Successfully deleted folder ${folderPath} from ${owner}/${repoName}`); | |
| } catch (error: any) { | |
| console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); | |
| throw error; | |
| } | |
| public async deleteFileInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { | |
| try { | |
| // Delete the file using Bitbucket API | |
| await this.bitbucketClient.repositories.deleteFile(owner, repoName, filePath, branch, commitMessage); | |
| console.log(`Successfully deleted file ${filePath} from ${owner}/${repoName}`); | |
| } catch (error: any) { | |
| if (error.status === 404 || error.message?.includes('not found')) { | |
| console.warn(`File ${filePath} already absent in ${owner}/${repoName}, skipping delete`); | |
| return; | |
| } | |
| console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`); | |
| throw error; | |
| } | |
| } | |
| /** | |
| * Deletes a folder from a repository by deleting all files in the folder | |
| * @param owner The repository owner (workspace) | |
| * @param repoName The repository name | |
| * @param folderPath The path to the folder to delete | |
| * @param branch The branch to delete from (default: 'main') | |
| * @param commitMessage The commit message for the deletion | |
| * @returns Promise<void> | |
| */ | |
| public async deleteFolderInRepository(owner: string, repoName: string, folderPath: string, branch: string = 'main', commitMessage: string = 'Delete folder'): Promise<void> { | |
| try { | |
| // Get the contents of the folder | |
| const folderContents = await this.bitbucketClient.repositories.getDirectoryContent(owner, repoName, folderPath, branch); | |
| if (!Array.isArray(folderContents)) { | |
| throw new Error(`Path ${folderPath} is not a folder`); | |
| } | |
| // Delete each file in the folder | |
| for (const item of folderContents) { | |
| if (item.type === 'file') { | |
| await this.deleteFileInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | |
| } else if (item.type === 'directory') { | |
| // Recursively delete subdirectories | |
| await this.deleteFolderInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); | |
| } | |
| } | |
| console.log(`Successfully deleted folder ${folderPath} from ${owner}/${repoName}`); | |
| } catch (error: any) { | |
| if (error.status === 404 || error.message?.includes('not found')) { | |
| console.warn(`Folder ${folderPath} already absent in ${owner}/${repoName}, skipping delete`); | |
| return; | |
| } | |
| console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
tests/templates/import_templates.test.ts (5)
141-147: Make file/folder deletion idempotent for reliable test cleanup.Deletion operations throw on 404, causing reruns after partial cleanup to fail. Wrap deletions to treat "not found" as success.
Apply this diff:
+ const owner = git.getRepoOwner(); + // Delete .tekton folder - await git.deleteFolderInRepository(owner, componentName, '.tekton'); + try { + await git.deleteFolderInRepository(owner, componentName, '.tekton'); + } catch (e: any) { + if (e.status === 404 || e.message?.includes('not found')) { + console.log(`.tekton folder not found in ${componentName}, skipping`); + } else { + throw e; + } + } // Delete catalog-info.yaml file - await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); + try { + await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); + } catch (e: any) { + if (e.status === 404 || e.message?.includes('not found')) { + console.log(`catalog-info.yaml not found in ${componentName}, skipping`); + } else { + throw e; + } + } - console.log(`Deleted .tekton, gitops folders and catalog-info.yaml from ${componentName} in ${gitType}`); + console.log(`Deleted .tekton folder and catalog-info.yaml from ${componentName}`);
38-43: Remove placeholder test that provides no validation.This test only asserts that a constant is defined, which provides no signal. It was flagged in previous reviews.
Apply this diff:
- test(`verifies if ${templateName} template exists in the catalog`, async () => { - // This would require implementing a method to get golden path templates - // For now, we'll assume the template exists if we can create a component - expect(templateName).toBeDefined(); - console.log(`Template ${templateName} is available for testing`); - });
50-60: Sanitize error logs to prevent leaking auth tokens from Git providers.Logging the full error object can expose sensitive credentials embedded in HTTP clients and provider errors.
Apply this diff:
} catch (error) { - console.error(`❌ Failed to create component: ${error}`); + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create component: ${message}`); throw error; }As per coding guidelines: Do not print secrets in logs within tests. Apply the same pattern at lines 184-193.
98-107: Use interface method getRepoOwner() instead of provider-specific methods.Calling provider-specific methods like
getOrganization(),getGroup(), orgetWorkspace()requires casting and breaks abstraction. The Git interface providesgetRepoOwner()for this purpose.Apply this diff:
const git = component.getGit(); const componentName = component.getName(); - const gitType = git.getGitType(); - // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner(); // Check if repository exists const repositoryExists = await git.checkIfRepositoryExists(owner, componentName);Apply the same simplification at lines 129-139 and 231-241.
184-193: Sanitize error logging for imported component creation.Mirrors the issue at lines 50-60. Apply the same sanitization pattern.
Apply this diff:
} catch (error) { - console.error(`❌ Failed to create imported component: ${error}`); + const message = error instanceof Error ? error.message : String(error); + console.error(`❌ Failed to create imported component: ${message}`); throw error; }As per coding guidelines: Do not print secrets in logs within tests.
src/rhtap/core/integration/git/providers/bitbucket.ts (2)
1012-1022: Make file deletion idempotent by treating 404 as success.Test cleanup becomes brittle when deletions fail on missing files. Treating "not found" as success allows tests to be rerun after partial cleanup.
Apply this diff:
} catch (error: any) { + if (error.status === 404 || error.message?.includes('not found')) { + console.warn(`File ${filePath} already absent in ${owner}/${repoName}, skipping delete`); + return; + } console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`); throw error; }
1033-1057: Make folder deletion idempotent by treating 404 as success.Similar to file deletion, folder removal should not fail when the folder is already absent.
Apply this diff:
} catch (error: any) { + if (error.status === 404 || error.message?.includes('not found')) { + console.warn(`Folder ${folderPath} already absent in ${owner}/${repoName}, skipping delete`); + return; + } console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); throw error; }playwright.config.ts (1)
81-90: Fix testMatch pattern: remove redundant 'tests/' prefix for proper test discovery.The testMatch pattern 'tests/templates//*.test.ts' is resolved relative to testDir ('./tests'), causing Playwright to look for 'tests/tests/templates//*.test.ts'. This prevents import tests from being discovered.
Apply this diff:
if (ENABLE_IMPORT_TESTS) { importProjects = projectConfigs.map(config => ({ name: `import-${config.name}`, - testMatch: 'tests/templates/**/*.test.ts', + testMatch: 'templates/**/*.test.ts', use: { testItem: config.testItem, }, })); }Based on past review feedback, this same issue applies to e2e and ui projects (lines 42, 56) for consistency.
src/api/rhdh/developerhub.ts (3)
202-206: Tighten selector matching to prevent unintended deletions.Using
includes()risks matching unrelated entities. For example, selector "foo" would match "foo-imported", "my-foo-app", etc., potentially deleting the wrong components.Apply this diff:
- const filteredEntities = response.data.filter((entity: any) => - (entity.kind === 'Component' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Resource' && entity.metadata?.name?.includes(selector)) || - (entity.kind === 'Location' && entity.spec?.target?.includes(selector)) - ); + const filteredEntities = response.data + .filter((entity: any) => Boolean(entity?.metadata?.uid)) + .filter((entity: any) => + (entity.kind === 'Component' && entity.metadata?.name === selector) || + (entity.kind === 'Resource' && entity.metadata?.name === selector) || + (entity.kind === 'Location' && entity.spec?.target === selector) + );This uses exact matching and filters out entities without UIDs before deletion attempts.
225-228: Sanitize error logging to prevent token leakage.Console.error with the raw error object can expose sensitive data like auth tokens and headers in logs.
Apply this diff:
} catch (error) { - console.error(`Error deleting entities with selector ${selector}:`, error); + const msg = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entities with selector ${selector}: ${msg}`); return false; }As per coding guidelines: Set logging below TRACE in tests to avoid capturing sensitive data.
247-249: Sanitize error logging in entity delete helper.Similar to the parent method, avoid logging raw error objects that may contain sensitive data.
Apply this diff:
} catch (error) { - console.error(`Error deleting entity with UID ${id}:`, error); + const msg = error instanceof Error ? error.message : String(error); + console.error(`Error deleting entity with UID ${id}: ${msg}`); return false; }As per coding guidelines: Do not print secrets in logs within tests.
🧹 Nitpick comments (1)
tests/templates/import_templates.test.ts (1)
159-159: Pass exact entity selector to avoid over-deleting Backstage catalog entries.Passing the bare component name to
deleteEntitiesBySelectorrelies on the implementation'sincludes()matching, which can match unintended entities.Since the DeveloperHub method uses substring matching, consider passing a more specific identifier or updating the test to verify only expected entities are deleted. If you control the component name format, ensure it's sufficiently unique to avoid false matches.
Example if you can construct a qualified selector:
const selector = componentName; // Ensure componentName is unique or use a qualified form const deleted = await developerHub.deleteEntitiesBySelector(selector);The real fix should be in
deleteEntitiesBySelector(already flagged separately) to use exact matching.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (8)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rhtap/core/integration/git/providers/github.ts
- package.json
- src/rhtap/core/integration/git/providers/gitlab.ts
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
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/rhdh/developerhub.tssrc/api/github/services/github-repository.service.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.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/git/providers/bitbucket.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/git/providers/bitbucket.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.ts
🧬 Code graph analysis (2)
tests/templates/import_templates.test.ts (7)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/rhtap/core/integration/git/gitInterface.ts (1)
Git(12-118)src/rhtap/core/integration/git/providers/github.ts (1)
GithubProvider(11-807)src/rhtap/core/integration/git/providers/gitlab.ts (1)
GitlabProvider(15-1000)src/rhtap/core/integration/git/providers/bitbucket.ts (1)
BitbucketProvider(16-1058)src/playwright/testItem.ts (1)
TestItem(6-120)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
🔇 Additional comments (2)
src/api/github/services/github-repository.service.ts (2)
216-251: Implementation follows existing patterns; respects maintainer's consistency preference.The deleteFile method correctly uses the established error handling pattern in this service. The try/catch structure and logging are consistent with other methods in the class.
Based on learnings: The maintainer prefers to keep error handling consistent across methods within this class, even when alternative patterns (like idempotency for cleanup) might be beneficial. This implementation aligns with that preference.
259-273: Repository deletion follows established service patterns.The deleteRepository method maintains consistency with the class's error handling approach. Logging and error propagation match other methods in this service.
Based on learnings: Consistent error handling across the GithubRepositoryService class was prioritized by the maintainer over scenario-specific patterns.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
playwright.config.ts (1)
85-85: Correct testMatch path for proper test discovery.The testMatch path includes a redundant
tests/prefix. Since testDir is already'./tests', Playwright resolves patterns relative to that directory, causing discovery failures.Apply this diff:
- testMatch: 'tests/templates/**/*.test.ts', + testMatch: 'templates/**/*.test.ts',This aligns with the correction needed for e2e and ui projects (lines 42, 56) to ensure consistent test discovery across all project types.
🧹 Nitpick comments (1)
playwright.config.ts (1)
81-90: Consider adding timeout override for import projects.Import tests interact with external services (GitHub/GitLab/Bitbucket per the AI summary), which may be slower than local operations. UI projects have a timeout override (line 62); import projects might benefit from similar configuration to handle service latencies gracefully.
Example addition:
if (ENABLE_IMPORT_TESTS) { importProjects = projectConfigs.map(config => ({ name: `import-${config.name}`, testMatch: 'templates/**/*.test.ts', use: { testItem: config.testItem, }, + expect: { + timeout: DEFAULT_UI_TIMEOUT, // or a separate IMPORT_TIMEOUT constant + }, })); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/gitlab/gitlab.client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
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/gitlab/gitlab.client.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
🧬 Code graph analysis (1)
src/api/gitlab/gitlab.client.ts (1)
src/api/gitlab/types/gitlab.types.ts (1)
ProjectIdentifier(189-189)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright.config.ts(3 hunks)src/api/bitbucket/services/bitbucket-repository.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
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/bitbucket/services/bitbucket-repository.service.ts
⏰ 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
| public async deleteFile(workspace: string, repoSlug: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { | ||
| try { | ||
| // Bitbucket API requires a commit with file deletion | ||
| const commitData = { | ||
| message: commitMessage, | ||
| branch: branch, | ||
| files: { | ||
| [filePath]: null // null value indicates file deletion | ||
| } | ||
| }; | ||
|
|
||
| await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, commitData, { | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| }); | ||
|
|
||
| console.log(`Successfully deleted file ${filePath} from ${workspace}/${repoSlug}`); | ||
| } catch (error) { | ||
| console.error(`Failed to delete file ${filePath} from ${workspace}/${repoSlug}:`, error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Form-urlencoded payload formatting is incorrect.
The deleteFile method has a critical implementation issue:
The Content-Type header is set to application/x-www-form-urlencoded, but the payload is sent as a plain JavaScript object. Form-urlencoded requires a URL-encoded string format (key1=value1&key2=value2). Sending a nested object with this content type will likely fail or produce unexpected results.
Additionally, per coding guidelines for src/api/**, implement retry mechanisms, timeouts, and circuit breaker patterns for API calls.
Verify the correct Bitbucket API format for file deletion.
Before applying fixes, verify the actual Bitbucket API requirements for file deletion. The current structure suggests form-urlencoded, but the nested files object is incompatible with that format.
Run the following script to check Bitbucket API documentation and verify the correct format:
If form-urlencoded is indeed required, the payload must be properly encoded. Consider using a library like qs or native URLSearchParams to encode the data. Alternatively, if Bitbucket accepts JSON for this endpoint, change the Content-Type to application/json.
🏁 Script executed:
#!/bin/bash
# Description: Search for Bitbucket file deletion patterns in the codebase and verify API format
# Search for other Bitbucket API calls with form-urlencoded to understand the pattern
rg -n --type=ts -A 5 -B 5 "application/x-www-form-urlencoded"
# Search for any existing file deletion implementations
rg -n --type=ts -A 10 "deleteFile|delete.*file" src/api/Length of output: 8451
Serialize form-urlencoded payload properly
The deleteFile method sets Content-Type: application/x-www-form-urlencoded but sends a raw JS object. Use URLSearchParams or qs.stringify to serialize message, branch and the dynamic files[filePath]=null into key=value&… form (or switch to application/json if supported).
src/api/bitbucket/services/bitbucket-repository.service.ts:72-75
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/api/rhdh/developerhub.ts (1)
195-251: Past review feedback appears addressed; remaining concerns tracked in previous comments.The code changes for entity deletion show the method has been renamed to
unregisterEntityByUidand now accepts any 2xx status code as suggested in past reviews. The remaining concerns about selector matching strictness, retry mechanisms, and error sanitization are already documented in past review comments.src/rhtap/core/integration/git/providers/github.ts (2)
717-727: File vs directory check still pending.As noted in past reviews, this method should verify that
getContentreturns a file object (not an array representing a directory) to avoid false positives. The past review comment provides the suggested implementation.
738-755: Past retry and error sanitization suggestions tracked.Previous reviews flagged the need for retry logic and sanitized error logging. These improvements are documented in earlier review comments.
src/api/gitlab/gitlab.client.ts (1)
309-347: Timeout and logging improvements tracked in past reviews.Past review comments comprehensively document the need for timeout configuration, structured logging, and type safety improvements for these API methods.
As per coding guidelines
tests/templates/import_templates.test.ts (2)
50-60: Error logging already flagged in past reviews for sanitization.The catch block at lines 57-59 logs the raw error object. Past review comments document the need to sanitize this to prevent token leakage.
88-118: Past review suggests using getRepoOwner() for provider-agnostic code.The verbose type-checking and casting logic at lines 88-107 (and duplicated at 222-241) is already addressed in past review comments, which suggest using
git.getRepoOwner()instead of provider-specific methods likegetOrganization(),getGroup(), orgetWorkspace().
🧹 Nitpick comments (2)
tests/templates/import_templates.test.ts (2)
1-5: Consider suppressing dotenv logs in CI environments.The dotenv configuration at line 5 will log injection details by default in v17.2.0+. For CI environments where this noise isn't helpful, consider setting
quiet: truein the config or using theDOTENV_CONFIG_QUIET=trueenvironment variable.Based on learnings
Apply this diff if you want to suppress the logs:
-config({ path: resolve(process.cwd(), '.env') }); +config({ path: resolve(process.cwd(), '.env'), quiet: process.env.CI === 'true' });
32-43: Consider test reliability: serial execution with proper isolation.The
test.describe.serialwrapper ensures tests run in order, which is appropriate for this workflow. However, ensure each test properly handles the case where previous tests failed (theif (!component)checks are good).One concern: if the test suite is interrupted mid-run, cleanup won't happen. Consider adding a global teardown or at least documenting the manual cleanup procedure.
Consider adding a note in the test file header:
/** * Import Templates Test Suite * * This test suite validates the import template functionality in Red Hat Developer Hub: * 1. Creates a component using a template * 2. Verifies the component is created successfully * 3. Deletes the component from Developer Hub * 4. Re-imports the component using the import template * 5. Verifies the imported component is created successfully * * NOTE: If tests are interrupted, manual cleanup may be required: * - Delete repositories from Git provider * - Delete entities from Developer Hub catalog * - Clean up ArgoCD applications if needed */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (10)
package.json(1 hunks)playwright.config.ts(3 hunks)src/api/bitbucket/services/bitbucket-repository.service.ts(1 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/gitlab/gitlab.client.ts(1 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/bitbucket/services/bitbucket-repository.service.ts
- playwright.config.ts
- package.json
- src/api/github/services/github-repository.service.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/git/providers/gitlab.tssrc/rhtap/core/integration/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/github.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/git/providers/gitlab.tssrc/rhtap/core/integration/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/github.ts
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/gitlab/gitlab.client.tssrc/api/rhdh/developerhub.ts
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (2)
src/api/gitlab/gitlab.client.ts (1)
src/api/gitlab/types/gitlab.types.ts (1)
ProjectIdentifier(189-189)
tests/templates/import_templates.test.ts (4)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/rhtap/core/integration/git/gitInterface.ts (1)
Git(12-118)src/playwright/testItem.ts (1)
TestItem(6-120)
⏰ 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 (5)
src/rhtap/core/integration/git/providers/github.ts (3)
697-707: LGTM!Clean existence check implementation that properly distinguishes 404 (not found) from other errors.
766-789: Implementation consistent with project error-handling patterns.The recursive deletion approach and error handling align with the maintainer's stated preference for uniform error behavior across methods. The optional batching improvement is tracked in past review comments.
Based on learnings
797-806: Consistent with established error-handling patterns.Implementation follows the project's uniform error-handling approach for deletion methods.
Based on learnings
src/rhtap/core/integration/git/providers/bitbucket.ts (1)
971-1057: Consistent implementation across Bitbucket provider methods.The four new methods (existence checks and deletions) follow patterns consistent with the GitHub provider implementation. Error handling includes both status code and message checks for not-found conditions. The past review comment regarding idempotent cleanup for test scenarios is already documented.
src/rhtap/core/integration/git/providers/gitlab.ts (1)
908-999: Consistent implementation with defensive error checking.The GitLab provider methods follow the same patterns as GitHub and Bitbucket implementations. The error handling includes both status code and message checks for not-found conditions (
error.status === 404 || error.message?.includes('not found')), which is more defensive than checking status alone.One minor observation: each method calls
getProject()separately. While this ensures the project exists before operations, it adds an extra API call. This is acceptable for the test/cleanup use case but could be optimized with caching if these methods are called frequently in sequence.Optional optimization suggestion for frequent sequential calls:
// Consider a private helper that caches project lookups within a request private projectCache = new Map<string, any>(); private async getProjectCached(path: string): Promise<any> { if (!this.projectCache.has(path)) { const project = await this.gitlabClient.getProject(path); this.projectCache.set(path, project); } return this.projectCache.get(path)!; }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
src/api/rhdh/developerhub.ts (1)
213-227: Skip entities that lack stable UIDs before deletingRight now we pipe every catalog item through the retry/delete path without first ensuring
entity.metadata?.uidexists. If Backstage ever responds with an entry missing a UID (or an unexpected shape), this code will passundefinedintounregisterEntityByUid, causing malformed DELETE URLs and flaky cleanup in the import E2E flows. Guard the payload up front so we only schedule deletions for entities we can address deterministically—this keeps the tests reliable and avoids hammering the catalog with bad requests.- const filteredEntities = response.data.filter((entity: any) => { + const catalogEntities: any[] = Array.isArray(response.data) ? response.data : []; + const filteredEntities = catalogEntities + .filter((entity: any) => Boolean(entity?.metadata?.uid)) + .filter((entity: any) => { // Check if entity matches all selector criteria return Object.entries(selectorCriteria).every(([key, value]) => { switch (key) { case 'kind': return entity.kind === value; case 'name': return entity.metadata?.name === value; case 'namespace': return entity.metadata?.namespace === value; default: return false; } }); - }); + });tests/templates/import_templates.test.ts (6)
19-20: Gate suite behind ENABLE_IMPORT_TESTS to prevent destructive runs by defaultconst test = createBasicFixture(); +// Skip the entire suite unless explicitly enabled +if (process.env.ENABLE_IMPORT_TESTS !== 'true') { + test.skip(true, 'Import tests are disabled. Set ENABLE_IMPORT_TESTS=true to enable this suite.'); +}
31-34: Increase timeout and tune retries; 5 minutes is too tight for Argo/cluster opsSet a generous bound and condition retries for CI.
-test.setTimeout(300000); // 5-minute timeout -test.describe.configure({ retries: 2 }); // Retry on transient CI flakiness +test.describe.configure({ + timeout: 45 * 60_000, // 45 minutes for e2e provisioning/sync + retries: process.env.CI ? 1 : 0, +});Please confirm recent pipeline timeouts; if failures are time-related, bumping this should stabilize runs.
40-45: Remove placeholder “template exists” testIt provides no signal and adds noise. Drop it or replace with a real catalog query.
- test(`verifies if ${templateName} template exists in the catalog`, async () => { - // This would require implementing a method to get golden path templates - // For now, we'll assume the template exists if we can create a component - expect(templateName).toBeDefined(); - console.log(`Template ${templateName} is available for testing`); - });
11-13: Use provider‑agnostic owner API to reduce coupling and importsPrefer git.getRepoOwner(); drop provider imports and instanceof checks.
-import { GithubProvider } from '../../src/rhtap/core/integration/git/providers/github'; -import { GitlabProvider } from '../../src/rhtap/core/integration/git/providers/gitlab'; -import { BitbucketProvider } from '../../src/rhtap/core/integration/git/providers/bitbucket';- // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner();- // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner();- // Get the appropriate owner based on Git provider type - let owner: string; - if (git instanceof GithubProvider) { - owner = git.getOrganization(); - } else if (git instanceof GitlabProvider) { - owner = git.getGroup(); - } else if (git instanceof BitbucketProvider) { - owner = git.getWorkspace(); - } else { - throw new Error(`Unsupported Git provider type: ${gitType}`); - } + const owner = git.getRepoOwner();Also applies to: 99-109, 131-141, 237-247
75-88: Mark long ArgoCD waits as slow for clearer reportingHelps triage time-dominant steps.
test(`waits for ${templateName} argocd to be synced in the cluster`, async () => { + test.slow(); // ...test(`waits for imported component argocd to be synced in the cluster`, async () => { + test.slow(); // ...Also applies to: 212-226
143-149: Harden cleanup to avoid flakiness and fix log messageBest-effort delete .tekton; check catalog-info.yaml exists before delete. Update log to not mention gitops.
- // Delete .tekton folder - await git.deleteFolderInRepository(owner, componentName, '.tekton'); - - // Delete catalog-info.yaml file - await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); - - console.log(`Deleted .tekton, gitops folders and catalog-info.yaml from ${componentName} in ${gitType}`); + // Best-effort delete .tekton (may be absent depending on template) + try { + await git.deleteFolderInRepository(owner, componentName, '.tekton'); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + console.warn(`.tekton delete skipped/failed for ${componentName}: ${msg}`); + } + + // Delete catalog-info.yaml only if present + const hasCatalog = await git.checkIfFileExistsInRepository(owner, componentName, 'catalog-info.yaml'); + if (hasCatalog) { + await git.deleteFileInRepository(owner, componentName, 'catalog-info.yaml'); + } else { + console.warn(`catalog-info.yaml not found in ${componentName}, skipping delete`); + } + + console.log(`Deleted .tekton folder and/or catalog-info.yaml from ${componentName} in ${gitType}`);src/rhtap/core/integration/git/providers/github.ts (2)
738-755: Stabilize file deletes with short retries on 409/429/5xxTransient API/race issues are common; add bounded backoff.
- public async deleteFileInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { - try { - // First, get the current file to get its SHA - const fileContent = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - - if (!fileContent || !('sha' in fileContent)) { - throw new Error(`File ${filePath} not found or is not a file`); - } - - // Delete the file using the GitHub API - await this.githubClient.repository.deleteFile(owner, repoName, filePath, commitMessage, fileContent.sha, branch); - - console.log(`Successfully deleted file ${filePath} from ${owner}/${repoName}`); - } catch (error: any) { - console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error.message}`); - throw error; - } - } + public async deleteFileInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { + const maxAttempts = 3; + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + const fileContent = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(fileContent) || !fileContent || !('sha' in fileContent)) { + throw new Error(`Path ${filePath} is not a file or does not exist`); + } + await this.githubClient.repository.deleteFile(owner, repoName, filePath, commitMessage, (fileContent as any).sha, branch); + console.log(`Successfully deleted file ${filePath} from ${owner}/${repoName}`); + return; + } catch (error: any) { + const status = error?.status ?? error?.statusCode ?? error?.response?.status; + const retryable = status === 409 || status === 429 || (typeof status === 'number' && status >= 500 && status < 600); + if (retryable && attempt < maxAttempts) { + const delayMs = 250 * Math.pow(2, attempt - 1); + console.warn(`Delete ${filePath} attempt ${attempt} failed (status: ${status}). Retrying in ${delayMs} ms...`); + await new Promise(r => setTimeout(r, delayMs)); + continue; + } + console.error(`Failed to delete file ${filePath} from ${owner}/${repoName}: ${error?.message ?? String(error)}`); + throw error; + } + } + }
717-727: Avoid false positives: ensure path is a file, not a directorygetContent returns array for directories. Only return true for files.
- public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { - try { - await this.githubClient.repository.getContent(owner, repoName, filePath, branch); - return true; - } catch (error: any) { + public async checkIfFileExistsInRepository(owner: string, repoName: string, filePath: string, branch: string = 'main'): Promise<boolean> { + try { + const content = await this.githubClient.repository.getContent(owner, repoName, filePath, branch); + if (Array.isArray(content)) return false; + return (content as any).type === 'file' || 'content' in (content as any); + } catch (error: any) { if (error.status === 404) { return false; } throw error; - } + } }
🧹 Nitpick comments (4)
src/api/rhdh/developerhub.ts (1)
229-231: Update the log message to reflect selector matchingThe log still says “description containing”, but we now parse explicit selector key/value pairs. Tweaking the wording keeps troubleshooting crisp and avoids confusion during import test triage.
tests/templates/import_templates.test.ts (2)
4-5: Silence dotenv in CI to avoid leaking env metadataUse quiet to suppress runtime tips/logs.
Based on learnings
-// Load environment variables from .env file -config({ path: resolve(process.cwd(), '.env') }); +// Load environment variables from .env file (quiet in CI) +config({ path: resolve(process.cwd(), '.env'), quiet: true });
228-259: Optional: add final cleanup to delete imported repo to keep environments tidyPrevents accumulation across retries.
}); + +test('cleanup imported repository', async () => { + if (!importedComponent) return; + const git = importedComponent.getGit(); + const owner = git.getRepoOwner(); + await git.deleteRepository(owner, importedComponent.getName()); + console.log(`Deleted imported repository ${importedComponent.getName()}`); +});src/rhtap/core/integration/git/providers/github.ts (1)
766-833: Avoid no‑op commits when folder is absent; detect and early‑exitCompute paths to remove; if none found, return without creating a commit. Optionally guard against truncated trees.
public async deleteFolderInRepository(owner: string, repoName: string, folderPath: string, branch: string = 'main', commitMessage: string = 'Delete folder'): Promise<void> { try { @@ - // Filter out any tree entries under the target folder path - const filteredTreeItems = treeData.tree.filter((item: any) => { - // Remove any items that start with the folder path - return !item.path.startsWith(folderPath + '/') && item.path !== folderPath; - }); + // Identify entries under the target folder path + const toRemove = treeData.tree.filter((item: any) => + item.path === folderPath || item.path.startsWith(folderPath + '/') + ); + if (toRemove.length === 0) { + console.log(`Folder ${folderPath} not found in ${owner}/${repoName}; nothing to delete.`); + return; + } + // Build new tree without the folder entries + const filteredTreeItems = treeData.tree.filter((item: any) => + !(item.path === folderPath || item.path.startsWith(folderPath + '/')) + ); @@ console.log(`Successfully deleted folder ${folderPath} from ${owner}/${repoName} in a single commit`); } catch (error: any) { console.error(`Failed to delete folder ${folderPath} from ${owner}/${repoName}: ${error.message}`); throw error; } }Optionally, if treeData.truncated is true, consider aborting to avoid accidental data loss and fall back to per-path deletes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
playwright.config.ts(4 hunks)src/api/rhdh/developerhub.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.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/rhdh/developerhub.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/git/providers/github.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/git/providers/github.ts
tests/**/*.ts
📄 CodeRabbit inference engine (README.md)
tests/**/*.ts: Do not print secrets in logs within tests
Set logging below TRACE in tests to avoid capturing sensitive data (e.g., network requests)
Files:
tests/templates/import_templates.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/templates/import_templates.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/templates/import_templates.test.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/rhtap/core/integration/git/providers/github.ts
🧬 Code graph analysis (1)
tests/templates/import_templates.test.ts (3)
src/utils/test/fixtures.ts (1)
createBasicFixture(29-36)src/rhtap/core/component.ts (1)
Component(15-260)src/playwright/testItem.ts (1)
TestItem(6-120)
🔇 Additional comments (1)
tests/templates/import_templates.test.ts (1)
160-166: Ignore suggestion to usemetadata.nameas the selector key;deleteEntitiesBySelectormaps thenamekey toentity.metadata.name, sokind=Component,name=${componentName}is correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/api/gitlab/gitlab.client.ts (1)
305-327: Replace console.error with structured logger and define return type.While the timeout concern is now addressed by the client-level configuration (line 61), this method still uses
console.error(line 324) instead of the importeddefaultLogger, and the return type remainsany[](line 316), bypassing TypeScript's type safety.Apply this diff to use structured logging:
} catch (error) { - console.error(`Failed to get repository tree for ${projectId}:`, error); + defaultLogger.error({ + operation: 'getRepositoryTree', + projectId, + path, + branch, + error: error instanceof Error ? error.message : String(error) + }, `Failed to get repository tree for ${projectId}`); throw error; }Consider defining a proper interface for the tree nodes to replace
any[]:interface RepositoryTreeNode { id: string; name: string; type: 'blob' | 'tree'; path: string; mode: string; } public async getRepositoryTree( projectId: ProjectIdentifier, path: string = '', branch: string = 'main' ): Promise<RepositoryTreeNode[]> { // ... }As per coding guidelines for
src/api/**.
🧹 Nitpick comments (2)
src/api/rhdh/developerhub.ts (1)
196-225: Prefer async-retry for consistency and maintainability.The manual retry loop reinvents functionality already provided by the
async-retrylibrary (imported at line 2 and used for entity deletions at lines 287-306). Switching toasync-retrywould reduce code complexity, improve consistency across the codebase, and make retry parameters easier to tune.Apply this diff to use
async-retry:- private async getEntitiesWithRetry(): Promise<AxiosResponse> { - const maxRetries = 3; - const baseDelay = 1000; // 1 second base delay - - for (let attempt = 1; attempt <= maxRetries; attempt++) { - try { - console.log(`Fetching entities (attempt ${attempt}/${maxRetries})`); - const response = await this.axios.get(`${this.url}/api/catalog/entities`, { - timeout: 10000, // 10 second per-request timeout - }); - return response; - } catch (error: any) { - const isLastAttempt = attempt === maxRetries; - - if (isLastAttempt) { - console.error(`Failed to fetch entities after ${maxRetries} attempts: ${error instanceof Error ? error.message : 'Unknown error'}`); - throw error; - } - - // Calculate exponential backoff delay - const delay = baseDelay * Math.pow(2, attempt - 1); - console.log(`Request failed (attempt ${attempt}/${maxRetries}), retrying in ${delay}ms: ${error instanceof Error ? error.message : 'Unknown error'}`); - - await new Promise(resolve => setTimeout(resolve, delay)); - } - } - - // This should never be reached, but TypeScript requires it - throw new Error('Unexpected error in getEntitiesWithRetry'); - } + private async getEntitiesWithRetry(): Promise<AxiosResponse> { + return await retry( + async () => { + return await this.axios.get(`${this.url}/api/catalog/entities`, { + timeout: 10000, // 10 second per-request timeout + }); + }, + { + retries: 3, + factor: 2, + minTimeout: 1000, + maxTimeout: 4000, + onRetry: (error: Error, attempt: number) => { + console.log( + `[RETRY ${attempt}/3] 🔄 Fetching entities | Reason: ${error.message}` + ); + }, + } + ); + }Based on learnings
src/rhtap/core/integration/git/providers/gitlab.ts (1)
970-1011: LGTM! Recursive folder deletion is now idempotent.The 404 handling (lines 1003-1005) makes folder deletion repeatable, which is essential for reliable test cleanup. The recursive approach correctly handles nested subdirectories (lines 994-996).
Consider parallel deletion for better performance with large folder structures:
// Delete files in parallel const deletePromises = folderContents.map(async (item) => { if (item.type === 'blob') { return this.deleteFileInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); } else if (item.type === 'tree') { return this.deleteFolderInRepository(owner, repoName, item.path, branch, `${commitMessage}: ${item.name}`); } }); await Promise.all(deletePromises);Note: Lines 1000, 1004, and 1008 still use
console.logandconsole.errorinstead of structured logging (similar to previous review comments).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
playwright.config.ts(4 hunks)src/api/github/services/github-repository.service.ts(1 hunks)src/api/gitlab/gitlab.client.ts(3 hunks)src/api/rhdh/developerhub.ts(2 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/templates/import_templates.test.ts
🧰 Additional context used
📓 Path-based instructions (4)
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/rhdh/developerhub.tssrc/api/gitlab/gitlab.client.tssrc/api/github/services/github-repository.service.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/git/providers/gitlab.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/git/providers/gitlab.ts
playwright.config.ts
⚙️ CodeRabbit configuration file
Review test configuration for optimal performance, browser configurations, device settings, reporter configurations, artifact management, and parallelization settings.
Files:
playwright.config.ts
🧠 Learnings (2)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
PR: redhat-appstudio/tssc-test#72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
playwright.config.ts
🧬 Code graph analysis (2)
src/api/gitlab/gitlab.client.ts (2)
src/api/gitlab/types/gitlab.types.ts (1)
ProjectIdentifier(189-189)src/log/logger.ts (1)
defaultLogger(113-113)
src/api/github/services/github-repository.service.ts (1)
src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
⏰ 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 (9)
playwright.config.ts (1)
81-92: LGTM! Import projects configuration is well-designed.The import template projects are properly configured with:
- Retry budget (
retries: 1) to handle transient flakiness during import flows- Authentication dependency to ensure auth setup runs before import tests
- Proper testMatch pattern relative to testDir
- TestItem configuration propagated from project configs
This aligns well with the E2E and UI project patterns and addresses the reliability concerns from previous reviews.
src/api/rhdh/developerhub.ts (3)
24-24: Good addition of global timeout.The 30-second global timeout provides a sensible upper bound for all HTTP requests, preventing indefinite hangs and improving overall reliability of the API client. This aligns with the coding guidelines for API client patterns.
232-321: Well-implemented deletion logic with proper safeguards.The deletion flow correctly addresses previous review concerns:
- Uses
getEntitiesWithRetry()for network resilience- Parses selector format with multiple criteria support
- Applies strict equality matching to prevent over-deletion (lines 257-276)
- Filters out entities without
metadata.uidto avoid undefined deletes (lines 252-254)- Wraps each deletion in
async-retrythat throws on failure, ensuring retries actually fire (lines 287-306)- Aggregates results and returns appropriate boolean status
The selector parsing is straightforward and suitable for typical entity identifiers (kind, name, namespace). If selectors ever need to support values containing
=or,, consider a more robust parser, but the current implementation is appropriate for the use case.
329-343: LGTM! Entity deletion method properly handles responses.The method correctly:
- Accepts any 2xx status code as success (lines 332-334), avoiding false negatives from Backstage 200 responses
- Returns boolean status for the retry wrapper to check
- Sanitizes error logging to prevent token/header leakage (line 340)
- Provides clear success/failure logging with status codes
This addresses all concerns from previous reviews and integrates well with the retry wrapper in
deleteEntitiesBySelector.src/api/gitlab/gitlab.client.ts (2)
2-3: LGTM! Imports support robust error handling.The addition of
async-retryanddefaultLoggerenables the newdeleteFilemethod to implement proper retry logic and structured logging, aligning with the coding guidelines forsrc/api/**.
57-61: LGTM! Timeout configuration addresses previous reliability concerns.The 30-second
requestTimeouton the GitLab client initialization ensures all API calls have a bounded timeout, preventing indefinite blocking on slow or unresponsive GitLab endpoints. This resolves the timeout concerns raised in previous reviews.src/rhtap/core/integration/git/providers/gitlab.ts (3)
902-918: LGTM! Repository existence check is clean and concise.The method correctly distinguishes between "not found" (returns false) and other errors (rethrows), providing a reliable way to check repository existence without unnecessary exceptions.
920-939: LGTM! File existence check follows the same reliable pattern.The implementation correctly handles 404 responses as "not found" while propagating other errors, matching the pattern established in
checkIfRepositoryExists.
941-968: LGTM! Idempotent deletion now implemented as requested.The 404 handling (lines 960-962) now treats "not found" as a successful no-op, making cleanup operations repeatable and reliable in test scenarios. This addresses the previous review concern about non-idempotent deletions.
Note: Lines 957, 961, and 965 still use
console.logandconsole.errorinstead of structured logging (similar to previous review comments on the GitLab client).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/api/bitbucket/services/bitbucket-repository.service.ts (1)
147-169: CRITICAL: Form-urlencoded payload formatting still incorrect.This critical issue from the previous review remains unfixed. The method sets
Content-Type: application/x-www-form-urlencodedbut sends a raw JavaScript object. Form-urlencoded requires a URL-encoded string format (key1=value1&key2=value2).Additionally, this method violates multiple coding guidelines for
src/api/**:
- No retry mechanism for transient failures
- No timeout on the HTTP request
- No circuit breaker pattern
- Uses
console.log/errorinstead ofdefaultLogger- No input validation for required parameters
- No structured logging for debugging
As per coding guidelines.
Apply this diff to fix the payload encoding and improve resilience:
+import { stringify } from 'qs'; public async deleteFile(workspace: string, repoSlug: string, filePath: string, branch: string = 'main', commitMessage: string = 'Delete file'): Promise<void> { + // Input validation + if (!workspace || workspace.trim() === '') { + throw new Error('Workspace is required and cannot be empty'); + } + if (!repoSlug || repoSlug.trim() === '') { + throw new Error('Repository slug is required and cannot be empty'); + } + if (!filePath || filePath.trim() === '') { + throw new Error('File path is required and cannot be empty'); + } + try { - // Bitbucket API requires a commit with file deletion - const commitData = { + const response = await retry( + async () => { + // Bitbucket API requires form-urlencoded data + const formData = stringify({ - message: commitMessage, - branch: branch, - files: { - [filePath]: null // null value indicates file deletion - } - }; - - await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, commitData, { - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', + message: commitMessage, + branch: branch, + [`files[${filePath}]`]: null + }); + + return await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, formData, { + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + }); }, - }); + { + retries: 3, + minTimeout: 1000, + maxTimeout: 5000, + factor: 2, + onRetry: (error: Error, attempt: number) => { + defaultLogger.warn({ + operation: 'deleteFile', + workspace, + repoSlug, + filePath, + branch, + attempt, + error: error.message + }, `Retrying file deletion (attempt ${attempt}/3)`); + } + } + ); - console.log(`Successfully deleted file ${filePath} from ${workspace}/${repoSlug}`); + defaultLogger.info({ + operation: 'deleteFile', + workspace, + repoSlug, + filePath, + branch + }, `Successfully deleted file ${filePath} from ${workspace}/${repoSlug}`); } catch (error) { - console.error(`Failed to delete file ${filePath} from ${workspace}/${repoSlug}:`, error); + defaultLogger.error({ + operation: 'deleteFile', + workspace, + repoSlug, + filePath, + branch, + error: error.message + }, `Failed to delete file ${filePath} from ${workspace}/${repoSlug}`); throw error; } }Note: You'll need to add
qsto your dependencies:npm install qs npm install --save-dev @types/qssrc/api/gitlab/gitlab.client.ts (1)
418-448: Retry logic still doesn't bail on 404 errors.The inner try-catch correctly identifies 404 errors, but it throws the error instead of calling
bail(error). This means the retry mechanism will still attempt all 3 retries for non-existent files, wasting time and API quota.Apply this diff to properly bail on 404 errors:
await retry( - async () => { + async (bail) => { try { await this.client.RepositoryFiles.remove(projectId, trimmedFilePath, trimmedBranch, trimmedCommitMessage); } catch (error: any) { // Check if error is 404 - don't retry on 404 errors if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) { - // Rethrow 404 errors immediately to prevent retries - throw error; + // Bail immediately on 404 - file already absent + bail(error); + return; } // For all other errors, rethrow to let retry mechanism handle them throw error;Based on learnings.
src/api/github/services/github-repository.service.ts (1)
241-268: Keep error contract consistent: wrap validation failures in GithubApiError.Right now the guard clauses throw bare
Errorinstances, whereas every other public method in this service surfaces aGithubApiError. That inconsistency leaks a different exception type to callers, so downstream cleanup flows can no longer rely oninstanceof GithubApiErroror status codes when input validation triggers. Please reuse the same typed error (status 400) so the service API stays predictable.- if (!owner || owner.trim() === '') { - throw new Error('Owner is required and cannot be empty'); - } - if (!repo || repo.trim() === '') { - throw new Error('Repository name is required and cannot be empty'); - } - if (!path || path.trim() === '') { - throw new Error('File path is required and cannot be empty'); - } - if (!message || message.trim() === '') { - throw new Error('Commit message is required and cannot be empty'); - } - if (!sha || sha.trim() === '') { - throw new Error('File SHA is required and cannot be empty'); - } - if (!branch || branch.trim() === '') { - throw new Error('Branch is required and cannot be empty'); - } - - const trimmedOwner = owner.trim(); - const trimmedRepo = repo.trim(); - const trimmedPath = path.trim(); - const trimmedMessage = message.trim(); - const trimmedSha = sha.trim(); - const trimmedBranch = branch.trim(); + const trimmedOwner = owner?.trim(); + const trimmedRepo = repo?.trim(); + const trimmedPath = path?.trim(); + const trimmedMessage = message?.trim(); + const trimmedSha = sha?.trim(); + const trimmedBranch = branch?.trim(); + + const missingFields: string[] = []; + if (!trimmedOwner) missingFields.push('owner'); + if (!trimmedRepo) missingFields.push('repo'); + if (!trimmedPath) missingFields.push('path'); + if (!trimmedMessage) missingFields.push('message'); + if (!trimmedSha) missingFields.push('sha'); + if (!trimmedBranch) missingFields.push('branch'); + + if (missingFields.length > 0) { + const details = missingFields.join(', '); + throw new GithubApiError( + `Invalid deleteFile parameters: missing or empty ${details}`, + 400, + new Error(`Missing required fields: ${details}`) + ); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/bitbucket/services/bitbucket-repository.service.ts(2 hunks)src/api/bitbucket/types/bitbucket.types.ts(1 hunks)src/api/github/services/github-repository.service.ts(2 hunks)src/api/gitlab/gitlab.client.ts(4 hunks)src/api/gitlab/types/gitlab.types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/bitbucket/types/bitbucket.types.tssrc/api/github/services/github-repository.service.tssrc/api/gitlab/gitlab.client.tssrc/api/bitbucket/services/bitbucket-repository.service.tssrc/api/gitlab/types/gitlab.types.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
PR: redhat-appstudio/tssc-test#114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.ts
🧬 Code graph analysis (3)
src/api/github/services/github-repository.service.ts (2)
src/log/logger.ts (1)
defaultLogger(113-113)src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
src/api/gitlab/gitlab.client.ts (2)
src/api/gitlab/types/gitlab.types.ts (2)
ProjectIdentifier(198-198)RepositoryTreeNode(45-51)src/log/logger.ts (1)
defaultLogger(113-113)
src/api/bitbucket/services/bitbucket-repository.service.ts (2)
src/api/bitbucket/types/bitbucket.types.ts (2)
BitbucketDirectoryEntry(87-94)BitbucketPaginatedResponse(96-103)src/log/logger.ts (1)
defaultLogger(113-113)
🔇 Additional comments (10)
src/api/bitbucket/types/bitbucket.types.ts (1)
87-94: LGTM! Type definition improves API safety.The
BitbucketDirectoryEntryinterface provides proper typing for directory listing operations, addressing the type safety concerns from the previous review. The structure correctly represents Bitbucket API response fields with appropriate optional properties.src/api/bitbucket/services/bitbucket-repository.service.ts (2)
53-145: Good progress on resilience patterns, but add timeout and circuit breaker.The method addresses most concerns from the previous review:
- ✓ Type safety with
BitbucketDirectoryEntry[]- ✓ Defensive response validation
- ✓ Retry mechanism with exponential backoff
- ✓ Structured logging with
defaultLogger- ✓ Graceful 404 handling
However, per coding guidelines for
src/api/**, you still need:
- Request timeout: The retry
minTimeout/maxTimeoutonly control retry intervals, not the overall HTTP request timeout. Add anAbortSignalor timeout to the HTTP client call.- Circuit breaker pattern: Implement a circuit breaker to prevent cascading failures when Bitbucket is consistently unavailable.
As per coding guidelines.
Consider this approach for timeout:
// Add timeout wrapper const TIMEOUT_MS = 10000; const response = await retry( async () => { const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), TIMEOUT_MS); try { return await this.httpClient.get<BitbucketPaginatedResponse<BitbucketDirectoryEntry>>( `/repositories/${trimmedWorkspace}/${trimmedRepoSlug}/src/${trimmedRef}/${trimmedPath}`, { signal: controller.signal } ); } finally { clearTimeout(timeoutId); } }, { retries: 3, minTimeout: 1000, maxTimeout: 5000, factor: 2, onRetry: ... } );For circuit breaker, consider libraries like
opossumor implement a simple state-based breaker that opens after consecutive failures.
1-1: async-retry version is valid Package.json specifies ^1.3.3, matching the latest release and ensuring reliable retry behavior.src/api/gitlab/types/gitlab.types.ts (1)
44-51: LGTM!Clean type definition with appropriate readonly modifiers and accurate representation of GitLab's repository tree structure. The
'blob' | 'tree'union type correctly models the two node types.src/api/gitlab/gitlab.client.ts (6)
2-3: LGTM!Appropriate imports for retry functionality and structured logging. The use of
async-retryanddefaultLoggeraligns with best practices for API client reliability and observability.Also applies to: 41-41
58-63: LGTM!The 30-second request timeout configuration addresses previous concerns about missing timeout protection. This prevents indefinite blocking on slow or unresponsive GitLab API calls, improving reliability for E2E test scenarios.
313-324: LGTM!Proper method signature with sensible defaults and comprehensive input validation. The JSDoc clearly documents parameters and return type.
363-363: Verify type assertion necessity.The
as RepositoryTreeNode[]assertion might be unnecessary if the@gitbeaker/restlibrary already returns the correct type. Type assertions can hide type mismatches and bypass TypeScript's safety guarantees.Verify whether the GitLab client library's type definitions already match
RepositoryTreeNode[]. If they do, remove the assertion. If not, document why the assertion is needed (e.g., library types are less specific).
397-416: LGTM!Thorough input validation with clear error messages and proper trimming. The method signature with sensible defaults is well-designed for cleanup scenarios.
456-466: LGTM!Excellent idempotent error handling for 404s. Treating already-absent files as success is appropriate for cleanup operations in test scenarios, and the structured logging clearly indicates the operation status.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
package.json (1)
14-14: Remove explicit file path fromtest:importscript—testMatch pattern already handles file filtering.The
playwright.config.tsproperly implements testMatch-based filtering for import tests:testMatch: 'templates/**/*.test.ts'is conditionally registered whenENABLE_IMPORT_TESTS=true. This means the explicit file pathtests/templates/import_templates.test.tsin thetest:importscript (line 20) is redundant.All other test scripts rely on testMatch patterns alone:
test:e2e: usestestMatch: 'tssc/**/*.test.ts'(no explicit path)test:tssc: usestestMatchpatterns (no explicit path)test:ui: usestestMatch: 'ui/**/*.test.ts'(no explicit path)Remove the file path from line 20 to achieve consistency and improve maintainability. Change:
"test:import": "npm run generate-config && ENABLE_E2E_TESTS=false ENABLE_UI_TESTS=false ENABLE_IMPORT_TESTS=true playwright test tests/templates/import_templates.test.ts"to:
"test:import": "npm run generate-config && ENABLE_E2E_TESTS=false ENABLE_UI_TESTS=false ENABLE_IMPORT_TESTS=true playwright test"The environment flags and testMatch configuration will correctly isolate and run only import tests without the explicit path specification.
src/api/gitlab/gitlab.client.ts (1)
329-385: Stop retrying on 404 responses in getRepositoryTree.Inside the
retrywrapper you rethrow 404s, but without callingbail()async-retry will still schedule the remaining attempts. That means an absent path burns all three retries before the outer catch finally turns it into an empty array, increasing latency and load in our end-to-end runs. Please wire inbail(error)so we return immediately on 404 and only retry truly transient failures.- const tree = await retry( - async () => { + const tree = await retry( + async (bail) => { try { return await this.client.Repositories.allRepositoryTrees(projectId, { path: trimmedPath, ref: trimmedBranch, }); } catch (error: any) { if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) { - // Rethrow 404 errors immediately to prevent retries - throw error; + bail(error); + return; } throw error; } },As per coding guidelines.
src/api/bitbucket/services/bitbucket-repository.service.ts (1)
147-167: Serialize the delete payload as form-urlencoded.This request sets
Content-Type: application/x-www-form-urlencodedbut sends a raw object. Axios will JSON-stringify the object, which Bitbucket rejects because it expects proper form fields (e.g.message=value&branch=value&files=path). As a result the delete endpoint will fail and our cleanup tests won’t remove the file. Please encode the payload before sending it.- const commitData = { - message: commitMessage, - branch: branch, - files: { - [filePath]: null - } - }; - - await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, commitData, { + const form = new URLSearchParams(); + form.append('message', commitMessage); + form.append('branch', branch); + form.append('files', filePath); + + await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, form.toString(), { headers: { 'Content-Type': 'application/x-www-form-urlencoded', }, });(If the Bitbucket client already offers a helper for multipart/form submissions, reusing it would work too—the key point is we must send a properly encoded form.)
As per coding guidelines.src/api/github/services/github-repository.service.ts (1)
241-259: Return a GithubApiError for invalid deleteFile parameters.The new validation paths throw plain
Errors. Callers of this service consistently expectGithubApiError(or subclasses) so they can handle failures uniformly. Surfacing a rawErrorchanges that contract and makes our test cleanup harder to reason about. Please wrap these validation failures inGithubApiError(status 400 is fine) the same way the rest of the class does.- if (!owner || owner.trim() === '') { - throw new Error('Owner is required and cannot be empty'); - } + if (!owner || owner.trim() === '') { + throw new GithubApiError('Owner is required and cannot be empty', 400); + }Apply the same pattern for the other validation branches so every failure path returns a
GithubApiError.
Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (12)
package.json(1 hunks)playwright.config.ts(4 hunks)src/api/bitbucket/services/bitbucket-repository.service.ts(2 hunks)src/api/bitbucket/types/bitbucket.types.ts(1 hunks)src/api/github/services/github-repository.service.ts(2 hunks)src/api/gitlab/gitlab.client.ts(4 hunks)src/api/gitlab/types/gitlab.types.ts(1 hunks)src/api/rhdh/developerhub.ts(2 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/api/bitbucket/types/bitbucket.types.ts
- src/rhtap/core/integration/git/providers/bitbucket.ts
- src/rhtap/core/integration/git/providers/github.ts
- tests/templates/import_templates.test.ts
- playwright.config.ts
- src/api/gitlab/types/gitlab.types.ts
- src/rhtap/core/integration/git/providers/gitlab.ts
🧰 Additional context used
📓 Path-based instructions (1)
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-repository.service.tssrc/api/gitlab/gitlab.client.tssrc/api/rhdh/developerhub.tssrc/api/bitbucket/services/bitbucket-repository.service.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging tests
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Run UI tests after a successful backend E2E test and execute UI tests in parallel for release testing
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
Repo: redhat-appstudio/tssc-test PR: 114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/api/github/services/github-repository.service.tssrc/api/rhdh/developerhub.ts
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
Repo: redhat-appstudio/tssc-test PR: 72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging tests
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Ensure project configuration files are present before running tests (generate via `npm run generate-config` or create manually)
Applied to files:
package.json
📚 Learning: 2025-08-21T08:57:44.968Z
Learnt from: Katka92
Repo: redhat-appstudio/tssc-test PR: 119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.968Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Applies to docs/playwright-report/** : Logs, screenshots, and videos generated by UI tests should be saved to the `playwright-report` folder
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Run all UI tests using the command `npm run test:ui`
Applied to files:
package.json
🧬 Code graph analysis (3)
src/api/github/services/github-repository.service.ts (2)
src/log/logger.ts (1)
defaultLogger(113-113)src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
src/api/gitlab/gitlab.client.ts (2)
src/api/gitlab/types/gitlab.types.ts (2)
ProjectIdentifier(198-198)RepositoryTreeNode(45-51)src/log/logger.ts (1)
defaultLogger(113-113)
src/api/bitbucket/services/bitbucket-repository.service.ts (2)
src/api/bitbucket/types/bitbucket.types.ts (2)
BitbucketDirectoryEntry(87-94)BitbucketPaginatedResponse(96-103)src/log/logger.ts (1)
defaultLogger(113-113)
⏰ 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)
package.json (1)
18-18: New test:report script improves test artifact accessibility.The addition of
playwright show-reportis a convenient way to view test results, aligning with observability best practices.
| try { | ||
| const response = await retry( | ||
| async () => { | ||
| return await this.httpClient.get<BitbucketPaginatedResponse<BitbucketDirectoryEntry>>( | ||
| `/repositories/${trimmedWorkspace}/${trimmedRepoSlug}/src/${trimmedRef}/${trimmedPath}` | ||
| ); | ||
| }, | ||
| { | ||
| retries: 3, | ||
| minTimeout: 1000, | ||
| maxTimeout: 5000, | ||
| factor: 2, | ||
| onRetry: (error: Error, attempt: number) => { | ||
| defaultLogger.warn({ | ||
| operation: 'getDirectoryContent', | ||
| workspace: trimmedWorkspace, | ||
| repoSlug: trimmedRepoSlug, | ||
| path: trimmedPath, | ||
| ref: trimmedRef, | ||
| attempt, | ||
| error: error.message | ||
| }, `Retrying directory content retrieval (attempt ${attempt}/3)`); | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| // Defensive validation of response | ||
| if (!response || !Array.isArray(response.values)) { | ||
| defaultLogger.warn({ | ||
| operation: 'getDirectoryContent', | ||
| workspace: trimmedWorkspace, | ||
| repoSlug: trimmedRepoSlug, | ||
| path: trimmedPath, | ||
| ref: trimmedRef, | ||
| responseType: typeof response, | ||
| hasValues: !!response?.values | ||
| }, `Invalid response structure for directory content, returning empty array`); | ||
| return []; | ||
| } | ||
|
|
||
| defaultLogger.info({ | ||
| operation: 'getDirectoryContent', | ||
| workspace: trimmedWorkspace, | ||
| repoSlug: trimmedRepoSlug, | ||
| path: trimmedPath, | ||
| ref: trimmedRef, | ||
| itemCount: response.values.length | ||
| }, `Successfully retrieved directory content for ${trimmedWorkspace}/${trimmedRepoSlug}/${trimmedPath}`); | ||
|
|
||
| return response.values; | ||
| } catch (error: any) { | ||
| // Handle 404 errors gracefully for idempotent operations | ||
| if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) { | ||
| defaultLogger.info({ | ||
| operation: 'getDirectoryContent', | ||
| workspace: trimmedWorkspace, | ||
| repoSlug: trimmedRepoSlug, | ||
| path: trimmedPath, | ||
| ref: trimmedRef, | ||
| status: 'not_found' | ||
| }, `Directory content not found for ${trimmedWorkspace}/${trimmedRepoSlug}/${trimmedPath} (404 Not Found)`); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Don’t retry directory lookups after a 404.
retry currently wraps the GET without any bail-out, so a missing directory forces three attempts before falling through to the 404 handler that returns an empty array. That slows our Bitbucket workflows and burns rate limit for an idempotent “not found” case. Please pass bail into the retry function and call it when you detect a 404 so we short-circuit immediately.
- const response = await retry(
- async () => {
+ const response = await retry(
+ async (bail) => {
try {
return await this.httpClient.get<BitbucketPaginatedResponse<BitbucketDirectoryEntry>>(
`/repositories/${trimmedWorkspace}/${trimmedRepoSlug}/src/${trimmedRef}/${trimmedPath}`
);
- } catch (error: any) {
- throw error;
+ } catch (error: any) {
+ if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) {
+ bail(error);
+ return;
+ }
+ throw error;
}
},As per coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
RHTAP-2547 Add import test
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/gitlab/gitlab.client.ts (1)
95-257: Initialize the service fields before calling them.
mergeMergeRequest,configWebhook, and the pipeline helpers dereferencethis.mergeRequestService,this.webhookService, andthis.pipelineService, but those fields are never assigned—onlythis.mergeRequests,this.webhooks, andthis.pipelinesare. At runtime each call hitsCannot read properties of undefined, so every new API surface added here is broken. Reuse the existing service instances (or assign these fields in the constructor) before exposing the methods.- mergeRequestService: any; - webhookService: any; - pipelineService: any; + mergeRequestService: GitLabMergeRequestService; + webhookService: GitLabWebhookService; + pipelineService: GitLabPipelineService; @@ - this.mergeRequests = new GitLabMergeRequestService( - this.client, - this.repositories - ); - this.webhooks = new GitLabWebhookService(this.client, this.projects); - this.pipelines = new GitLabPipelineService(this.client); + this.mergeRequestService = new GitLabMergeRequestService( + this.client, + this.repositories + ); + this.mergeRequests = this.mergeRequestService; + this.webhookService = new GitLabWebhookService(this.client, this.projects); + this.webhooks = this.webhookService; + this.pipelineService = new GitLabPipelineService(this.client); + this.pipelines = this.pipelineService;
♻️ Duplicate comments (3)
src/api/bitbucket/services/bitbucket-repository.service.ts (2)
147-168: Form-encode Bitbucket delete payload properly.
Right now we POST a plain object with a nestedfilesmap while announcingapplication/x-www-form-urlencoded. Bitbucket’s/srcendpoint expects URL-encoded key/value pairs (files=/path/to/file) and will reject the current shape, so the delete never lands—hurting cleanup reliability for the import suite. Serialize the payload withURLSearchParams(or similar) and send eachfilesfield as its own entry.- const commitData = { - message: commitMessage, - branch: branch, - files: { - [filePath]: null // null value indicates file deletion - } - }; - - await this.httpClient.post(`/repositories/${workspace}/${repoSlug}/src`, commitData, { - headers: { - 'Content-Type': 'application/x-www-form-urlencoded', - }, - }); + const payload = new URLSearchParams(); + payload.append('message', commitMessage); + payload.append('branch', branch); + payload.append('files', filePath); + + await this.httpClient.post( + `/repositories/${workspace}/${repoSlug}/src`, + payload.toString(), + { + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + }, + }, + );
72-144: Stop retrying deterministic 404s in directory lookup.
Line 122 already short-circuits 404s, but without usingbailinside the retry block each not-found folder still burns all three attempts and the associated delays/log noise. That drags out cleanup-heavy flows and wastes API quota during import tests. Please detect 404s inside the retry lambda and callbail(error)so we fail fast on idempotent misses.- const response = await retry( - async () => { - return await this.httpClient.get<BitbucketPaginatedResponse<BitbucketDirectoryEntry>>( - `/repositories/${trimmedWorkspace}/${trimmedRepoSlug}/src/${trimmedRef}/${trimmedPath}` - ); - }, + const response = await retry( + async (bail) => { + try { + return await this.httpClient.get<BitbucketPaginatedResponse<BitbucketDirectoryEntry>>( + `/repositories/${trimmedWorkspace}/${trimmedRepoSlug}/src/${trimmedRef}/${trimmedPath}` + ); + } catch (error: any) { + if (error?.response?.status === 404 || error?.status === 404 || error?.message?.includes('404')) { + bail(error); + return; + } + throw error; + } + },src/api/gitlab/gitlab.client.ts (1)
266-440: Bail out retries on GitLab 404s.
BothgetRepositoryTreeanddeleteFilealready treat 404 as “not found” in the outer catch, yet the inner retry closures keep rethrowing the same 404 so async-retry dutifully hammers GitLab three times. That slows down cleanup-heavy scenarios and clutters logs. Please use thebailcallback to stop retrying once a 404 is detected.- const tree = await retry( - async () => { + const tree = await retry( + async (bail) => { try { return await this.client.Repositories.allRepositoryTrees(projectId, { path: trimmedPath, ref: trimmedBranch, }); } catch (error: any) { // Check if error is 404 - don't retry on 404 errors if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) { - // Rethrow 404 errors immediately to prevent retries - throw error; + bail(error); + return; } // For all other errors, rethrow to let retry mechanism handle them throw error; } }, @@ - await retry( - async () => { + await retry( + async (bail) => { try { await this.client.RepositoryFiles.remove(projectId, trimmedFilePath, trimmedBranch, trimmedCommitMessage); } catch (error: any) { // Check if error is 404 - don't retry on 404 errors if (error.response?.status === 404 || error.status === 404 || error.message?.includes('404')) { - // Rethrow 404 errors immediately to prevent retries - throw error; + bail(error); + return; } // For all other errors, rethrow to let retry mechanism handle them throw error; } },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (12)
package.json(1 hunks)playwright.config.ts(2 hunks)src/api/bitbucket/services/bitbucket-repository.service.ts(2 hunks)src/api/bitbucket/types/bitbucket.types.ts(1 hunks)src/api/github/services/github-repository.service.ts(2 hunks)src/api/gitlab/gitlab.client.ts(3 hunks)src/api/gitlab/types/gitlab.types.ts(1 hunks)src/api/rhdh/developerhub.ts(2 hunks)src/rhtap/core/integration/git/providers/bitbucket.ts(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)src/rhtap/core/integration/git/providers/gitlab.ts(1 hunks)tests/templates/import_templates.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/rhdh/developerhub.ts
- playwright.config.ts
- tests/templates/import_templates.test.ts
- src/rhtap/core/integration/git/providers/github.ts
🧰 Additional context used
📓 Path-based instructions (3)
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/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/gitlab.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/git/providers/bitbucket.tssrc/rhtap/core/integration/git/providers/gitlab.ts
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/gitlab/gitlab.client.tssrc/api/bitbucket/types/bitbucket.types.tssrc/api/github/services/github-repository.service.tssrc/api/gitlab/types/gitlab.types.tssrc/api/bitbucket/services/bitbucket-repository.service.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: jkopriva
Repo: redhat-appstudio/tssc-test PR: 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.
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging tests
Applied to files:
package.json
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
Repo: redhat-appstudio/tssc-test PR: 72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Ensure project configuration files are present before running tests (generate via `npm run generate-config` or create manually)
Applied to files:
package.json
📚 Learning: 2025-08-21T08:57:44.968Z
Learnt from: Katka92
Repo: redhat-appstudio/tssc-test PR: 119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.968Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Applies to docs/playwright-report/** : Logs, screenshots, and videos generated by UI tests should be saved to the `playwright-report` folder
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Run all UI tests using the command `npm run test:ui`
Applied to files:
package.json
📚 Learning: 2025-08-19T06:47:38.630Z
Learnt from: jkopriva
Repo: redhat-appstudio/tssc-test PR: 114
File: src/rhtap/core/integration/git/providers/github.ts:798-807
Timestamp: 2025-08-19T06:47:38.630Z
Learning: In the TSSC E2E testing framework, the maintainer jkopriva prefers to keep error handling consistent across methods within a class, even when idempotency might be beneficial for cleanup scenarios. All deletion methods in GithubProvider should throw errors uniformly rather than treating 404s specially.
Applied to files:
src/rhtap/core/integration/git/providers/bitbucket.tssrc/api/github/services/github-repository.service.tssrc/rhtap/core/integration/git/providers/gitlab.ts
🧬 Code graph analysis (3)
src/api/gitlab/gitlab.client.ts (2)
src/api/gitlab/types/gitlab.types.ts (8)
ProjectIdentifier(198-198)MergeMergeRequestOptions(88-93)MergeResult(100-104)CreateWebhookOptions(151-157)GitLabWebhook(141-149)GitLabPipeline(107-117)GitLabPipelineSearchParams(133-138)RepositoryTreeNode(45-51)src/log/logger.ts (1)
defaultLogger(113-113)
src/api/github/services/github-repository.service.ts (2)
src/log/logger.ts (1)
defaultLogger(113-113)src/api/github/errors/github.errors.ts (1)
GithubApiError(65-70)
src/api/bitbucket/services/bitbucket-repository.service.ts (2)
src/api/bitbucket/types/bitbucket.types.ts (2)
BitbucketDirectoryEntry(88-95)BitbucketPaginatedResponse(97-104)src/log/logger.ts (1)
defaultLogger(113-113)
⏰ 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 (4)
package.json (1)
15-15:⚠️ TypeScript configuration blocks test:import execution.The script syntax is correct, but it fails immediately because
tsconfig.jsonlacks Node.js type definitions. Thelibarray must include"node"to allow ts-node to compilescripts/generateProjectConfig.ts.Fix required in
tsconfig.json:"lib": ["ES2020", "DOM", "node"]Without this, the
generate-configstep fails with compilation errors for built-in modules (fs,path,process, etc.), preventing the entire import test suite from running. This is a test reliability blocker—add the Node.js lib to restore functionality.⛔ Skipped due to learnings
Learnt from: CR Repo: redhat-appstudio/tssc-test PR: 0 File: docs/UI_TESTS.md:0-0 Timestamp: 2025-09-04T09:02:35.573Z Learning: Ensure project configuration files are present before running tests (generate via `npm run generate-config` or create manually)Learnt from: rhopp Repo: redhat-appstudio/tssc-test PR: 72 File: package.json:15-15 Timestamp: 2025-07-31T07:43:28.355Z Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.Learnt from: CR Repo: redhat-appstudio/tssc-test PR: 0 File: docs/UI_TESTS.md:0-0 Timestamp: 2025-09-04T09:02:35.573Z Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging testsLearnt from: CR Repo: redhat-appstudio/tssc-test PR: 0 File: docs/UI_TESTS.md:0-0 Timestamp: 2025-09-04T09:02:35.573Z Learning: Applies to docs/playwright-report/** : Logs, screenshots, and videos generated by UI tests should be saved to the `playwright-report` folderLearnt from: CR Repo: redhat-appstudio/tssc-test PR: 0 File: docs/UI_TESTS.md:0-0 Timestamp: 2025-09-04T09:02:35.573Z Learning: Run all UI tests using the command `npm run test:ui`src/rhtap/core/integration/git/providers/bitbucket.ts (3)
975-991: LGTM! Clean existence check implementation.The method correctly returns boolean for existence while properly handling 404s and propagating unexpected errors.
993-1011: LGTM! Consistent with repository existence check.File existence detection follows the same clean pattern as
checkIfRepositoryExists.
1022-1032: Deletion error handling aligns with maintainer preference.The method throws on all errors, including 404s. This is consistent with the maintainer's preference for uniform error handling across deletion methods. Based on learnings.
| if (!owner || owner.trim() === '') { | ||
| throw new Error('Owner is required and cannot be empty'); | ||
| } | ||
| if (!repo || repo.trim() === '') { | ||
| throw new Error('Repository name is required and cannot be empty'); | ||
| } | ||
| if (!path || path.trim() === '') { | ||
| throw new Error('File path is required and cannot be empty'); | ||
| } | ||
| if (!message || message.trim() === '') { | ||
| throw new Error('Commit message is required and cannot be empty'); | ||
| } | ||
| if (!sha || sha.trim() === '') { | ||
| throw new Error('File SHA is required and cannot be empty'); | ||
| } | ||
| if (!branch || branch.trim() === '') { | ||
| throw new Error('Branch is required and cannot be empty'); | ||
| } | ||
|
|
||
| const trimmedOwner = owner.trim(); | ||
| const trimmedRepo = repo.trim(); | ||
| const trimmedPath = path.trim(); | ||
| const trimmedMessage = message.trim(); | ||
| const trimmedSha = sha.trim(); | ||
| const trimmedBranch = branch.trim(); | ||
|
|
||
| try { | ||
| await retry( | ||
| async () => { | ||
| await this.octokit.repos.deleteFile({ | ||
| owner: trimmedOwner, | ||
| repo: trimmedRepo, | ||
| path: trimmedPath, | ||
| message: trimmedMessage, | ||
| sha: trimmedSha, | ||
| branch: trimmedBranch, | ||
| }); | ||
| }, | ||
| { | ||
| retries: 3, | ||
| minTimeout: 1000, | ||
| maxTimeout: 5000, | ||
| factor: 2, | ||
| onRetry: (error: any, attempt: number) => { | ||
| const status = error.status || error.response?.status; | ||
| const errorCode = error.code; | ||
|
|
||
| // Check for retryable HTTP status codes | ||
| const isHttpRetryable = status === 429 || (status >= 500 && status < 600); | ||
|
|
||
| // Check for retryable transport error codes | ||
| const retryableTransportCodes = [ | ||
| 'ETIMEDOUT', | ||
| 'ECONNRESET', | ||
| 'ENOTFOUND', | ||
| 'EAI_AGAIN', | ||
| 'ECONNREFUSED', | ||
| 'EHOSTUNREACH', | ||
| 'ENETUNREACH', | ||
| 'ETIMEOUT' | ||
| ]; | ||
| const isTransportRetryable = errorCode && retryableTransportCodes.includes(errorCode); | ||
|
|
||
| const isRetryable = isHttpRetryable || isTransportRetryable; | ||
|
|
||
| if (isRetryable) { | ||
| defaultLogger.warn({ | ||
| operation: 'deleteFile', | ||
| owner: trimmedOwner, | ||
| repo: trimmedRepo, | ||
| path: trimmedPath, | ||
| attempt, | ||
| status, | ||
| errorCode, | ||
| error: error.message | ||
| }, `Retrying file deletion (attempt ${attempt}/3) - Status: ${status}, Code: ${errorCode}`); | ||
| } else { | ||
| // Non-retryable error, throw immediately | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| ); | ||
|
|
||
| defaultLogger.info({ | ||
| operation: 'deleteFile', | ||
| owner: trimmedOwner, | ||
| repo: trimmedRepo, | ||
| path: trimmedPath, | ||
| branch: trimmedBranch | ||
| }, `Successfully deleted file ${trimmedPath} from ${trimmedOwner}/${trimmedRepo}`); | ||
| } catch (error: any) { | ||
| const status = error.status || error.response?.status; | ||
|
|
||
| defaultLogger.error({ | ||
| operation: 'deleteFile', | ||
| owner: trimmedOwner, | ||
| repo: trimmedRepo, | ||
| path: trimmedPath, | ||
| branch: trimmedBranch, | ||
| error: error.message, | ||
| status | ||
| }, `Failed to delete file ${trimmedPath} from ${trimmedOwner}/${trimmedRepo} - Status: ${status}`); | ||
|
|
||
| throw new GithubApiError(`Failed to delete file ${trimmedPath} from ${trimmedOwner}/${trimmedRepo}`, status, error); | ||
| } |
There was a problem hiding this comment.
Normalize deleteFile validation to GithubApiError
Input validation currently throws plain Error, so upstream cleanup flows that rely on catching GithubApiError will miss these failures and lose status metadata—undermining the uniform error contract we rely on for retries and diagnostics. Please mirror the repository-level validation by aggregating missing fields and throwing GithubApiError with a 400 status instead of raw Error.
[suggested_fix]
- if (!owner || owner.trim() === '') {
- throw new Error('Owner is required and cannot be empty');
- }
- if (!repo || repo.trim() === '') {
- throw new Error('Repository name is required and cannot be empty');
- }
- if (!path || path.trim() === '') {
- throw new Error('File path is required and cannot be empty');
- }
- if (!message || message.trim() === '') {
- throw new Error('Commit message is required and cannot be empty');
- }
- if (!sha || sha.trim() === '') {
- throw new Error('File SHA is required and cannot be empty');
- }
- if (!branch || branch.trim() === '') {
- throw new Error('Branch is required and cannot be empty');
- }
-
- const trimmedOwner = owner.trim();
- const trimmedRepo = repo.trim();
- const trimmedPath = path.trim();
- const trimmedMessage = message.trim();
- const trimmedSha = sha.trim();
- const trimmedBranch = branch.trim();
+ const trimmedOwner = owner.trim();
+ const trimmedRepo = repo.trim();
+ const trimmedPath = path.trim();
+ const trimmedMessage = message.trim();
+ const trimmedSha = sha.trim();
+ const trimmedBranch = branch.trim();
+
+ const missingFields: string[] = [];
+ if (!trimmedOwner) missingFields.push('owner');
+ if (!trimmedRepo) missingFields.push('repo');
+ if (!trimmedPath) missingFields.push('path');
+ if (!trimmedMessage) missingFields.push('message');
+ if (!trimmedSha) missingFields.push('sha');
+ if (!trimmedBranch) missingFields.push('branch');
+
+ if (missingFields.length > 0) {
+ throw new GithubApiError(
+ `Invalid deleteFile parameters: missing or empty ${missingFields.join(', ')}`,
+ 400,
+ new Error(`Missing required fields: ${missingFields.join(', ')}`)
+ );
+ }Based on learnings
🤖 Prompt for AI Agents
In src/api/github/services/github-repository.service.ts around lines 242 to 347,
the input validation currently throws plain Error for missing/empty
owner/repo/path/message/sha/branch; change this to aggregate the names of any
missing or empty trimmed fields and throw a GithubApiError with status 400
(include the aggregated message and any relevant metadata) instead of throwing
raw Error so upstream logic can catch GithubApiError and retain status and error
info; ensure you trim inputs first (use trimmedOwner/trimmedRepo/etc.) when
validating/collecting missing fields and construct the GithubApiError with the
400 status and the original or aggregated error details.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
integration-tests/config/testplan.json(2 hunks)integration-tests/tasks/tssc-e2e.yaml(1 hunks)package.json(1 hunks)src/rhtap/core/integration/git/providers/github.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rhtap/core/integration/git/providers/github.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging tests
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
Repo: redhat-appstudio/tssc-test PR: 72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
integration-tests/config/testplan.jsonintegration-tests/tasks/tssc-e2e.yamlpackage.json
📚 Learning: 2025-08-19T06:42:15.469Z
Learnt from: jkopriva
Repo: redhat-appstudio/tssc-test PR: 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:
integration-tests/tasks/tssc-e2e.yaml
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Run UI tests after a successful backend E2E test and execute UI tests in parallel for release testing
Applied to files:
integration-tests/tasks/tssc-e2e.yamlpackage.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Run all UI tests using the command `npm run test:ui`
Applied to files:
integration-tests/tasks/tssc-e2e.yamlpackage.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Use `npm run test:ui-interactive` to open Playwright UI for running and debugging tests
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Ensure project configuration files are present before running tests (generate via `npm run generate-config` or create manually)
Applied to files:
package.json
📚 Learning: 2025-08-21T08:57:44.968Z
Learnt from: Katka92
Repo: redhat-appstudio/tssc-test PR: 119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.968Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
package.json
📚 Learning: 2025-09-04T09:02:35.573Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: docs/UI_TESTS.md:0-0
Timestamp: 2025-09-04T09:02:35.573Z
Learning: Applies to docs/playwright-report/** : Logs, screenshots, and videos generated by UI tests should be saved to the `playwright-report` folder
Applied to files:
package.json
⏰ 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 (3)
package.json (2)
15-16: LGTM. The test:import and test:backend scripts are syntactically correct and properly use TESTPLAN_NAME with comma-separated values where needed. Once line 14 is fixed, these will execute the intended test orchestration.
19-19: Orchestration flow is correct. The test:all script runs backend tests first (with generate-config), then UI tests sequentially, which aligns with the expected test workflow and the learning that UI tests should run after successful backend E2E.Also applies to: 19-19
integration-tests/tasks/tssc-e2e.yaml (1)
145-145: LGTM. The switch fromtest:e2etotest:backendcorrectly aligns with the test plan restructuring. Sincetest:backendnow invokes both the renamed e2e-tests and the new import-tests plans, this maintains the original semantic scope (running all backend tests) while incorporating the new import test coverage.Also applies to: 145-145
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
14-19: Optional: normalize spacing for consistency.Lines 14-16 and 19 have double spacing before
playwright test(two spaces after&&). While this doesn't affect functionality, normalizing to single spacing would improve consistency across all scripts.- "test:e2e": "TESTPLAN_NAME=e2e-tests npm run generate-config && playwright test", - "test:import": "TESTPLAN_NAME=import-tests npm run generate-config && playwright test", - "test:backend": "TESTPLAN_NAME=e2e-tests,import-tests npm run generate-config && playwright test", + "test:e2e": "TESTPLAN_NAME=e2e-tests npm run generate-config && playwright test", + "test:import": "TESTPLAN_NAME=import-tests npm run generate-config && playwright test", + "test:backend": "TESTPLAN_NAME=e2e-tests,import-tests npm run generate-config && playwright test",- "test:all": "TESTPLAN_NAME=e2e-tests,import-tests npm run generate-config && playwright test && TESTPLAN_NAME=ui-tests playwright test", + "test:all": "TESTPLAN_NAME=e2e-tests,import-tests npm run generate-config && playwright test && TESTPLAN_NAME=ui-tests playwright test",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)tests/templates/import_templates.test.ts(1 hunks)tests/tssc/full_workflow.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/templates/import_templates.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
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
🧠 Learnings (2)
📚 Learning: 2025-07-31T07:43:28.355Z
Learnt from: rhopp
Repo: redhat-appstudio/tssc-test PR: 72
File: package.json:15-15
Timestamp: 2025-07-31T07:43:28.355Z
Learning: In the tssc-test project, the `test:ui` script intentionally does not include the `generate-config` step. This is expected behavior because UI tests are designed to run in scenarios where the project-configs.json file already exists - either from prior E2E test execution or from manual template creation in RHDH.
Applied to files:
package.json
📚 Learning: 2025-08-21T08:57:44.968Z
Learnt from: Katka92
Repo: redhat-appstudio/tssc-test PR: 119
File: playwright.config.ts:109-112
Timestamp: 2025-08-21T08:57:44.968Z
Learning: In the TSSC E2E testing framework, trace retention should not be enabled in Playwright configuration for CI jobs due to security concerns, as traces may leak sensitive information.
Applied to files:
package.json
🔇 Additional comments (3)
tests/tssc/full_workflow.test.ts (2)
46-48: Component naming change improves test isolation.The addition of the "-workflow" suffix ensures component name uniqueness when running multiple test suites concurrently, reducing the risk of collisions and flaky test failures.
33-77: Add test teardown to properly clean up component resources after test suite completion.The test suite creates actual components and resource instances (Component, ArgoCD, CI, Git) that are never explicitly cleaned up. The
ComponentCleanupActionclass exists in the codebase to handle this, but it's never invoked in this test file. Additionally, the fixture provides no teardown logic afterawait use(testItem).Add an
test.afterAll()hook to execute cleanup:test.afterAll(async () => { if (component) { const cleanupAction = new ComponentCleanupAction(component); await cleanupAction.execute(); } });Alternatively, enhance
createBasicFixtureto handle resource cleanup in its teardown phase. This ensures test environment stability across runs and prevents resource leaks, especially in serial test suites where state accumulates.package.json (1)
14-16: Scripts follow consistent pattern and align with expanded test plans.The new test:import script and updated test:e2e and test:backend scripts properly set TESTPLAN_NAME environment variables and call generate-config before running Playwright tests. This aligns well with the expanded test plan structure (e2e-tests and import-tests) introduced in this PR.
|
@jkopriva: 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-ssn4dTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
2 participants
Summary by CodeRabbit
New Features
Tests
Chores