fix gitops_auth_password when using jenkins as pipeline provider #241
Conversation
📝 WalkthroughWalkthroughAdds a Git username accessor to the Git abstraction and implements it in the base provider; uses the username when creating Jenkins GitOps credentials. Introduces a Jenkinsfile modification to enable Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as AddJenkinsSecretsCommand
participant Git as Git Provider
participant JMod as JenkinsfileModifierFactory / Modifier
Cmd->>Git: getUsername()
Note right of Git: validates secret.username and throws if missing
Git-->>Cmd: username
Cmd->>Cmd: construct credential "username:password"
Cmd->>JMod: enableGitoAuthUsername() → get modification snippet
JMod-->>Cmd: modification snippet (GITOPS_AUTH_USERNAME)
Cmd->>JMod: apply modifications / provision secrets
JMod-->>Cmd: confirmation / updated Jenkinsfile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/rhtap/modification/jenkinsfile.ts (1)
113-124: Tighten GITOPS auth username modification naming and coverageThe new
GITOPS_AUTH_USERNAMEwiring (class, enum, factory, modifier) is consistent with the existing pattern, but two small maintainability/testability points:
- The class and method names use
Gito(EnableGitoAuthUsernameModification,enableGitoAuthUsername) while the enum and credential ID useGITOPS_.... Renaming toGitOpsfor both would make this easier to grep and reason about.getAllJenkinsfileModifications()does not includeenableGitoAuthUsername(). If the intent is that “all Jenkinsfile modifications” should also flip this credential on, consider adding it there or documenting why it’s excluded. An accompanying unit test aroundapplyModificationfor theGITOPS_AUTH_USERNAMEline would guard Jenkinsfile behavior over time.Also applies to: 135-135, 161-162, 237-243
src/rhtap/core/integration/git/gitInterface.ts (1)
119-119: Document newgetUsernamecontract and make sure providers/tests alignAdding
getUsername(): stringto theGitinterface makes the Jenkins auth flow more explicit, which is good. To keep integrations and tests reliable:
- Add a brief JSDoc on
getUsername()clarifying that it comes from the integration secret and is used for Git/Jenkins authentication.- Ensure all non-
BaseGitProviderimplementations and any test doubles/mocks implement this method and are covered by at least a basic integration/E2E check so Jenkins credential creation doesn’t silently regress for any provider.src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts (1)
65-72: Good fix; add tests around username sourcing and Jenkins expectationsSwitching from a hard-coded
fakeUsernametothis.git.getUsername()makes the GitOps auth credential much more realistic and should fix Jenkins behavior, assuming the integration secret hasusernameset.For long-term reliability and security:
- Add/extend tests for each supported
GitTypeto assert that the resulting credential value matches what the Jenkins side expects, and that missingusernamein the secret fails fast (asBaseGitProvider.getUsername()now does).- Double‑check that
ensureServicesInitialized()always loads the integration secret beforeaddGitAuthSecrets()runs sogetUsername()never sees an uninitializedsecret, and verify that none of the logging paths ever dump${username}:${password}.src/rhtap/core/integration/git/baseGitProvider.ts (1)
169-174: Clarify precondition ongetUsernameand cover it with integration testsThe
getUsername()implementation is consistent withgetHost()and gives a clear error when the secret lacks a username, which is good for debugging misconfigurations.Because it relies on
this.secretalready being loaded, consider:
- Explicitly documenting (class- or method-level) that callers must ensure
getIntegrationSecret()has been invoked before usinggetUsername()(andgetHost()), and- Adding integration/E2E tests that exercise the typical initialization path (e.g., via
ensureServicesInitialized()) to prove that Jenkins secret creation can’t hit an uninitializedsecretand that the error message is surfaced cleanly whenusernameis missing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/rhtap/core/integration/git/baseGitProvider.ts(1 hunks)src/rhtap/core/integration/git/gitInterface.ts(1 hunks)src/rhtap/modification/jenkinsfile.ts(4 hunks)src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts(2 hunks)src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts(0 hunks)src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts
- src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,json}
📄 CodeRabbit inference engine (README.md)
All TypeScript code must pass linting checks, type checking, and formatting validation through
npm run validate
Files:
src/rhtap/core/integration/git/gitInterface.tssrc/rhtap/core/integration/git/baseGitProvider.tssrc/rhtap/modification/jenkinsfile.tssrc/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.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/gitInterface.tssrc/rhtap/core/integration/git/baseGitProvider.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/gitInterface.tssrc/rhtap/core/integration/git/baseGitProvider.ts
src/rhtap/postcreation/**
⚙️ CodeRabbit configuration file
Review command pattern implementation, strategy pattern usage, credential handling security, error handling for external service calls, and ensure proper cleanup of resources. Validate secret management practices.
Files:
src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts
🧬 Code graph analysis (1)
src/rhtap/modification/jenkinsfile.ts (2)
src/rhtap/core/integration/git/index.ts (1)
ContentModifications(25-25)src/rhtap/modification/contentModification.ts (1)
ContentModifications(12-12)
xinredhat
force-pushed
the
fix_jenkins_git-auth-password
branch
from
b533174 to
d19eec9
Compare
xinredhat
force-pushed
the
fix_jenkins_git-auth-password
branch
2 times, most recently
from
475bc3d to
43dc507
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/rhtap/core/integration/git/gitInterface.ts (1)
118-119: Username accessor fits the Git contract; consider documenting semantics and tightening testsThe new
getUsername(): stringis consistent with the existing getters (getToken,getHost) and is the right abstraction point for the Jenkins GitOps creds flow. From a maintainability and E2E test perspective, two follow‑ups would help:
- Add a short JSDoc on
getUsername()(similar to other methods) clarifying what it returns and whether it throws when the underlying secret is missing or malformed. That makes it easier for tests to assert the correct failure behavior and for callers to understand expectations.- Ensure all
Gitimplementations and integration tests are updated to cover both the “happy path” (valid username present) and the misconfiguration path (no username / empty username), since this method will now be critical to the Jenkins credentials setup.Overall, the interface change itself looks sound; the main risk is missing implementations or tests rather than the signature here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
integration-tests/config/testplan.json(2 hunks)src/rhtap/core/integration/git/baseGitProvider.ts(1 hunks)src/rhtap/core/integration/git/gitInterface.ts(1 hunks)src/rhtap/modification/jenkinsfile.ts(4 hunks)src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts(2 hunks)src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts(0 hunks)src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts
- src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts
- src/rhtap/core/integration/git/baseGitProvider.ts
- src/rhtap/modification/jenkinsfile.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,json}
📄 CodeRabbit inference engine (README.md)
All TypeScript code must pass linting checks, type checking, and formatting validation through
npm run validate
Files:
src/rhtap/core/integration/git/gitInterface.tsintegration-tests/config/testplan.json
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/gitInterface.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/gitInterface.ts
**/testplan.json
📄 CodeRabbit inference engine (README.md)
**/testplan.json: Test plan configuration must follow the structure defined intestplan.jsontemplate with required fields: templates, tssc, and optional tests array for filtering
Support multiple test plans in a single testplan.json file using thetestPlansarray format, each with uniquename,templates,tssc, andtestsproperties
Files:
integration-tests/config/testplan.json
🧠 Learnings (4)
📚 Learning: 2025-11-25T12:32:09.336Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: README.md:0-0
Timestamp: 2025-11-25T12:32:09.336Z
Learning: Applies to **/testplan.json : Support multiple test plans in a single testplan.json file using the `testPlans` array format, each with unique `name`, `templates`, `tssc`, and `tests` properties
Applied to files:
integration-tests/config/testplan.json
📚 Learning: 2025-11-25T12:32:09.336Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: README.md:0-0
Timestamp: 2025-11-25T12:32:09.336Z
Learning: Applies to **/testplan.json : Test plan configuration must follow the structure defined in `testplan.json` template with required fields: templates, tssc, and optional tests array for filtering
Applied to files:
integration-tests/config/testplan.json
📚 Learning: 2025-11-25T12:32:09.336Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: README.md:0-0
Timestamp: 2025-11-25T12:32:09.336Z
Learning: When filtering tests through the `tests` array in testplan.json, support both directory-based filtering (e.g., 'ui', 'tssc') and file-based filtering (e.g., 'component.test.ts')
Applied to files:
integration-tests/config/testplan.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:
integration-tests/config/testplan.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)
integration-tests/config/testplan.json (2)
1-67: Inconsistency between AI summary and actual code changes.The AI summary claims that six tssc entries were removed (gitlab/tekton/nexus, gitlab/gitlabci/quay, github/githubactions/artifactory, github/azure/quay, github/jenkins/quay, gitlab/jenkins/quay), but these entries are all still present in the current file (lines 14–55) with no change markers (
~). Only the structural brackets and new bitbucket entry show modifications.Please clarify which tssc entries were intentionally retained and whether the AI summary accurately reflects the changes.
2-67: Test plan structure follows coding guidelines correctly.The
testplan.jsonfile maintains proper compliance with the required structure:
- ✓ Outer
testPlansarray with unique test plans- ✓ Each test plan contains required fields:
name,templates,tssc, and optionaltestsarray for filtering- ✓
tsscis now properly formatted as an array of configuration objects (supporting multiple combinations)- ✓ JSON syntax is valid
The addition of the bitbucket/jenkins/quay entry (lines 56–62) follows established pattern conventions and integrates cleanly with existing entries.
xinredhat
changed the title
xinredhat
force-pushed
the
fix_jenkins_git-auth-password
branch
from
1e48728 to
f1ec2b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts (1)
17-18: Log message references wrong repository type.The
logStartcall at line 18 logs'source repository modifications', but this command handles GitOps repository modifications. This could cause confusion when debugging pipeline issues.📝 Proposed fix
public async execute(): Promise<void> { - this.logStart('source repository modifications'); + this.logStart('GitOps repository modifications');
🧹 Nitpick comments (1)
src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts (1)
51-58: Consider adding error handling for commit failures.The
commitChangesmethod logs success but doesn't handle potential failures gracefully. While the async call will propagate exceptions, adding explicit error logging before re-throwing would improve debuggability when investigating test failures.🛡️ Optional: Enhanced error handling
private async commitChanges( repoName: string, modifications: ContentModifications, message: string ): Promise<void> { console.log(`Committing changes to ${repoName}...`); - await this.git.commitChangesToRepo(this.git.getRepoOwner(), repoName, modifications, message); - console.log(`Changes committed to ${repoName} successfully.`); + try { + await this.git.commitChangesToRepo(this.git.getRepoOwner(), repoName, modifications, message); + console.log(`Changes committed to ${repoName} successfully.`); + } catch (error) { + console.error(`Failed to commit changes to ${repoName}:`, error); + throw error; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
integration-tests/config/testplan.jsonsrc/rhtap/core/integration/git/baseGitProvider.tssrc/rhtap/core/integration/git/gitInterface.tssrc/rhtap/modification/jenkinsfile.tssrc/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.tssrc/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.tssrc/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rhtap/core/integration/git/baseGitProvider.ts
- src/rhtap/core/integration/git/gitInterface.ts
- src/rhtap/modification/jenkinsfile.ts
- src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/rhtap/postcreation/**
⚙️ CodeRabbit configuration file
Review command pattern implementation, strategy pattern usage, credential handling security, error handling for external service calls, and ensure proper cleanup of resources. Validate secret management practices.
Files:
src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.tssrc/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts
🧠 Learnings (3)
📚 Learning: 2026-01-08T11:53:04.866Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: README.md:0-0
Timestamp: 2026-01-08T11:53:04.866Z
Learning: Applies to testplan.json : Multiple test plans can be defined using the 'testPlans' array format with each plan containing name, templates, tssc, and tests fields
Applied to files:
integration-tests/config/testplan.json
📚 Learning: 2026-01-08T11:53:04.866Z
Learnt from: CR
Repo: redhat-appstudio/tssc-test PR: 0
File: README.md:0-0
Timestamp: 2026-01-08T11:53:04.866Z
Learning: Applies to testplan.json : Test plan configuration must be defined in `testplan.json` file at the root directory
Applied to files:
integration-tests/config/testplan.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:
integration-tests/config/testplan.json
🔇 Additional comments (4)
integration-tests/config/testplan.json (2)
6-63: Array structure fortsscentries is correctly implemented.The conversion to an array format aligns with the documented test plan configuration pattern supporting multiple tssc entries. All entries maintain consistent field structure.
56-62: Bitbucket/Jenkins test coverage appropriately extends E2E validation.Adding the
bitbucket/jenkins/quaytest plan entry properly validates the Jenkins credential fix across Git providers. Bitbucket is a fully supported provider with completegetUsername()implementation that integrates seamlessly with the Jenkins secret injection flow viaaddJenkinsSecretsCommand, ensuring the gitops_auth_password fix works reliably when using Bitbucket workspaces with Jenkins.src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts (1)
40-46: Consistent modification chain with GitOps repository command.The
.enableGitoAuthUsername()addition mirrors the change in the GitOps repo command, ensuring both source and GitOps repositories are properly configured with the authentication username fix. This consistency is important for the Jenkins pipeline to work correctly across repositories.src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts (1)
34-40: Clarify that this is a new GitOps command file, not a modification removing existing methods.The builder chain is intentional and correctly focused on the GitOps repository workflow. The file was created (not modified) by commit f1ec2b0 with the explicit purpose of fixing
gitops_auth_password. The methodsupdateKubernetesAgentConfig()andenableTPAVariables()were never part of this command's workflow—they belong to different modification contexts (Kubernetes agent setup and TPA configuration respectively). The fluent builder chain with.enableRegistryPassword(),.disableQuayCredentials(), and.enableGitoAuthUsername()correctly addresses the PR objective and reflects the segmented design of source vs. GitOps repository modifications. No issues with credential handling security or resource cleanup in this implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@xinredhat: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.20-pm85tTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
3 participants
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.