Skip to content

Conversation

@xinredhat
Copy link
Member

@xinredhat xinredhat commented Nov 27, 2025

Summary by CodeRabbit

  • New Features

    • Jenkins/GitOps auth now uses the real Git username for credentials.
    • Added an option to enable GitOps auth username in Jenkinsfile modifications.
  • Refactor

    • Simplified Jenkinsfile modification pipeline by removing unused Kubernetes agent and TPA variable updates.
  • Tests

    • Updated integration test plan: converted matrix to an array and revised entries.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 27, 2025

Walkthrough

Adds 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

Cohort / File(s) Summary
Git Interface & Provider
src/rhtap/core/integration/git/gitInterface.ts, src/rhtap/core/integration/git/baseGitProvider.ts
Added getUsername(): string to the Git interface and implemented it in BaseGitProvider. The provider validates secret.username and throws if absent.
Jenkinsfile Modifications
src/rhtap/modification/jenkinsfile.ts
Added EnableGitoAuthUsernameModification, added GITOPS_AUTH_USERNAME to JenkinsfileModificationType, updated JenkinsfileModificationFactory.create to return the new modification, and added enableGitoAuthUsername() to JenkinsfileModifier.
Jenkins Secrets Command
src/rhtap/postcreation/strategies/commands/addJenkinsSecretsCommand.ts
Replaced hard-coded "fakeUsername" with this.git.getUsername() when constructing the GitOps credential formatted as <username>:<password>.
Jenkinsfile Modification Chains
src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnGitopsRepoCommand.ts, src/rhtap/postcreation/strategies/commands/jenkinsfileAndEnvModificationsOnSourceRepoCommand.ts
Removed updateKubernetesAgentConfig() (both) and enableTPAVariables() (GitOps repo) from the chained JenkinsfileModifier calls so those modifications are no longer requested.
Integration Tests Config
integration-tests/config/testplan.json
Converted tssc from a single-entry object to a multi-entry array, retained the first entry, removed several old entries, and added a final Bitbucket/Jenkins/Quay entry.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas needing attention:
    • Verify all concrete Git provider implementations inherit/implement getUsername() and that missing usernames produce clear, testable errors.
    • Confirm secret construction (<username>:<password>) does not leak credentials to logs or telemetry and follows secure handling patterns.
    • Validate the new Jenkinsfile modification emits the correct snippet and is correctly created by the factory.
    • Review removal of updateKubernetesAgentConfig() and enableTPAVariables() for potential test or pipeline behavior impacts.

Possibly related PRs

Suggested reviewers

  • chetna14manku
  • jkopriva

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing GitOps auth password when using Jenkins as a pipeline provider. This matches the core objective of the PR which adds username handling to the GitOps authentication flow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 coverage

The new GITOPS_AUTH_USERNAME wiring (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 use GITOPS_.... Renaming to GitOps for both would make this easier to grep and reason about.
  • getAllJenkinsfileModifications() does not include enableGitoAuthUsername(). 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 around applyModification for the GITOPS_AUTH_USERNAME line 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 new getUsername contract and make sure providers/tests align

Adding getUsername(): string to the Git interface 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-BaseGitProvider implementations 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 expectations

Switching from a hard-coded fakeUsername to this.git.getUsername() makes the GitOps auth credential much more realistic and should fix Jenkins behavior, assuming the integration secret has username set.

For long-term reliability and security:

  • Add/extend tests for each supported GitType to assert that the resulting credential value matches what the Jenkins side expects, and that missing username in the secret fails fast (as BaseGitProvider.getUsername() now does).
  • Double‑check that ensureServicesInitialized() always loads the integration secret before addGitAuthSecrets() runs so getUsername() never sees an uninitialized secret, and verify that none of the logging paths ever dump ${username}:${password}.
src/rhtap/core/integration/git/baseGitProvider.ts (1)

169-174: Clarify precondition on getUsername and cover it with integration tests

The getUsername() implementation is consistent with getHost() and gives a clear error when the secret lacks a username, which is good for debugging misconfigurations.

Because it relies on this.secret already being loaded, consider:

  • Explicitly documenting (class- or method-level) that callers must ensure getIntegrationSecret() has been invoked before using getUsername() (and getHost()), 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 uninitialized secret and that the error message is surfaced cleanly when username is missing.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 67ef722 and b533174.

📒 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.ts
  • src/rhtap/core/integration/git/baseGitProvider.ts
  • src/rhtap/modification/jenkinsfile.ts
  • src/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.ts
  • src/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.ts
  • src/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 xinredhat force-pushed the fix_jenkins_git-auth-password branch from b533174 to d19eec9 Compare November 27, 2025 06:26
@xinredhat xinredhat force-pushed the fix_jenkins_git-auth-password branch from d19eec9 to 475bc3d Compare November 27, 2025 07:41
@xinredhat xinredhat force-pushed the fix_jenkins_git-auth-password branch from 475bc3d to 43dc507 Compare November 27, 2025 07:42
Copy link

@coderabbitai coderabbitai bot left a 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 tests

The new getUsername(): string is 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 Git implementations 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

📥 Commits

Reviewing files that changed from the base of the PR and between d19eec9 and 43dc507.

📒 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.ts
  • integration-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 in testplan.json template with required fields: templates, tssc, and optional tests array for filtering
Support multiple test plans in a single testplan.json file using the testPlans array format, each with unique name, templates, tssc, and tests properties

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.json file maintains proper compliance with the required structure:

  • ✓ Outer testPlans array with unique test plans
  • ✓ Each test plan contains required fields: name, templates, tssc, and optional tests array for filtering
  • tssc is 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.

@konflux-ci-qe-bot
Copy link

@xinredhat: The following test has Failed, say /retest to rerun failed tests.

PipelineRun Name Status Rerun command Build Log Test Log
e2e-4.19-g2hdx Failed /retest View Pipeline Log View Test Logs

Inspecting Test Artifacts

To inspect your test artifacts, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.19-g2hdx

Test results analysis

<not enabled>

OCI Artifact Browser URL

<not enabled>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants