Skip to content

Conversation

@gustavolira
Copy link
Member

… subshells

  • Skip orchestrator E2E tests in mandatory PR job (e2e-ocp-helm) to speed up PRs
  • Skip orchestrator-infra chart installation in PR jobs
  • Skip orchestrator workflows deployment in PR jobs
  • Add PR-specific Helm values that disable orchestrator plugins and config
  • Export log functions in log.sh to make them available in subshells (fixes 'command not found' errors)
  • Update CI documentation to reflect orchestrator is nightly-only
  • Orchestrator tests continue running in all nightly jobs with full coverage

This change reduces PR job execution time by avoiding unnecessary orchestrator infrastructure setup while maintaining complete orchestrator test coverage in nightly runs across all platforms (OCP, AKS, EKS, GKE).

Description

Please explain the changes you made here.

Which issue(s) does this PR fix

  • Fixes #?

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@openshift-ci openshift-ci bot requested review from psrna and subhashkhileri January 9, 2026 16:17
@openshift-ci
Copy link

openshift-ci bot commented Jan 9, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign josephca for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Job Detection

The PR-only behavior is gated by substring checks on process.env.JOB_NAME. Validate this reliably matches all intended presubmit job names and cannot accidentally match unrelated jobs (e.g., names containing pull or e2e-ocp-helm as substrings). Consider centralizing/mirroring the exact same detection logic used in the pipeline scripts to avoid drift between CI behavior and Playwright test selection.

const isPrE2eOcpHelmJob =
  process.env.JOB_NAME.includes("pull") &&
  process.env.JOB_NAME.includes("e2e-ocp-helm");
Temp File Collision

The merged PR Helm values are written to fixed paths under /tmp (e.g., merged-values_showcase_PR.yaml). If multiple deployments run on the same runner/host (or if the script is re-entered), these fixed filenames can collide and cause cross-contamination between runs. Prefer using mktemp (optionally under ${ARTIFACT_DIR}) and ensure cleanup/traceability of which merged file was applied.

if is_pr_e2e_ocp_helm_job; then
  local merged_pr_value_file="/tmp/merged-values_showcase_PR.yaml"
  yq_merge_value_files "merge" "${DIR}/value_files/${HELM_CHART_VALUE_FILE_NAME}" "${DIR}/value_files/diff-values_showcase_PR.yaml" "${merged_pr_value_file}"
  mkdir -p "${ARTIFACT_DIR}/${NAME_SPACE}"
  cp -a "${merged_pr_value_file}" "${ARTIFACT_DIR}/${NAME_SPACE}/" || true
  # shellcheck disable=SC2046
  helm upgrade -i "${RELEASE_NAME}" -n "${NAME_SPACE}" \
    "${HELM_CHART_URL}" --version "${CHART_VERSION}" \
    -f "${merged_pr_value_file}" \
    --set global.clusterRouterBase="${K8S_CLUSTER_ROUTER_BASE}" \
    $(get_image_helm_set_params)
else
📚 Focus areas based on broader codebase context

Values Shape

The PR sets orchestrator: null to disable orchestrator in PR jobs. Validate that the Helm chart actually supports orchestrator being null (as opposed to an object with an enabled field), otherwise template lookups like .Values.orchestrator.enabled can break or behave unexpectedly. (Ref 1, Ref 2)

# Disable orchestrator for PRs to speed up the mandatory presubmit job and avoid extra infra installs.
orchestrator: null

Reference reasoning: The existing CI values in the chart configure orchestrator as a structured map (orchestrator: with enabled: true), implying templates likely expect orchestrator to be an object. Using null changes the value shape and can cause rendering errors or conditional logic to mis-evaluate compared to the established pattern.

📄 References
  1. redhat-developer/rhdh-chart/charts/backstage/ci/with-orchestrator-values.yaml [1-21]
  2. redhat-developer/rhdh-chart/charts/backstage/ci/with-orchestrator-and-dynamic-plugins-npmrc-values.yaml [1-24]
  3. redhat-developer/rhdh-chart/charts/orchestrator-software-templates/ci/upstream-values.yaml [1-16]
  4. redhat-developer/rhdh-chart/charts/backstage/ci/with-test-pod-disabled-values.yaml [1-12]
  5. redhat-developer/rhdh-chart/charts/orchestrator-infra/values.yaml [37-41]
  6. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/values.yaml [141-163]
  7. redhat-developer/rhdh-chart/charts/orchestrator-software-templates-infra/ci/upstream-values.yaml [1-27]
  8. redhat-developer/rhdh-chart/charts/orchestrator-infra/templates/tests/infra-test.yaml [0-2]

@rhdh-qodo-merge
Copy link

PR Type

Enhancement, Tests


Description

  • Skip orchestrator tests and infra setup in mandatory PR job to reduce execution time

  • Add PR-specific Helm value files to disable orchestrator plugins and configuration

  • Implement conditional logic to detect PR job and skip orchestrator-related deployments

  • Update CI documentation to reflect orchestrator is nightly-only

  • Maintain full orchestrator test coverage in all nightly jobs across platforms


File Walkthrough

Relevant files
Enhancement
playwright.config.ts
Skip orchestrator tests in PR job                                               

e2e-tests/playwright.config.ts

  • Add detection logic for PR e2e-ocp-helm job using JOB_NAME environment
    variable
  • Conditionally exclude orchestrator E2E tests from PR job by adding
    them to skip list
  • Tests remain included in nightly jobs for full coverage
+7/-0     
utils.sh
Conditionally skip orchestrator setup in PR jobs                 

.ibm/pipelines/utils.sh

  • Add new helper function is_pr_e2e_ocp_helm_job() to detect mandatory
    PR job
  • Update cluster_setup_ocp_helm() to skip orchestrator-infra
    installation for PR jobs
  • Refactor conditional logic to use new helper function for cleaner code
  • Skip orchestrator workflows deployment in base_deployment() for PR
    jobs
  • Skip orchestrator workflows and external DB SSL workaround in
    rbac_deployment() for PR jobs
  • Use PR-specific merged Helm values when deploying in PR jobs
+59/-15 
Configuration changes
diff-values_showcase_PR.yaml
Add PR-specific Helm values for showcase                                 

.ibm/pipelines/value_files/diff-values_showcase_PR.yaml

  • Create new PR-specific Helm values file for showcase deployment
  • Disable orchestrator configuration by setting it to null
  • Disable three orchestrator plugins (orchestrator,
    orchestrator-backend, orchestrator-form-widgets)
  • File is merged with base values only during PR job execution
+19/-0   
diff-values_showcase-rbac_PR.yaml
Add PR-specific Helm values for showcase-rbac                       

.ibm/pipelines/value_files/diff-values_showcase-rbac_PR.yaml

  • Create new PR-specific Helm values file for showcase-rbac deployment
  • Disable orchestrator configuration by setting it to null
  • Disable three orchestrator plugins matching showcase configuration
  • File is merged with base values only during PR job execution
+19/-0   
Documentation
CI.md
Document orchestrator nightly-only testing                             

docs/e2e-tests/CI.md

  • Add note that orchestrator tests and infra setup are excluded from
    mandatory PR job
  • Clarify that orchestrator tests run in nightly jobs instead
  • Fix indentation and formatting inconsistencies in documentation
  • Improve table formatting for job name format documentation
+13/-15 

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 9, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
invert orchestrator test inclusion

Invert the condition for including orchestrator tests to !isPrE2eOcpHelmJob,
ensuring they are excluded from the PR job as intended.

e2e-tests/playwright.config.ts [87-89]

-...(isPrE2eOcpHelmJob
+...(!isPrE2eOcpHelmJob
   ? ["**/playwright/e2e/plugins/orchestrator/**/*.spec.ts"]
   : []),
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical logic error that would cause the PR to do the opposite of its intent for e2e tests. Applying this fix is essential for the correctness of the PR.

High
High-level
Avoid hardcoding versions in Helm values

The current method of disabling orchestrator plugins by hardcoding versioned
package names in Helm value files is brittle. A more robust solution would be to
modify the base Helm chart to use a single boolean flag to enable or disable the
orchestrator and its associated plugins.

Examples:

.ibm/pipelines/value_files/diff-values_showcase-rbac_PR.yaml [14-17]
      - package: "oci://ghcr.io/redhat-developer/rhdh-plugin-export-overlays/red-hat-developer-hub-backstage-plugin-orchestrator:bs_1.45.3__5.1.0!red-hat-developer-hub-backstage-plugin-orchestrator"
        disabled: true
      - package: "oci://ghcr.io/redhat-developer/rhdh-plugin-export-overlays/red-hat-developer-hub-backstage-plugin-orchestrator-backend:bs_1.45.3__8.3.0!red-hat-developer-hub-backstage-plugin-orchestrator-backend"
        disabled: true
.ibm/pipelines/value_files/diff-values_showcase_PR.yaml [14-17]
      - package: "oci://ghcr.io/redhat-developer/rhdh-plugin-export-overlays/red-hat-developer-hub-backstage-plugin-orchestrator:bs_1.45.3__5.1.0!red-hat-developer-hub-backstage-plugin-orchestrator"
        disabled: true
      - package: "oci://ghcr.io/redhat-developer/rhdh-plugin-export-overlays/red-hat-developer-hub-backstage-plugin-orchestrator-backend:bs_1.45.3__8.3.0!red-hat-developer-hub-backstage-plugin-orchestrator-backend"
        disabled: true

Solution Walkthrough:

Before:

# In .ibm/pipelines/value_files/diff-values_showcase_PR.yaml
global:
  dynamic:
    plugins:
      # Disable orchestrator plugins by matching exact package string with version
      - package: "oci://.../rhdh-plugin-orchestrator:bs_1.45.3__5.1.0!..."
        disabled: true
      - package: "oci://.../rhdh-plugin-orchestrator-backend:bs_1.45.3__8.3.0!..."
        disabled: true
      # ... and so on for other plugins

# In .ibm/pipelines/utils.sh
if is_pr_e2e_ocp_helm_job; then
  # Merge the diff file with hardcoded versions to disable plugins
  yq_merge_value_files "merge" "base-values.yaml" "diff-values_PR.yaml" "merged.yaml"
  helm upgrade ... -f "merged.yaml"
fi

After:

# In the base Helm chart's values.yaml (conceptual change)
orchestrator:
  enabled: true
# ...
global:
  dynamic:
    plugins:
      {{- if .Values.orchestrator.enabled }}
      - package: "oci://.../rhdh-plugin-orchestrator:{{ .Values.versions.orchestrator }}!..."
      - package: "oci://.../rhdh-plugin-orchestrator-backend:{{ .Values.versions.orchestrator_backend }}!..."
      {{- end }}
      # ... other plugins

# In .ibm/pipelines/utils.sh
if is_pr_e2e_ocp_helm_job; then
  # Simply set the boolean flag to false for PR jobs
  helm upgrade ... --set orchestrator.enabled=false
fi
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a brittle implementation where hardcoded version strings in Helm values create future maintenance burdens and risk of breakage; the proposed alternative of using a boolean flag is a much more robust design.

Medium
General
use mktemp for temp files

Use mktemp to generate a unique temporary filename for merged_pr_value_file to
prevent potential race conditions.

.ibm/pipelines/utils.sh [1066]

-local merged_pr_value_file="/tmp/merged-values_showcase_PR.yaml"
+local merged_pr_value_file="$(mktemp "/tmp/merged-values_showcase_PR.XXXXXX.yaml")"
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a valid improvement for shell scripting best practices, as using mktemp prevents potential race conditions with temporary files, making the script more robust in concurrent environments.

Low
Log a warning on artifact copy failure

Replace || true with a logged warning after the cp command to provide visibility
on potential copy failures without halting the build.

.ibm/pipelines/utils.sh [1069]

-cp -a "${merged_pr_value_file}" "${ARTIFACT_DIR}/${NAME_SPACE}/" || true
+cp -a "${merged_pr_value_file}" "${ARTIFACT_DIR}/${NAME_SPACE}/" || echo "WARNING: Failed to copy ${merged_pr_value_file} to artifacts directory."
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that || true swallows errors. Replacing it with a warning improves the script's robustness and observability without being overly strict, which is a good practice.

Low
safely expand helm params

Capture the output of get_image_helm_set_params into a bash array using mapfile
and expand it safely in the helm upgrade command to avoid word-splitting issues.

.ibm/pipelines/utils.sh [1070-1075]

-# shellcheck disable=SC2046
+mapfile -t image_params < <(get_image_helm_set_params)
 helm upgrade -i "${RELEASE_NAME}" -n "${NAME_SPACE}" \
   "${HELM_CHART_URL}" --version "${CHART_VERSION}" \
   -f "${merged_pr_value_file}" \
   --set global.clusterRouterBase="${K8S_CLUSTER_ROUTER_BASE}" \
-  $(get_image_helm_set_params)
+  "${image_params[@]}"
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion proposes a safer way to handle command output in shell scripts, avoiding potential word-splitting issues and removing the need for a shellcheck disable comment. It's a good practice improvement.

Low
  • Update

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

The image is available at:

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2026

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

/test e2e-ocp-helm

@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

/test e2e-ocp-helm

@gustavolira
Copy link
Member Author

/test ?

@openshift-ci
Copy link

openshift-ci bot commented Jan 10, 2026

@gustavolira: The following commands are available to trigger required jobs:

/test e2e-ocp-helm

The following commands are available to trigger optional jobs:

/test cleanup-mapt-destroy-orphaned-aks-clusters
/test cleanup-mapt-destroy-orphaned-eks-clusters
/test e2e-aks-helm-nightly
/test e2e-aks-operator-nightly
/test e2e-eks-helm-nightly
/test e2e-eks-operator-nightly
/test e2e-gke-helm-nightly
/test e2e-gke-operator-nightly
/test e2e-ocp-helm-nightly
/test e2e-ocp-helm-upgrade-nightly
/test e2e-ocp-operator-auth-providers-nightly
/test e2e-ocp-operator-nightly
/test e2e-ocp-v4-17-helm-nightly
/test e2e-ocp-v4-19-helm-nightly
/test e2e-ocp-v4-20-helm-nightly
/test e2e-osd-gcp-helm-nightly
/test e2e-osd-gcp-operator-nightly

Use /test all to run the following jobs that were automatically triggered:

pull-ci-redhat-developer-rhdh-main-e2e-ocp-helm
Details

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@gustavolira
Copy link
Member Author

/test e2e-ocp-helm-nightly

@openshift-ci
Copy link

openshift-ci bot commented Jan 11, 2026

@gustavolira: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm-nightly e7ac590 link false /test e2e-ocp-helm-nightly

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

… subshells

- Skip orchestrator E2E tests in mandatory PR job (e2e-ocp-helm) to speed up PRs
- Skip orchestrator-infra chart installation in PR jobs
- Skip orchestrator workflows deployment in PR jobs
- Add PR-specific Helm values that disable orchestrator plugins and config
- Export log functions in log.sh to make them available in subshells (fixes 'command not found' errors)
- Update CI documentation to reflect orchestrator is nightly-only
- Orchestrator tests continue running in all nightly jobs with full coverage

This change reduces PR job execution time by avoiding unnecessary orchestrator
infrastructure setup while maintaining complete orchestrator test coverage in
nightly runs across all platforms (OCP, AKS, EKS, GKE).
Skip 'Edit users and groups and update policies of a role from the overview page' test
that is failing due to changes in the RBAC wizard flow. The test needs to be updated
to handle the new wizard step sequence.
Skip 'Test that user with IsOwner condition can access the RBAC page' test
that is also affected by RBAC wizard flow changes.
@gustavolira gustavolira force-pushed the move-orchestrator-nightly branch from e7ac590 to 7ddbf43 Compare January 12, 2026 00:03
@sonarqubecloud
Copy link

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.

1 participant