-
Notifications
You must be signed in to change notification settings - Fork 10
fix gitiops_auth_password when using jenkins as pipeline provider #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix gitiops_auth_password when using jenkins as pipeline provider #241
Conversation
WalkthroughAdds a Git username accessor to the Git abstraction and uses it when creating Jenkins GitOps credentials; introduces a Jenkinsfile modification to enable GITOPS_AUTH_USERNAME; and removes two Jenkinsfile modification calls from post-creation modification chains. Also adjusts the integration test plan tssc entries to an array form with updated entries. 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 `#D3E4FF`: Validates secret.username\nthrows 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
Note right of JMod `#E8F5E9`: Jenkinsfile & secrets updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
b533174 to
d19eec9
Compare
d19eec9 to
475bc3d
Compare
475bc3d to
43dc507
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: 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-g2hdxTest results analysis<not enabled> OCI Artifact Browser URL<not enabled> |
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.