test: refactor pairing with UI code#1227
test: refactor pairing with UI code#1227rsoaresd merged 24 commits intocodeready-toolchain:masterfrom
Conversation
WalkthroughRebrands Sandbox UI to Developer Sandbox Dashboard across CI/makeflows, adds a new manage-devsandbox-dashboard.sh script to publish/deploy the dashboard, updates E2E targets/docs/tests, and switches deployment manifests and helpers to use DEVSANDBOX_DASHBOARD_NS. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI
participant Script as manage-devsandbox-dashboard.sh
participant Quay
participant OpenShift
participant Kustomize as kustomize/envsubst
CI->>Script: invoke (PUBLISH_UI?, DEPLOY_UI?, image/tag, ns, creds)
alt PUBLISH_UI = true
Script->>Quay: build/push image (local or forced tag)
Quay-->>Script: image URL / success
end
alt DEPLOY_UI = true
Script->>OpenShift: ensure namespace exists
Script->>Kustomize: render manifests (envsubst)
Kustomize->>OpenShift: apply manifests
Script->>OpenShift: create/patch OAuth IDP secret & patch OAuth config
OpenShift-->>Script: rollout status / service URL
end
Script-->>CI: exit status (and deployment URL if deployed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
deploy/devsandbox-dashboard/base/deployment.yaml (1)
62-62: Consider standardizing image versioning strategy.The init container uses a digest-pinned image (sha256:796...) while the main container uses a tag (
:1.5). This mixed approach could lead to version drift between the containers.Consider using consistent versioning (either both digest-pinned or both tag-based) to ensure the init and runtime containers remain compatible.
Also applies to: 83-83
test/e2e/devsandbox-dashboard/homepage_test.go (1)
7-7: Consider updating the import alias to match the new package name.The import uses the alias
sandboxuifor the renameddevsandbox-dashboardpackage. While this works and minimizes code changes in this refactoring PR, it creates a naming mismatch that could confuse future developers.For better clarity, consider either:
- Removing the alias to use the actual package name
devsandboxdashboard:- sandboxui "github.com/codeready-toolchain/toolchain-e2e/testsupport/devsandbox-dashboard" + devsandboxdashboard "github.com/codeready-toolchain/toolchain-e2e/testsupport/devsandbox-dashboard"Then update all references from
sandboxui.todevsandboxdashboard.throughout the file.
- Or use a more neutral alias like
dsdif brevity is preferred:- sandboxui "github.com/codeready-toolchain/toolchain-e2e/testsupport/devsandbox-dashboard" + dsd "github.com/codeready-toolchain/toolchain-e2e/testsupport/devsandbox-dashboard"scripts/ci/manage-devsandbox-dashboard.sh (3)
23-23: Prefer[[overtestcommand.The
[[construct is safer and more feature-rich in bash. Based on static analysis hint.- while test $# -gt 0; do + while [[ $# -gt 0 ]]; do
99-101: Handle pre-existing secret gracefully.If the secret already exists from a previous run,
oc create secretwill fail. Consider using the dry-run/apply pattern for idempotency.- oc create secret generic ${OPENID_SECRET_NAME} \ - --from-literal=clientSecret=dummy \ - --namespace=openshift-config + oc create secret generic ${OPENID_SECRET_NAME} \ + --from-literal=clientSecret=dummy \ + --namespace=openshift-config \ + --dry-run=client -o yaml | oc apply -f -
163-164: Prefer[[over[for conditional tests.Consistent with other bash best practices in the script. Based on static analysis hints.
- SSO_USERNAME_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) - SSO_PASSWORD_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) + SSO_USERNAME_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) + SSO_PASSWORD_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi)make/devsandbox-dashboard.mk (1)
89-104: Handle missing KUBECONFIG gracefully.If
KUBECONFIGis not set, the podman volume mount will fail. Consider defaulting to~/.kube/config.@echo "running the e2e tests in podman container..." podman run --platform $(PLATFORM) --rm \ - -v $(KUBECONFIG):/root/.kube/config \ + -v $(or $(KUBECONFIG),$(HOME)/.kube/config):/root/.kube/config \ -e KUBECONFIG=/root/.kube/config \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml(0 hunks).gitignore(1 hunks)deploy/devsandbox-dashboard/base/deployment.yaml(1 hunks)deploy/devsandbox-dashboard/base/route.yaml(1 hunks)deploy/devsandbox-dashboard/base/service-accounts.yaml(1 hunks)deploy/devsandbox-dashboard/base/service.yaml(1 hunks)make/devsandbox-dashboard.mk(1 hunks)make/sandbox-ui.mk(0 hunks)make/test.mk(2 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)test/e2e/devsandbox-dashboard/README.md(1 hunks)test/e2e/devsandbox-dashboard/activities_page_test.go(1 hunks)test/e2e/devsandbox-dashboard/header_test.go(1 hunks)test/e2e/devsandbox-dashboard/homepage_test.go(1 hunks)test/e2e/devsandbox-dashboard/setup/setup_test.go(1 hunks)test/e2e/sandbox-ui/README.md(0 hunks)testsupport/devsandbox-dashboard/login.go(1 hunks)testsupport/devsandbox-dashboard/popup.go(1 hunks)testsupport/devsandbox-dashboard/setup.go(2 hunks)testsupport/devsandbox-dashboard/trace.go(2 hunks)testsupport/devsandbox-dashboard/utils.go(1 hunks)testsupport/init.go(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml
- test/e2e/sandbox-ui/README.md
- make/sandbox-ui.mk
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/devsandbox-dashboard/setup/setup_test.go (1)
testsupport/init.go (1)
WaitForDevSandboxDashboard(306-344)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 3-3: Add an explicit return statement at the end of the function.
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 17-17: Add an explicit return statement at the end of the function.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 24-24: Assign this positional parameter to a local variable.
[warning] 67-67: Add an explicit return statement at the end of the function.
[failure] 164-164: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 163-163: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 107-107: Add an explicit return statement at the end of the function.
[warning] 59-59: Assign this positional parameter to a local variable.
🪛 LanguageTool
test/e2e/devsandbox-dashboard/README.md
[style] ~8-~8: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ... from ClusterBot will not work since we are not able to modify the OAuth configuration of ROSA ...
(BE_ABLE_TO)
[style] ~27-~27: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
test/e2e/devsandbox-dashboard/README.md
15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 131-131: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (3)
- GitHub Check: Unit Tests
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (26)
.gitignore (1)
342-343: LGTM!The path and comment updates correctly reflect the Developer Sandbox Dashboard naming.
deploy/devsandbox-dashboard/base/route.yaml (1)
5-5: LGTM!The namespace variable update is correct and aligns with the refactoring to Developer Sandbox Dashboard.
deploy/devsandbox-dashboard/base/service.yaml (1)
5-5: LGTM!The namespace variable update is consistent with the other manifest files.
test/e2e/devsandbox-dashboard/header_test.go (2)
1-1: LGTM!Package rename is consistent with the broader refactoring to Developer Sandbox Dashboard.
6-6: LGTM!Import path update correctly reflects the new test support package location.
test/e2e/devsandbox-dashboard/activities_page_test.go (2)
1-1: LGTM!Package rename aligns with the Developer Sandbox Dashboard refactoring.
7-7: LGTM!Import path update is consistent with the test support package reorganization.
deploy/devsandbox-dashboard/base/service-accounts.yaml (1)
5-5: LGTM!Namespace variable update is consistent with the deployment refactoring.
deploy/devsandbox-dashboard/base/deployment.yaml (1)
5-5: LGTM!Namespace variable update is consistent with the other manifest files in this refactoring.
testsupport/init.go (1)
305-307: Unable to verify migration completeness due to repository access limitations.I attempted to verify the migration by searching the codebase for lingering references to the old function name (
WaitForSandboxUI) and environment variable (SANDBOX_UI_NS), but encountered repository access issues that prevented the verification from running. Additionally, web searches did not return relevant public information about this specific migration.To complete this verification, you will need to manually run the verification script provided in the review comment, or provide access to the repository logs/CI output showing whether:
- All references to
WaitForSandboxUIhave been replaced withWaitForDevSandboxDashboard- All references to
SANDBOX_UI_NShave been replaced withDEVSANDBOX_DASHBOARD_NStest/e2e/devsandbox-dashboard/setup/setup_test.go (1)
10-11: LGTM!The update from
WaitForSandboxUItoWaitForDevSandboxDashboardis correct and aligns with the package-wide rename to Developer Sandbox Dashboard.testsupport/devsandbox-dashboard/trace.go (2)
1-1: LGTM!The package rename from
sandboxuitodevsandboxdashboardis consistent with the repository-wide refactoring.
33-34: LGTM!The CI artifact path update from
sandbox-uitodevsandbox-dashboardcorrectly aligns with the new naming convention.testsupport/devsandbox-dashboard/utils.go (1)
1-1: LGTM!Package rename is consistent with the refactoring effort.
testsupport/devsandbox-dashboard/popup.go (1)
1-1: LGTM!Package rename is consistent with the broader refactoring.
testsupport/devsandbox-dashboard/setup.go (2)
1-1: LGTM!Package rename aligns with the refactoring to Developer Sandbox Dashboard.
25-25: LGTM!The config file path correctly points to the new
devsandbox-dashboarddirectory structure.testsupport/devsandbox-dashboard/login.go (1)
1-1: LGTM!Package rename is consistent with the repository-wide refactoring.
test/e2e/devsandbox-dashboard/homepage_test.go (2)
1-1: LGTM!Package declaration correctly updated to
devsandboxdashboard.
15-15: LGTM!Comment accurately reflects the updated Developer Sandbox Dashboard terminology.
make/test.mk (2)
54-54: LGTM!The echo message correctly references the new
clean-devsandbox-dashboardtarget, consistent with the renaming across the PR.
297-304: LGTM!The addition of
get-and-publish-devsandbox-dashboardas a dependency forpublish-current-bundles-for-e2ecorrectly integrates the new dashboard publishing into the CI workflow. ThePUBLISH_UI=trueandDEPLOY_UI=falseprerequisites onget-and-publish-operatorsare appropriate since this target focuses solely on publishing.scripts/ci/manage-devsandbox-dashboard.sh (3)
67-94: LGTM!Good practice to validate SSO credentials before proceeding with deployment. The HTTP status check provides clear feedback on credential issues.
107-115: LGTM!Good idempotent namespace creation logic with proper existence check.
40-40: I encountered a repository clone failure, so I cannot directly verify the shell scripts. However, I need to verify the core claim in your review comment: whether the variablesPROVIDED_RHDH_PLUGINS_PATH,DATE_SUFFIX,FORCED_TAG, andPROVIDED_REPOSITORY_PATHare actually used by the sourcedmanage-operator.shscript.To proceed with verification, could you please provide one of the following:
Run the verification script yourself and share the output:
rg -n 'PROVIDED_RHDH_PLUGINS_PATH|PROVIDED_REPOSITORY_PATH|DATE_SUFFIX|FORCED_TAG' scripts/ci/manage-operator.shShare the relevant sections of
scripts/ci/manage-operator.shthat use these variables, orConfirm whether these variables are indeed used in the functions mentioned (
get_repo,set_tags,push_image)Once I have this information, I can provide a properly verified rewritten review comment.
make/devsandbox-dashboard.mk (1)
31-48: LGTM!Well-structured e2e test execution flow with proper Playwright setup, environment configuration, and cleanup (usersignup deletion).
| *) | ||
| echo "$1 is not a recognized flag!" >> /dev/stderr | ||
| user_help | ||
| exit -1 | ||
| ;; |
There was a problem hiding this comment.
Use valid exit code.
exit -1 is invalid; shell exit codes must be in the range 0-255. Use exit 1 instead. Based on shellcheck SC2242.
*)
echo "$1 is not a recognized flag!" >> /dev/stderr
user_help
- exit -1
+ exit 1
;;📝 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.
| *) | |
| echo "$1 is not a recognized flag!" >> /dev/stderr | |
| user_help | |
| exit -1 | |
| ;; | |
| *) | |
| echo "$1 is not a recognized flag!" >> /dev/stderr | |
| user_help | |
| exit 1 | |
| ;; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 59-59: Assign this positional parameter to a local variable.
🪛 Shellcheck (0.11.0)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
🤖 Prompt for AI Agents
In scripts/ci/manage-devsandbox-dashboard.sh around lines 58 to 62, the script
uses an invalid exit code "exit -1"; shell exit codes must be between 0 and 255,
so replace "exit -1" with a valid non-zero code such as "exit 1" (or another
appropriate 1-255 value) to signal failure; update the line accordingly and keep
the rest of the error handling intact.
| if [[ ${DEPLOY_UI} == "true" ]]; then | ||
| # Get the HOST_NS (host operator namespace) | ||
| HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) | ||
|
|
||
| # Get the Registration Service API URL | ||
| REGISTRATION_SERVICE_API="https://$(oc get route registration-service -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')/api/v1" | ||
|
|
||
| # Get the Host Operator API URL | ||
| HOST_OPERATOR_API="https://$(oc get route api -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')" | ||
|
|
||
| # Get the RHDH URL | ||
| RHDH="https://rhdh-${DEVSANDBOX_DASHBOARD_NS}.$(oc get ingress.config.openshift.io/cluster -o jsonpath='{.spec.domain}')" |
There was a problem hiding this comment.
Verify HOST_NS is set before using it.
If no host-operator project exists, HOST_NS will be empty, causing subsequent oc get commands to fail with unclear errors. Add validation after line 150.
# Get the HOST_NS (host operator namespace)
HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1)
+
+ if [[ -z "${HOST_NS}" ]]; then
+ echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed."
+ exit 1
+ fi
# Get the Registration Service API URL📝 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.
| if [[ ${DEPLOY_UI} == "true" ]]; then | |
| # Get the HOST_NS (host operator namespace) | |
| HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) | |
| # Get the Registration Service API URL | |
| REGISTRATION_SERVICE_API="https://$(oc get route registration-service -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')/api/v1" | |
| # Get the Host Operator API URL | |
| HOST_OPERATOR_API="https://$(oc get route api -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')" | |
| # Get the RHDH URL | |
| RHDH="https://rhdh-${DEVSANDBOX_DASHBOARD_NS}.$(oc get ingress.config.openshift.io/cluster -o jsonpath='{.spec.domain}')" | |
| if [[ ${DEPLOY_UI} == "true" ]]; then | |
| # Get the HOST_NS (host operator namespace) | |
| HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) | |
| if [[ -z "${HOST_NS}" ]]; then | |
| echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." | |
| exit 1 | |
| fi | |
| # Get the Registration Service API URL | |
| REGISTRATION_SERVICE_API="https://$(oc get route registration-service -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')/api/v1" | |
| # Get the Host Operator API URL | |
| HOST_OPERATOR_API="https://$(oc get route api -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')" | |
| # Get the RHDH URL | |
| RHDH="https://rhdh-${DEVSANDBOX_DASHBOARD_NS}.$(oc get ingress.config.openshift.io/cluster -o jsonpath='{.spec.domain}')" |
🤖 Prompt for AI Agents
In scripts/ci/manage-devsandbox-dashboard.sh around lines 148 to 159, HOST_NS
may be empty if no host-operator project exists which will make subsequent oc
get calls fail; after the line that assigns HOST_NS (around line 150) add a
validation check that verifies HOST_NS is non-empty and, if it is empty, print a
clear error message to stderr including guidance (e.g., "no host-operator
project found"), and exit with a non-zero status so the script stops before
running the oc get commands that depend on HOST_NS.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/ci/manage-devsandbox-dashboard.sh (3)
58-62: Fix invalid exit code.Line 61 uses
exit -1, which is invalid; shell exit codes must be in the range 0-255. Useexit 1instead.*) echo "$1 is not a recognized flag!" >> /dev/stderr user_help - exit -1 + exit 1 ;;
119-119: Quote array expansion to preserve argument boundaries.Unquoted
$@can cause arguments containing spaces to be split incorrectly. Wrap the expansion in quotes.-read_arguments $@ +read_arguments "$@"
148-159: Add validation for HOST_NS before using it.If no host-operator project exists,
HOST_NSwill be empty (line 150), causing subsequentoc getcommands to fail with unclear errors. Add a validation check after the assignment to ensure HOST_NS is non-empty and exit early if not found.# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >> /dev/stderr + exit 1 + fi # Get the Registration Service API URL
🧹 Nitpick comments (7)
scripts/ci/manage-devsandbox-dashboard.sh (7)
23-23: Use[[instead oftestfor conditional expressions.Line 23 uses
testfor the conditional check. Replace with[[for better safety and consistency with modern Bash practices.- if [[ $# -lt 2 ]] + if (( $# < 2 ))Alternatively, keep the condition as-is but convert to
[[:- if [[ $# -lt 2 ]] + if [[ $# -lt 2 ]](Note: The current code already uses
[[on line 18, so this appears to be a static analysis false positive. Verify the actual line 23 syntax.)
163-164: Use[[instead of[for conditional tests.Lines 163–164 use single-bracket
[for conditionals. Replace with[[for safety and feature-richness (e.g., proper word splitting and globbing handling).- SSO_USERNAME_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) - SSO_PASSWORD_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) + SSO_USERNAME_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) + SSO_PASSWORD_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi)
67-94: Add explicit return statement tocheck_sso_credentials()function.SonarCloud recommends adding an explicit
returnstatement at the end of functions for clarity. While Bash functions implicitly return the status of the last command, explicit returns improve readability.echo "SSO credentials validated successfully" + return 0 }
96-105: Add explicit return statement toconfigure_oauth_idp()function.Add an explicit
returnat the end of this function for consistency and clarity.oc patch oauths.config.openshift.io/cluster --type=merge --patch-file=/dev/stdin + return 0 }
107-115: Add explicit return statement tocreate_namespace()function.Add an explicit
returnat the end of this function for consistency.oc project ${DEVSANDBOX_DASHBOARD_NS} >/dev/null 2>&1 + return 0 }
3-15: Add explicit return statement touser_help()function.The function ends with
exit 0, which terminates the script. For consistency with other functions, consider adding areturn 0before theexit 0if this function is ever called in a non-exit context, or document that it always terminates the script.
17-65: Assign positional parameter$1to a local variable inread_arguments()function.SonarCloud flags line 24 and 59 for reassigning positional parameters directly. Best practice is to assign
$1to a local variable at the start of the loop iteration to avoid confusion and side effects.while test $# -gt 0; do - case "$1" in + local arg="$1" + case "${arg}" inThen update all references in the loop to use
${arg}instead of$1for the case statement, and continue using$1for the shift operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 164-164: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 67-67: Add an explicit return statement at the end of the function.
[warning] 24-24: Assign this positional parameter to a local variable.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 17-17: Add an explicit return statement at the end of the function.
[warning] 59-59: Assign this positional parameter to a local variable.
[warning] 107-107: Add an explicit return statement at the end of the function.
[failure] 163-163: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 131-131: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (3)
- GitHub Check: Unit Tests
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
40-40: I'll help you verify whether these variables are unused or intended to be exported for sourced scripts. Let me start by examining the main script and then checking for references in sourced scripts.
<function_calls>
#!/bin/bashFirst, let's examine the relevant section of manage-devsandbox-dashboard.sh
echo "=== Examining manage-devsandbox-dashboard.sh (lines 30-70) ==="
sed -n '30,70p' scripts/ci/manage-devsandbox-dashboard.sh
</function_calls>
#!/bin/bashCheck for source/exec patterns to see which scripts are sourced
echo "=== Checking for sourced scripts in manage-devsandbox-dashboard.sh ==="
rg -n 'source|^.' scripts/ci/manage-devsandbox-dashboard.sh | head -20
</function_calls>
#!/bin/bashSearch for references to the three flagged variables in manage-operator.sh
echo "=== Checking manage-operator.sh for variable references ==="
rg -n 'PROVIDED_RHDH_PLUGINS_PATH|DATE_SUFFIX|FORCED_TAG|PROVIDED_REPOSITORY_PATH' scripts/ci/manage-operator.sh
</function_calls>
#!/bin/bashLet's check what other scripts exist in the scripts/ci directory
echo "=== Scripts in scripts/ci directory ==="
ls -la scripts/ci/*.sh 2>/dev/null | awk '{print $NF}'
</function_calls>
#!/bin/bashSearch for these variables across all scripts in scripts/ci
echo "=== Searching for variable references across all scripts/ci/*.sh ==="
rg -n 'PROVIDED_RHDH_PLUGINS_PATH|DATE_SUFFIX|FORCED_TAG' scripts/ci/ --type=sh
</function_calls><budget_exhausted:The response has exceeded the allocated token budget. The task cannot be completed as designed. To continue, consider:
- Splitting the work across multiple interactions
- Using fewer or simpler verification steps
- Narrowing the scope of verification
The assistant is unable to continue due to budget constraints.>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
make/devsandbox-dashboard.mk (2)
29-32: Fix.PHONYdeclaration to match the actual e2e target name.Line 29 declares
.PHONY: e2e-run-, but the real target ise2e-run-devsandbox-dashboard. This is a small typo and can confuse tooling or readers.-.PHONY: e2e-run- +.PHONY: e2e-run-devsandbox-dashboard
57-62: UseSSO_USERNAME_READconsistently for usersignup cleanup.
e2e-run-devsandbox-dashboarddeletes the usersignup with$(SSO_USERNAME_READ), butclean-devsandbox-dashboard(Line 62) still uses${SSO_USERNAME}directly. That can break cleanup in CI where only the secret-backed value is present.- @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup ${SSO_USERNAME_READ} -n ${HOST_NS}
🧹 Nitpick comments (2)
make/devsandbox-dashboard.mk (2)
33-37: Resolve Playwright path viago env GOPATHinstead of raw$(GOPATH).Modern Go often leaves
GOPATHunset in the environment; relying on$(GOPATH)/bincan yield/bin/playwrightand fail even thoughgo installsucceeded. Usinggo env GOPATHmakes the Playwright CLI path consistent with where Go actually installs it.For example:
- $(eval PWGO_VER := $(shell grep -oE "playwright-go v\S+" go.mod | sed 's/playwright-go //g')) + $(eval PWGO_VER := $(shell grep -oE "playwright-go v\S+" go.mod | sed 's/playwright-go //g')) + $(eval GO_BIN_DIR := $(shell go env GOPATH)/bin) @echo "Installing Playwright CLI version: $(PWGO_VER)" go install github.com/playwright-community/playwright-go/cmd/playwright@$(PWGO_VER) @echo "Installing Firefox browser for Playwright..." - $(GOPATH)/bin/playwright install firefox + $(GO_BIN_DIR)/playwright install firefox
15-24: Simplify nestedifneqblocks for parameter construction.The double
ifneqaroundUI_REPO_PATHandFORCED_TAGare redundant (same condition checked twice). You can keep the behavior and make it clearer with a single guarded block usingstrip:-ifneq (${UI_REPO_PATH},"") - ifneq (${UI_REPO_PATH},) - $(eval UI_REPO_PATH_PARAM = -ur ${UI_REPO_PATH}) - endif -endif +ifneq ($(strip $(UI_REPO_PATH)),) + $(eval UI_REPO_PATH_PARAM = -ur ${UI_REPO_PATH}) +endif -ifneq (${FORCED_TAG},"") - ifneq (${FORCED_TAG},) - $(eval FORCED_TAG_PARAM = -ft ${FORCED_TAG}) - endif -endif +ifneq ($(strip $(FORCED_TAG)),) + $(eval FORCED_TAG_PARAM = -ft ${FORCED_TAG}) +endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
make/devsandbox-dashboard.mk(1 hunks)
⏰ 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). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
scripts/ci/manage-devsandbox-dashboard.sh (3)
58-62: Use valid exit code and quote positional parameter.Line 61 uses an invalid exit code (
exit -1); shell exit codes must be in range 0–255. Line 59 also uses an unquoted positional parameter that should be assigned to a local variable first for safety.*) - echo "$1 is not a recognized flag!" >> /dev/stderr + local arg="$1" + echo "${arg} is not a recognized flag!" >> /dev/stderr user_help - exit -1 + exit 1 ;;
119-119: Quote array expansion to preserve argument boundaries.Unquoted
$@can cause issues with arguments containing spaces.-read_arguments "$@" +read_arguments "$@"
149-162: Add validation for HOST_NS before using it.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands to fail with unclear errors. Add validation after the assignment.# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >> /dev/stderr + exit 1 + fi # Get the Registration Service API URL
🧹 Nitpick comments (2)
scripts/ci/manage-devsandbox-dashboard.sh (2)
23-23: Standardize on[[for conditional tests.Replace
testwith the safer and more feature-rich[[construct for consistency across the script.- while test $# -gt 0; do + while [[ $# -gt 0 ]]; do
164-166: Standardize on[[for all conditional tests.Mix of
[and[[operators. Use[[throughout for consistency and safety.- SSO_USERNAME_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) - SSO_PASSWORD_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) + SSO_USERNAME_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) + SSO_PASSWORD_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 67-67: Add an explicit return statement at the end of the function.
[warning] 107-107: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 17-17: Add an explicit return statement at the end of the function.
[failure] 165-165: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 24-24: Assign this positional parameter to a local variable.
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 59-59: Assign this positional parameter to a local variable.
[failure] 164-164: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 131-131: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 137-137: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
rsoaresd
force-pushed
the
refactor_pairing_with_ui
branch
from
8279abf to
330c59d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
test/e2e/devsandbox-dashboard/README.md (1)
15-15: Fix heading levels for proper markdown structure.The headings jump from h1 to h3 without an intermediate h2 level. Use h2 (
##) instead of h3 (###) for proper markdown hierarchy.Also applies to: 23-23, 30-30, 42-42
scripts/ci/manage-devsandbox-dashboard.sh (3)
58-62: Use valid exit code.
exit -1is invalid; shell exit codes must be 0-255. Useexit 1instead.
119-119: Quote array expansion to preserve argument boundaries.Unquoted
$@can cause issues with arguments containing spaces. Use"$@"instead.
155-166: Verify HOST_NS is set before using it.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands to fail with unclear errors.make/devsandbox-dashboard.mk (1)
57-62: Inconsistent variable usage persists from previous review.Line 62 uses
${SSO_USERNAME}while line 45 uses$(SSO_USERNAME_READ). A previous review flagged this exact inconsistency and it was marked as addressed in commit 330c59d, but the issue still appears in the current code. In CI environments whereSSO_USERNAMEmay not be directly set as an environment variable, this cleanup step could fail.Apply this diff to use the consistent variable:
.PHONY: clean-devsandbox-dashboard clean-devsandbox-dashboard: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) clean-devsandbox-dashboard: @oc delete ns ${DEVSANDBOX_DASHBOARD_NS} @oc delete secret ${OPENID_SECRET_NAME} -n openshift-config - @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup ${SSO_USERNAME_READ} -n ${HOST_NS}
🧹 Nitpick comments (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
23-23: Consider using[[for conditional tests.While the current syntax works,
[[is safer and more feature-rich thantestand[commands in bash. Based on static analysis hints.Example updates:
-while test $# -gt 0; do +while [[ $# -gt 0 ]]; do-SSO_USERNAME_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) -SSO_PASSWORD_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) +SSO_USERNAME_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) +SSO_PASSWORD_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi)Also applies to: 170-171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml(0 hunks).gitignore(1 hunks)deploy/devsandbox-dashboard/base/deployment.yaml(1 hunks)deploy/devsandbox-dashboard/base/route.yaml(1 hunks)deploy/devsandbox-dashboard/base/service-accounts.yaml(1 hunks)deploy/devsandbox-dashboard/base/service.yaml(1 hunks)make/devsandbox-dashboard.mk(1 hunks)make/sandbox-ui.mk(0 hunks)make/test.mk(2 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)test/e2e/devsandbox-dashboard/README.md(1 hunks)test/e2e/devsandbox-dashboard/activities_page_test.go(1 hunks)test/e2e/devsandbox-dashboard/header_test.go(1 hunks)test/e2e/devsandbox-dashboard/homepage_test.go(1 hunks)test/e2e/devsandbox-dashboard/setup/setup_test.go(1 hunks)test/e2e/sandbox-ui/README.md(0 hunks)testsupport/devsandbox-dashboard/login.go(1 hunks)testsupport/devsandbox-dashboard/popup.go(1 hunks)testsupport/devsandbox-dashboard/setup.go(2 hunks)testsupport/devsandbox-dashboard/trace.go(2 hunks)testsupport/devsandbox-dashboard/utils.go(1 hunks)testsupport/init.go(1 hunks)
💤 Files with no reviewable changes (3)
- test/e2e/sandbox-ui/README.md
- .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml
- make/sandbox-ui.mk
🚧 Files skipped from review as they are similar to previous changes (8)
- testsupport/devsandbox-dashboard/setup.go
- testsupport/init.go
- deploy/devsandbox-dashboard/base/service.yaml
- test/e2e/devsandbox-dashboard/header_test.go
- test/e2e/devsandbox-dashboard/activities_page_test.go
- deploy/devsandbox-dashboard/base/service-accounts.yaml
- deploy/devsandbox-dashboard/base/deployment.yaml
- testsupport/devsandbox-dashboard/utils.go
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/devsandbox-dashboard/setup/setup_test.go (1)
testsupport/init.go (1)
WaitForDevSandboxDashboard(306-344)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[failure] 171-171: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 67-67: Add an explicit return statement at the end of the function.
[warning] 107-107: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 17-17: Add an explicit return statement at the end of the function.
[warning] 24-24: Assign this positional parameter to a local variable.
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 170-170: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 59-59: Assign this positional parameter to a local variable.
🪛 LanguageTool
test/e2e/devsandbox-dashboard/README.md
[style] ~8-~8: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ... from ClusterBot will not work since we are not able to modify the OAuth configuration of ROSA ...
(BE_ABLE_TO)
[style] ~27-~27: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 131-131: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 138-138: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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: Build & push operator bundles for e2e tests
🔇 Additional comments (14)
deploy/devsandbox-dashboard/base/route.yaml (1)
5-5: Namespace variable updated consistently with refactoring objectives.The namespace placeholder has been updated from
${SANDBOX_UI_NS}to${DEVSANDBOX_DASHBOARD_NS}in alignment with the dashboard naming convention. The Route manifest remains otherwise unchanged and correctly references therhdhservice.Verify that the
DEVSANDBOX_DASHBOARD_NSenvironment variable is consistently defined across all deployment contexts and that no orphaned references toSANDBOX_UI_NSremain in related manifests or deployment scripts..gitignore (1)
342-346: LGTM! The gitignore changes appropriately reflect the sandbox-ui → devsandbox-dashboard refactoring. The new.enventry and updated comment are consistent with the test infrastructure reorganization.To confirm these paths align with the actual test infrastructure, please verify the following:
- Does the path
testsupport/devsandbox-dashboard/.envmatch where the E2E test configuration is located?- Should the general
trace/folder entry (line 346) be scoped to devsandbox-dashboard, or is a global trace exclusion appropriate?testsupport/devsandbox-dashboard/popup.go (1)
1-44: LGTM!The popup handling logic is well-implemented with proper retry logic for timeout errors and appropriate load state waiting.
testsupport/devsandbox-dashboard/login.go (1)
1-1: LGTM!Package rename aligns with the broader refactoring from Sandbox UI to Developer Sandbox Dashboard.
testsupport/devsandbox-dashboard/trace.go (2)
1-1: LGTM!Package rename is consistent with the broader refactoring.
33-34: LGTM!Trace artifact path correctly updated to reflect the new devsandbox-dashboard directory structure.
test/e2e/devsandbox-dashboard/setup/setup_test.go (1)
10-11: LGTM!Test setup correctly updated to use the new
WaitForDevSandboxDashboardhelper function.test/e2e/devsandbox-dashboard/README.md (1)
1-44: Well-documented test instructions.The README provides comprehensive guidance on prerequisites, test execution, deployment, and cleanup for the Developer Sandbox Dashboard E2E tests.
test/e2e/devsandbox-dashboard/homepage_test.go (2)
1-1: LGTM!Package rename is consistent with the broader refactoring effort.
7-7: LGTM!Keeping the import alias as
sandboxuiis a pragmatic approach that minimizes code changes while updating the import path.make/test.mk (3)
304-306: LGTM!The addition of UI-related prerequisites (
PUBLISH_UI=true,DEPLOY_UI=false) properly integrates Developer Sandbox Dashboard publishing into the operator build workflow.
54-54: LGTM!Comment properly updated to reflect the new Developer Sandbox Dashboard naming.
297-299: Unable to verify target definition due to repository access failure.The repository clone operation failed, preventing direct verification of whether the
get-and-publish-devsandbox-dashboardtarget is defined in the makefiles. Manual verification is required to confirm that this target exists inmake/devsandbox-dashboard.mkor the relevant included makefile before merging.scripts/ci/manage-devsandbox-dashboard.sh (1)
40-40: Based on my attempt to verify the information, I cannot access the repository directly or retrieve information through web search. However, I can provide analysis based on the context in the review comment.The review comment correctly identifies that shellcheck reports these variables as unused, but the core issue is that shellcheck may not detect usage of variables by sourced scripts. When
scripts/ci/manage-operator.shis sourced, it executes in the same shell context and can access all parent shell variables, even if shellcheck doesn't track this.Since I cannot directly verify whether these variables are actually referenced in the sourced script, and the determination of whether they are "genuinely unused" requires examining the sourced script's code, this requires manual verification.
Verify if reported variables are actually used by the sourced
manage-operator.shscript before removing.Shellcheck reports
PROVIDED_RHDH_PLUGINS_PATH,DATE_SUFFIX,FORCED_TAG,PROVIDED_REPOSITORY_PATH, andREPOSITORY_NAMEas unused. However, these may be referenced in the sourcedscripts/ci/manage-operator.shscript, which runs in the same shell context and can access parent variables. Manual inspection ofmanage-operator.shis needed to confirm whether these variables are genuinely unused or legitimately passed to the sourced script.Also applies to: 50-50, 55-55, 131-131, 138-138
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/ci/manage-devsandbox-dashboard.sh (3)
61-61: Use valid exit code.
exit -1is invalid; shell exit codes must be in the range 0-255. Useexit 1instead.*) echo "$1 is not a recognized flag!" >> /dev/stderr user_help - exit -1 + exit 1 ;;
119-119: Quote array expansion to avoid re-splitting elements.Unquoted
$@can split arguments containing spaces incorrectly.-read_arguments $@ +read_arguments "$@"
155-167: Validate HOST_NS before using it in OpenShift commands.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands to fail with unclear errors. Add validation immediately after the assignment.# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >&2 + exit 1 + fi # Get the Registration Service API URL
🧹 Nitpick comments (2)
scripts/ci/manage-devsandbox-dashboard.sh (2)
23-23: Use[[instead oftestfor conditional tests.The
[[construct is safer and more feature-rich than thetestcommand in bash.- while test $# -gt 0; do + while [[ $# -gt 0 ]]; do
170-171: Use[[instead of[for conditional tests.The
[[construct is safer and more feature-rich than the[command in bash.- SSO_USERNAME_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) - SSO_PASSWORD_READ=$(if [ -n "${CI}" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) + SSO_USERNAME_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_USERNAME 2>/dev/null || echo ""; else echo "${SSO_USERNAME}"; fi) + SSO_PASSWORD_READ=$(if [[ -n "${CI}" ]]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/publish-operators-for-e2e-tests.yml(2 hunks)make/test.mk(2 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/publish-operators-for-e2e-tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- make/test.mk
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 67-67: Add an explicit return statement at the end of the function.
[failure] 171-171: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 170-170: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 59-59: Assign this positional parameter to a local variable.
[warning] 24-24: Assign this positional parameter to a local variable.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 107-107: Add an explicit return statement at the end of the function.
[warning] 17-17: Add an explicit return statement at the end of the function.
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 131-131: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 138-138: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (3)
- GitHub Check: Unit Tests
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
40-40: I apologize, but I'm unable to proceed with the verification due to persistent repository access issues in the sandbox environment. The system is unable to clone the repositorycodeready-toolchain/toolchain-e2e.Given these constraints and the nature of the concern raised in the review comment, here is my rewritten assessment:
Verify variable naming aligns with manage-operator.sh conventions and get_repo() expectations.
Line 40 assigns
PROVIDED_RHDH_PLUGINS_PATHfrom the--ui-repo-pathflag. This may need alignment with themanage-operator.shpattern wherePROVIDED_REPOSITORY_PATHis used with theget_repo()function (sourced on line 127). Confirm whether the naming difference is intentional or ifPROVIDED_RHDH_PLUGINS_PATHshould follow the same convention for compatibility withget_repo().
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Great job 🙌
I have only some minor comments.
I wasn't able to have it fully working on my local crc, but it might be a specific issues with my arm64 crc instance only, I'm still debugging it and trying to have it running.
|
|
||
| .PHONY: test-ui-e2e | ||
| test-ui-e2e: | ||
| $(MAKE) get-and-publish-devsandbox-dashboard e2e-run-devsandbox-dashboard ENVIRONMENT=${UI_ENVIRONMENT} |
There was a problem hiding this comment.
maybe I'm blind, but where is this ENVIRONMENT used ? what is the purpose of having this variable being propagated to those make targets ?
There was a problem hiding this comment.
I set the ENVIRONMENT because in the future maybe we would like to have dev environment too. Now it is used in scripts/ci/manage-devsandbox-dashboard.sh
| 2. Ensure you are using Node.js version 22 | ||
| - to easily manage it, you can run `nvm use 22` | ||
| 3. Ensure you have `yarn` installed | ||
| 4. Make sure you can log in at <https://sso.devsandbox.dev/auth/realms/sandbox-dev/account> using your SSO_USERNAME and SSO_PASSWORD |
There was a problem hiding this comment.
I think it would be safe to share the sso test user in the doc, as others might not have that and there is no ability to create a user there if you're not an admin afaik.
There was a problem hiding this comment.
I agree with you. I will just confirm with the rest of the team via Slack
There was a problem hiding this comment.
We can also just add a note about asking devsandbox team on how obtain the credentials for this test user.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/e2e/devsandbox-dashboard/README.md (3)
8-8: Improve grammar: use "cannot" instead of "not able to".Apply this diff:
- - ROSA cluster from ClusterBot will not work since we are not able to modify the OAuth configuration of ROSA clusters created by the ClusterBot. + - ROSA cluster from ClusterBot will not work since we cannot modify the OAuth configuration of ROSA clusters created by the ClusterBot.
20-20: Reduce phrasing repetition for better readability.Both line 20 and line 28 start with "If you want to"; consider rephrasing one to add variety.
For example:
- Line 20: "To run and test..." instead of "If you want to run and test..."
- Line 28: "To use your local..." instead of "If you want to use your local..."
Also applies to: 28-28
13-13: Consider including alternative make command for Apple chipset users.A past review comment suggested adding the
make prepare-and-deploy-e2e-local PLATFORM=linux/arm64variant for users on Apple chipsets. The current line only mentionsmake prepare-and-deploy-e2e. If this variant is relevant, consider adding it as an alternative option to help Apple users set up their environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/devsandbox-dashboard/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
test/e2e/devsandbox-dashboard/README.md
[style] ~8-~8: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ... from ClusterBot will not work since we are not able to modify the OAuth configuration of ROSA ...
(BE_ABLE_TO)
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...} SSO_PASSWORD=${SSO_PASSWORD}` If you want to run and test the Developer Sandbox Dash...
(REP_WANT_TO_VB)
[style] ~28-~28: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
@rsoaresd this PR looks to be pretty big and I have a feeling that there are a few changes that could be addressed separately - there is the new script, moves of the files to a new folder, drop the old logic, changes in the worklows, changes in the makefiles (maybe something else?).
Let's do the changes incrementally if it's possible.
|
@mfrancisc thank you so much for your suggestions! I addressed them! |
|
@MatousJobanek thank you so much for your suggestion! I will rename the files in a separated PR as well as deleting the |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
scripts/ci/manage-devsandbox-dashboard.sh (4)
99-101: Make secret creation idempotent to avoid script failures on re-runs.
oc create secretwill fail if the secret already exists. Useoc applywith--dry-run=client -o yamlfor idempotency.Apply this diff:
- oc create secret generic ${OPENID_SECRET_NAME} \ - --from-literal=clientSecret=dummy \ - --namespace=openshift-config + oc create secret generic ${OPENID_SECRET_NAME} \ + --from-literal=clientSecret=dummy \ + --namespace=openshift-config \ + --dry-run=client -o yaml | oc apply -f -
159-172: Validate HOST_NS is non-empty before using it.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands to fail with unclear errors.Add validation after line 161:
# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >&2 + exit 1 + fi # Get the Registration Service API URL
61-61: Use valid exit code.Shell exit codes must be in the range 0-255. Replace
exit -1withexit 1.Apply this diff:
- exit -1 + exit 1
119-119: Quote array expansion to prevent argument splitting.Unquoted
$@can cause issues with arguments containing spaces.Apply this diff:
-read_arguments $@ +read_arguments "$@"make/devsandbox-dashboard.mk (2)
15-24: Simplify redundant nested conditionals.The outer conditionals compare against the literal string
""(two quote characters) rather than testing for empty, making them redundant. The inner conditionals already correctly test for non-empty values.Apply this diff:
.PHONY: get-and-publish-devsandbox-dashboard get-and-publish-devsandbox-dashboard: -ifneq (${UI_REPO_PATH},"") - ifneq (${UI_REPO_PATH},) +ifneq (${UI_REPO_PATH},) $(eval UI_REPO_PATH_PARAM = -ur ${UI_REPO_PATH}) - endif endif -ifneq (${FORCED_TAG},"") - ifneq (${FORCED_TAG},) +ifneq (${FORCED_TAG},) $(eval FORCED_TAG_PARAM = -ft ${FORCED_TAG}) - endif endif
62-62: Use consistent variable for SSO username.
clean-devsandbox-dashboarduses${SSO_USERNAME}whilee2e-run-devsandbox-dashboard(line 45) uses${SSO_USERNAME_READ}. This inconsistency could cause failures in CI whereSSO_USERNAMEis not directly set.Apply this diff:
- @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup ${SSO_USERNAME_READ} -n ${HOST_NS}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build/devsandbox-dashboard/Dockerfile(1 hunks)make/devsandbox-dashboard.mk(1 hunks)make/test.mk(2 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)test/e2e/devsandbox-dashboard/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- make/test.mk
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 17-17: Add an explicit return statement at the end of the function.
[warning] 96-96: Add an explicit return statement at the end of the function.
[warning] 24-24: Assign this positional parameter to a local variable.
[failure] 175-175: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
[failure] 23-23: Use '[[' instead of 'test' command for conditional tests. The '[[' construct is safer and more feature-rich.
[warning] 59-59: Assign this positional parameter to a local variable.
[warning] 107-107: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 67-67: Add an explicit return statement at the end of the function.
[failure] 174-174: Use '[[' instead of '[' for conditional tests. The '[[' construct is safer and more feature-rich.
🪛 LanguageTool
test/e2e/devsandbox-dashboard/README.md
[style] ~8-~8: As a shorter alternative for ‘able to’, consider using “can not”.
Context: ... from ClusterBot will not work since we are not able to modify the OAuth configuration of ROSA ...
(BE_ABLE_TO)
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...} SSO_PASSWORD=${SSO_PASSWORD}` If you want to run and test the Developer Sandbox Dash...
(REP_WANT_TO_VB)
[style] ~28-~28: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 40-40: PROVIDED_RHDH_PLUGINS_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 55-55: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 61-61: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[error] 119-119: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 135-135: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 142-142: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (3)
build/devsandbox-dashboard/Dockerfile (1)
79-79: LGTM! CMD target correctly updated.The CMD update aligns with the repository-wide refactoring from Sandbox UI to Developer Sandbox Dashboard E2E tests.
test/e2e/devsandbox-dashboard/README.md (1)
1-47: LGTM! Comprehensive documentation.The README provides clear prerequisites, setup instructions, and test execution commands for both local and containerized environments. The guidance on OCP cluster requirements, SSO credentials, and deployment steps is well-structured.
scripts/ci/manage-devsandbox-dashboard.sh (1)
67-94: LGTM! Credential validation is well-implemented.The SSO credential validation correctly checks both CI and local environments, validates via HTTP POST to the OIDC token endpoint, and provides clear error messages.
| elif [[ -n "${CI}" && -z "${REPO_NAME}" ]]; then | ||
| # Running in CI periodic job (REPO_NAME not set), use latest image | ||
| IMAGE_LOC="${DEFAULT_SANDBOX_PLUGIN_IMAGE}" | ||
| echo "Running in CI periodic job, using latest image: ${IMAGE_LOC}" |
There was a problem hiding this comment.
this should be already handled by the if statement above, right?
There was a problem hiding this comment.
I think not. In the periodic job, CI will be true, UI_REPO_PATH and REPO_NAME will be empty. So:
[[ -n "${CI}${UI_REPO_PATH}" ]]will be true becauseCIis true[[ $(echo ${REPO_NAME} | sed 's/"//g') != "release" ]]will be also true since[[ "" != "release" ]]
There was a problem hiding this comment.
ok, how is it done with the host & member operators? because we don't have this extra check there
There was a problem hiding this comment.
We use DEPLOY_LATEST=true on periodic job: https://github.com/openshift/release/blob/master/ci-operator/config/codeready-toolchain/toolchain-e2e/codeready-toolchain-toolchain-e2e-master.yaml#L49. So, we install the operators through ksctl: https://github.com/codeready-toolchain/toolchain-e2e/blob/master/make/test.mk#L335-L341
There was a problem hiding this comment.
As we discussed, we can use FORCED_TAG to latest on the ci job config and drop this check on the script. Addressed in 7c92dfd
There was a problem hiding this comment.
@MatousJobanek i was thinking, to not add more envs on the ci config to not confuse us in the future, we could use DEPLOY_LATEST
| libX11-xcb && \ | ||
| yum clean all |
There was a problem hiding this comment.
also suggestion for later - can you move this yum install section more up before copying the folder so it uses the cached snapshots? With the current order it always installs the dependency for every run
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/ci/manage-devsandbox-dashboard.sh (2)
79-79: Fix the invalid exit code.The exit code
-1is invalid; shell exit codes must be in the range 0-255.Apply this diff:
- exit -1 + exit 1
177-177: Validate HOST_NS before using it.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands on lines 180, 183, 186, and 216 to fail with unclear errors.Apply this diff:
# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >&2 + exit 1 + fi # Get the Registration Service API URL
🧹 Nitpick comments (2)
test/e2e/sandbox-ui/README.md (1)
20-20: Consider varying the phrasing for improved readability.Lines 20 and 28 both start with "If you want to...", which creates repetition. Consider rephrasing one of them for variety.
For example, line 28 could be reworded as: "To use your local devsandbox-dashboard, please:"
Also applies to: 28-28
make/sandbox-ui.mk (1)
30-32: Consider installing Playwright to a temporary directory.Similar to how
ksctlis installed in this repository, consider installing Playwright to a temporary directory instead of using$(GOPATH)/bin. This approach:
- Avoids polluting the global GOPATH
- Provides better isolation and reproducibility
- Is more consistent with existing patterns in the codebase
Reference: ksctl installation pattern
Based on learnings from past reviews.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/publish-operators-for-e2e-tests.yml(2 hunks).gitignore(1 hunks)build/sandbox-ui/Dockerfile(1 hunks)deploy/sandbox-ui/base/deployment.yaml(1 hunks)deploy/sandbox-ui/base/route.yaml(1 hunks)deploy/sandbox-ui/base/service-accounts.yaml(1 hunks)deploy/sandbox-ui/base/service.yaml(1 hunks)make/sandbox-ui.mk(1 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)test/e2e/sandbox-ui/README.md(3 hunks)test/e2e/sandbox-ui/homepage_test.go(1 hunks)test/e2e/sandbox-ui/setup/setup_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/sandbox-ui/homepage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/publish-operators-for-e2e-tests.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
🧬 Code graph analysis (2)
test/e2e/sandbox-ui/setup/setup_test.go (1)
testsupport/init.go (1)
WaitForDevSandboxDashboard(306-344)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 77-77: Assign this positional parameter to a local variable.
[warning] 126-126: Add an explicit return statement at the end of the function.
[warning] 114-114: Add an explicit return statement at the end of the function.
[warning] 20-20: Add an explicit return statement at the end of the function.
[warning] 27-27: Assign this positional parameter to a local variable.
[warning] 85-85: Add an explicit return statement at the end of the function.
🪛 LanguageTool
test/e2e/sandbox-ui/README.md
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...} SSO_PASSWORD=${SSO_PASSWORD}` If you want to run and test the Developer Sandbox Dash...
(REP_WANT_TO_VB)
[style] ~28-~28: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 53-53: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 58-58: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 79-79: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[warning] 154-154: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 163-163: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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: Build & push operator bundles for e2e tests
🔇 Additional comments (11)
deploy/sandbox-ui/base/service.yaml (1)
5-5: LGTM!The namespace variable update aligns with the Developer Sandbox Dashboard refactoring.
deploy/sandbox-ui/base/service-accounts.yaml (1)
5-5: LGTM!The namespace variable update is consistent with the Dashboard refactoring.
build/sandbox-ui/Dockerfile (1)
79-79: LGTM!The change to an interactive shell CMD provides flexibility for debugging and manual test execution, consistent with the reviewer suggestion in past comments.
deploy/sandbox-ui/base/deployment.yaml (1)
5-5: LGTM!The namespace variable update is consistent with the Dashboard refactoring across all deployment manifests.
test/e2e/sandbox-ui/setup/setup_test.go (1)
10-11: LGTM!The comment update correctly reflects the Developer Sandbox Dashboard context and aligns with the helper function renaming in testsupport.
deploy/sandbox-ui/base/route.yaml (1)
5-5: LGTM!The namespace variable update aligns with the Dashboard refactoring.
.gitignore (1)
342-346: LGTM!The comment updates reflect the Developer Sandbox Dashboard rebranding while maintaining the same ignore patterns.
test/e2e/sandbox-ui/README.md (1)
1-46: LGTM! Documentation is comprehensive and well-structured.The documentation has been successfully updated to reflect the Developer Sandbox Dashboard rebrand. Prerequisites, setup instructions, test commands, and cleanup procedures are all clear and complete.
make/sandbox-ui.mk (3)
13-22: LGTM! Dashboard publishing logic is well-structured.The conditional parameter building and script invocation are clean and maintainable.
70-94: LGTM! Container-based testing flow is well-designed.The logic correctly handles:
- Conditional UI repository mounting
- Publishing images outside the container (PUBLISH_UI=true, DEPLOY_UI=false)
- Deploying inside the container (DEPLOY_UI=true, PUBLISH_UI=false)
- Proper volume mounts for kubeconfig and repositories
56-56: Verify variable consistency for usersignup deletion.This line uses
${SSO_USERNAME}, but line 40 in the same file uses${SSO_USERNAME_READ}. Confirm whether this inconsistency exists and whetherSSO_USERNAMEis properly set in CI environments. If the inconsistency is confirmed, change line 56 to use${SSO_USERNAME_READ}for consistency.
| PUSH_SANDBOX_IMAGE ?= false | ||
| DEVSANDBOX_DASHBOARD_NS := devsandbox-dashboard | ||
| IMAGE_PLATFORM ?= linux/amd64 | ||
| KUBECONFIG ?= $(HOME)/.kube/config |
There was a problem hiding this comment.
just curious - why do we need to set the KUBECONFIG env variable ? This is already the default path already if you haven't specified anything as KUBECONFIG env variable.
There was a problem hiding this comment.
Oh nvm, now I saw it's because you were using it here and about to remove it I guess ..
There was a problem hiding this comment.
To be used on test-devsandbox-dashboard-in-container
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/ci/manage-devsandbox-dashboard.sh (2)
172-181: Add validation for HOST_NS before using it.If no host-operator project exists,
HOST_NSwill be empty, causing the subsequentoc get routecommands at lines 177–180 to fail with unclear errors. Add a check after line 174.# Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >&2 + exit 1 + fi # Get the Registration Service API URLThis validation was flagged in a prior review and should have been applied.
77-81: Fix invalid shell exit code at line 80.Shell exit codes must be in range 0-255.
exit -1is invalid and will be interpreted unpredictably.*) echo "$1 is not a recognized flag!" >> /dev/stderr user_help - exit -1 + exit 1 ;;This was flagged in a prior review (SC2242) and should have been addressed.
🧹 Nitpick comments (6)
test/e2e/sandbox-ui/README.md (1)
18-31: Minor: Reduce repetitive phrasing for better readability.Lines 18 and 28 both start with "If you want to..." Consider rephrasing one to add variety.
Example:
- Line 18: "If you want to run..." → "To test the Developer Sandbox Dashboard from your local..."
- Line 28: "If you want to use..." → "To use your local devsandbox-dashboard..."
scripts/ci/manage-devsandbox-dashboard.sh (5)
86-113: Add explicit return statement to check_sso_credentials function.Per SonarCloud, add an explicit
return 0or similar at the end of the function for clarity.echo "SSO credentials validated successfully" + return 0 }
115-125: Add explicit return statement to configure_oauth_idp function.Per SonarCloud, add an explicit
return 0or similar at the end of the function for clarity.oc patch oauths.config.openshift.io/cluster --type=merge --patch-file=/dev/stdin + return 0 }
127-135: Add explicit return statement to create_namespace function.Per SonarCloud, add an explicit
return 0or similar at the end of the function for clarity.oc project ${DEVSANDBOX_DASHBOARD_NS} >/dev/null 2>&1 + return 0 }
3-18: Add explicit return statement to user_help function.Per SonarCloud, while this function exits, adding an explicit
return 0beforeexit 0improves clarity (though it won't be reached).echo "-h, --help To show this help text" echo "" + return 0 exit 0 }
20-84: Add explicit return statement to read_arguments function.Per SonarCloud, add an explicit
return 0at the end of the function for clarity.esac done + return 0 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)test/e2e/sandbox-ui/README.md(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
📚 Learning: 2025-08-01T09:54:02.406Z
Learnt from: fbm3307
Repo: codeready-toolchain/toolchain-e2e PR: 1175
File: deploy/base1ns-gotemplate/cluster.yaml:69-70
Timestamp: 2025-08-01T09:54:02.406Z
Learning: The codeready-toolchain/toolchain-e2e project is not using the latest version of Kubernetes yet, so deprecated API groups like `ingresses.extensions` may still be functional in their current environment. Consider this when reviewing resource quotas and API usage.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 20-20: Add an explicit return statement at the end of the function.
[warning] 127-127: Add an explicit return statement at the end of the function.
[warning] 78-78: Assign this positional parameter to a local variable.
[warning] 86-86: Add an explicit return statement at the end of the function.
[warning] 115-115: Add an explicit return statement at the end of the function.
🪛 LanguageTool
test/e2e/sandbox-ui/README.md
[style] ~20-~20: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...} SSO_PASSWORD=${SSO_PASSWORD}` If you want to run and test the Developer Sandbox Dash...
(REP_WANT_TO_VB)
[style] ~28-~28: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...E> SSO_PASSWORD=<SSO_PASSWORD>` If you want to use your local devsandbox-dashboard, pl...
(REP_WANT_TO_VB)
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 54-54: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 59-59: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 80-80: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[warning] 155-155: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 164-164: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (3)
test/e2e/sandbox-ui/README.md (1)
1-47: Documentation properly rebranded and well-structured.The rebranding from Sandbox UI to Developer Sandbox Dashboard is consistent throughout. Prerequisites, test execution, and deployment instructions are clear and actionable. The addition of the
sandbox-rhdh-pluginrepository setup and SSO credential guidance aligns with reviewer feedback.scripts/ci/manage-devsandbox-dashboard.sh (2)
147-170: Approve script structure and integration with manage-operator.sh.The script properly sources
manage-operator.shto reuseget_repo,set_tags, andpush_imagefunctions, avoiding duplication. The conditional logic at lines 153–170 correctly determines whether to build/push the dashboard image from a provided repository or use a pre-built default image. Idempotent secret creation (lines 118–121) and namespace handling (lines 127–135) follow best practices.
153-166: Verify QUAY_NAMESPACE is defined before use at line 159.Line 159 references
QUAY_NAMESPACEin the IMAGE_LOC assignment, but QUAY_NAMESPACE is only set via the-qnflag or sourced frommanage-operator.sh. If neither provides it and the condition at line 153 evaluates to true, IMAGE_LOC will contain an empty namespace. Consider adding validation or ensuring QUAY_NAMESPACE has a default.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/ci/manage-devsandbox-dashboard.sh (2)
21-82: Wire-dl/--deploy-latestflag correctly and fix invalid exit code.Right now
-dlis advertised in the help and passed frommake/sandbox-ui.mk, but it is not handled inread_arguments, so it falls into the default case and triggersuser_help+exit -1. That means:
- The script can exit early (often with success code 0 because
user_helpalready exits).- Static analysis is correct that
exit -1is invalid for shells (status must be 0–255).- The Sonar warning about
$1vs a local copy is also valid; you already havelocal arg="$1"but don’t use it.You can address all of this by adding a proper
-dl|--deploy-latestcase that accepts an optional value and by fixing the default branch to useargand a valid non‑zero exit code:@@ - while [[ $# -gt 0 ]]; do - local arg="$1" + while [[ $# -gt 0 ]]; do + local arg="$1" case "$arg" in @@ - -en|--environment) - shift - ENVIRONMENT=$1 - shift - ;; - *) - echo "$1 is not a recognized flag!" >> /dev/stderr - user_help - exit -1 - ;; + -en|--environment) + shift + ENVIRONMENT=$1 + shift + ;; + -dl|--deploy-latest) + # Optional value: `-dl true` or just `-dl` to mean "true". + shift + if [[ $# -gt 0 && "$1" != -* ]]; then + DEPLOY_LATEST=$1 + shift + else + DEPLOY_LATEST=true + fi + ;; + *) + echo "${arg} is not a recognized flag!" >> /dev/stderr + user_help + exit 1 + ;; esac doneThis keeps the existing
DEPLOY_LATESTenvironment‑variable semantics, makes the-dlflag usable, and fixes the invalid exit code while aligning with the Sonar suggestion to use the local copy of the argument.
176-188: Guard against empty HOST_NS before using it in multipleoccommands.If no host‑operator project exists,
HOST_NSwill be empty and all subsequentoc get/oc applycalls using-n ${HOST_NS}will fail with less clear errors. Adding a simple guard improves failure diagnostics.You can insert this check right after assigning
HOST_NS:- # Get the HOST_NS (host operator namespace) - HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) - - # Get the Registration Service API URL + # Get the HOST_NS (host operator namespace) + HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." >&2 + exit 1 + fi + + # Get the Registration Service API URLmake/sandbox-ui.mk (1)
52-57: Use SSO_USERNAME_READ for cleanup to match e2e-run behavior.
clean-devsandbox-dashboardstill uses${SSO_USERNAME}, while the e2e target creates/deletes usersignups via${SSO_USERNAME_READ}(which is sourced from CI secrets orSSO_USERNAMElocally). In CI,SSO_USERNAMEmay not be exported directly, so this can cause cleanup to fail or target the wrong user. UsingSSO_USERNAME_READkeeps both targets consistent..PHONY: clean-devsandbox-dashboard clean-devsandbox-dashboard: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) clean-devsandbox-dashboard: @oc delete ns ${DEVSANDBOX_DASHBOARD_NS} @oc delete secret ${OPENID_SECRET_NAME} -n openshift-config - @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup ${SSO_USERNAME_READ} -n ${HOST_NS}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
make/sandbox-ui.mk(1 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
📚 Learning: 2025-08-01T09:54:02.406Z
Learnt from: fbm3307
Repo: codeready-toolchain/toolchain-e2e PR: 1175
File: deploy/base1ns-gotemplate/cluster.yaml:69-70
Timestamp: 2025-08-01T09:54:02.406Z
Learning: The codeready-toolchain/toolchain-e2e project is not using the latest version of Kubernetes yet, so deprecated API groups like `ingresses.extensions` may still be functional in their current environment. Consider this when reviewing resource quotas and API usage.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 79-79: Assign this positional parameter to a local variable.
[warning] 21-21: Add an explicit return statement at the end of the function.
[warning] 128-128: Add an explicit return statement at the end of the function.
[warning] 116-116: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 87-87: Add an explicit return statement at the end of the function.
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 55-55: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 60-60: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 81-81: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[warning] 159-159: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 168-168: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (7)
scripts/ci/manage-devsandbox-dashboard.sh (4)
87-114: SSO credential validation flow looks solid.The SSO credential handling correctly:
- Reads from CI secrets or local env vars.
- Fails fast with a clear message if either value is missing.
- Validates credentials via a real token request and exits non‑zero on any non‑200 status.
This is a good balance between safety and simplicity for this script.
116-136: OAuth IDP configuration and namespace creation are idempotent and appropriate.
configure_oauth_idpandcreate_namespaceuseoc ... --dry-run=client -o yaml | oc apply -f -andoc new-project ... || true, plusoc project, which makes these operations idempotent and safe to re‑run. No changes needed here.
154-174: Image selection logic for DEV vs CI looks consistent with manage-operator patterns.The
DEPLOY_LATEST/CI/UI_REPO_PATHbranching mirrors the existing operator-flow conventions (use a fixed latest image for periodic jobs, otherwise build/tag from the UI repo or fall back to the default image). The reuse ofmanage-operator.sh’sget_repo/set_tags/push_imagemachinery is sensible and keeps image handling centralized.
191-227: Deployment orchestration (SSO read, kustomize, OAuth IDP, rollout wait) looks good.The deployment block correctly:
- Reads SSO credentials for CI/local, validates them, and fails fast via
check_sso_credentials.- Ensures the target namespace exists before applying manifests.
- Uses
SCRIPT_DIR/REPO_ROOTto build a robust kustomize path.- Wires all required environment variables through
envsubst.- Configures OAuth IDP and optionally patches
toolchainconfigonly for theui-e2e-testsenvironment.- Waits on the RHDH deployment with a bounded timeout.
Overall this flow is coherent and matches expected behavior for the dashboard deployment.
make/sandbox-ui.mk (3)
1-12: Defaults for dashboard namespace, platform, kubeconfig, and OpenID secret look appropriate.The top-level defaults (namespace, image platform, kubeconfig fallback to
~/.kube/config, and OpenID secret) match how the script expects to be called and are reasonable for both CI and local runs.
25-44: E2E run target wiring (Playwright, setup tests, main tests, cleanup) looks good.The
e2e-run-devsandbox-dashboardtarget:
- Calculates the RHDH URL from the cluster ingress domain.
- Installs the Playwright CLI and Firefox only when needed.
- Runs setup tests and main e2e tests with the correct namespace and
.envwiring.- Cleans up the usersignup created for the test run.
The flow and environment passing look consistent with the new dashboard setup.
60-95: Containerized e2e flow is coherent and reuses the new dashboard machinery correctly.The
build-devsandbox-dashboard-e2e-testsandtest-devsandbox-dashboard-in-containertargets:
- Build the e2e test image with
podmanusingIMAGE_PLATFORM.- Optionally publish the dashboard image when
UI_REPO_PATHis set, usingFORCED_TAGand the sameget-and-publish-devsandbox-dashboardentrypoint.- Mount kubeconfig and repo state into the container and delegate to
make test-devsandbox-dashboard-e2einside.This gives a clean, reproducible path for running the dashboard e2e tests in a container. No further changes needed here.
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Looks good, just minor things
| publish-current-bundles-for-e2e: PUBLISH_UI=true | ||
| publish-current-bundles-for-e2e: DEPLOY_UI=false |
There was a problem hiding this comment.
I'm wondering, couldn't we use the same default values as we use for operators? - to keep it consistent
There was a problem hiding this comment.
.PHONY: publish-current-bundles-for-e2e
## Target that is supposed to be called from CI - it builds & publishes the current operator bundles
publish-current-bundles-for-e2e: PUBLISH_UI=true
publish-current-bundles-for-e2e: DEPLOY_UI=false
publish-current-bundles-for-e2e: get-and-publish-operators get-and-publish-devsandbox-dashboard
.PHONY: get-and-publish-operators
get-and-publish-operators: PUBLISH_OPERATOR=true
get-and-publish-operators: INSTALL_OPERATOR=false
get-and-publish-operators: clean-e2e-files get-and-publish-host-operator get-and-publish-member-operator
I set them in publish-current-bundles-for-e2e because the default values are the opposite: https://github.com/rsoaresd/toolchain-e2e/blob/refactor_pairing_with_ui/make/sandbox-ui.mk#L11-L12
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
scripts/ci/manage-devsandbox-dashboard.sh (3)
78-82: Use valid exit code.
exit -1is invalid; shell exit codes must be 0-255. Useexit 1instead.*) echo "$1 is not a recognized flag!" >> /dev/stderr user_help - exit -1 + exit 1 ;;
173-176: Verify HOST_NS is set before using it.If no host-operator project exists,
HOST_NSwill be empty, causing subsequentoc getcommands to fail with unclear errors.if [[ ${DEPLOY_UI} == "true" ]]; then # Get the HOST_NS (host operator namespace) HOST_NS=$(oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) + + if [[ -z "${HOST_NS}" ]]; then + echo "Error: Could not find host-operator namespace. Ensure host-operator is deployed." + exit 1 + fi
140-148: Add default values for namespace and openid-secret flags.The help text on lines 12-13 implies defaults exist, but the script doesn't set them when flags are omitted. This will cause failures when these values are not provided.
read_arguments "$@" + +# Set defaults for optional parameters +DEVSANDBOX_DASHBOARD_NS=${DEVSANDBOX_DASHBOARD_NS:-devsandbox-dashboard} +OPENID_SECRET_NAME=${OPENID_SECRET_NAME:-openid-sandbox-public-client-secret} if [[ -n "${CI}" ]]; thenmake/sandbox-ui.mk (1)
54-59: Inconsistent variable usage for SSO_USERNAME.
e2e-run-devsandbox-dashboarduses$(SSO_USERNAME_READ)(line 43) for the usersignup deletion, butclean-devsandbox-dashboarduses$(SSO_USERNAME)(line 59). This could cause failures in CI whereSSO_USERNAMEis not directly set as an environment variable..PHONY: clean-devsandbox-dashboard clean-devsandbox-dashboard: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) clean-devsandbox-dashboard: @oc delete ns ${DEVSANDBOX_DASHBOARD_NS} @oc delete secret ${OPENID_SECRET_NAME} -n openshift-config - @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup $(SSO_USERNAME_READ) -n $(HOST_NS)
🧹 Nitpick comments (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
101-106: Consider credential exposure in verbose mode.When
set -exis enabled (line 143), the curl command with credentials will be echoed to the log. While this is a common pattern in CI, consider wrapping sensitive commands to prevent credential leakage.status=$(curl -s -o /dev/null -w "%{http_code}" \ -X POST "https://sso.devsandbox.dev/auth/realms/sandbox-dev/protocol/openid-connect/token" \ -d "grant_type=password" \ -d "client_id=sandbox-public" \ - -d "username=${SSO_USERNAME_READ}" \ - -d "password=${SSO_PASSWORD_READ}") + -d "username=${SSO_USERNAME_READ}" \ + -d "password=${SSO_PASSWORD_READ}") 2>/dev/nullAlternatively, temporarily disable tracing around the curl command with
set +xbefore andset -xafter ifCIis set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
make/sandbox-ui.mk(1 hunks)scripts/ci/manage-devsandbox-dashboard.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
📚 Learning: 2025-08-01T09:54:02.406Z
Learnt from: fbm3307
Repo: codeready-toolchain/toolchain-e2e PR: 1175
File: deploy/base1ns-gotemplate/cluster.yaml:69-70
Timestamp: 2025-08-01T09:54:02.406Z
Learning: The codeready-toolchain/toolchain-e2e project is not using the latest version of Kubernetes yet, so deprecated API groups like `ingresses.extensions` may still be functional in their current environment. Consider this when reviewing resource quotas and API usage.
Applied to files:
scripts/ci/manage-devsandbox-dashboard.sh
🧬 Code graph analysis (1)
scripts/ci/manage-devsandbox-dashboard.sh (1)
scripts/ci/manage-operator.sh (3)
get_repo(11-24)set_tags(26-45)push_image(47-55)
🪛 GitHub Check: SonarCloud Code Analysis
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 87-87: Add an explicit return statement at the end of the function.
[warning] 21-21: Add an explicit return statement at the end of the function.
[warning] 3-3: Add an explicit return statement at the end of the function.
[warning] 116-116: Add an explicit return statement at the end of the function.
[warning] 128-128: Add an explicit return statement at the end of the function.
[warning] 79-79: Assign this positional parameter to a local variable.
🪛 Shellcheck (0.11.0)
scripts/ci/manage-devsandbox-dashboard.sh
[warning] 55-55: DATE_SUFFIX appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 60-60: FORCED_TAG appears unused. Verify use (or export if used externally).
(SC2034)
[error] 81-81: Can only exit with status 0-255. Other data should be written to stdout/stderr.
(SC2242)
[warning] 156-156: PROVIDED_REPOSITORY_PATH appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 165-165: REPOSITORY_NAME appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (9)
scripts/ci/manage-devsandbox-dashboard.sh (4)
138-148: LGTM!Proper initialization with
set -e, correctly quoted argument handling, and conditional tracing in CI mode.
116-126: LGTM!Secret creation is now idempotent using
--dry-run=client -o yaml | oc apply -f -.
154-171: LGTM!The conditional logic correctly handles CI/local modes. The shellcheck warnings (SC2034) for
REPOSITORY_NAMEandPROVIDED_REPOSITORY_PATHare false positives—these variables are consumed by the sourcedmanage-operator.shscript viaget_repo,set_tags, andpush_imagefunctions.
197-206: LGTM!Path resolution using
BASH_SOURCEis robust, and the kustomize pipeline withenvsubstproperly substitutes environment variables before applying.make/sandbox-ui.mk (5)
1-11: LGTM!Variable definitions are well-structured with sensible defaults. The
KUBECONFIGdefault to$(HOME)/.kube/configis appropriate for local usage.
13-25: LGTM!The target correctly builds optional parameters only when set, avoiding spurious flag passing. The
DEPLOY_LATEST_PARAMhandling addresses the prior review concern.
62-69: LGTM!The podman build command correctly uses
--platformflag (addressing the prior--IMAGE_PLATFORMtypo).
71-97: LGTM!The containerized test target properly handles optional
UI_REPO_PATHwith conditional volume mounting and environment variable passing. The--platformflag is correctly used.
27-45: LGTM!The e2e test runner is well-structured: installs Playwright dependencies, runs setup and main tests sequentially, and properly cleans up the usersignup resource. The consistent use of
$(SSO_USERNAME_READ)ensures CI compatibility.
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Awesome, thanks a lot. Great job 🚀
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
Since we moved the Developer Sandbox Plugin code outside the RHDH org to ours (https://github.com/codeready-toolchain/devsandbox-dashboard), we need need to refactor the existing pairing logic with UI code in order to:
How to test
make test-devsandbox-dashboard-in-container SSO_USERNAME=<SSO_USERNAME> SSO_PASSWORD=<SSO_PASSWORD> UI_REPO_PATH=${PWD}/../devsandbox-dashboardWhat's the next steps?
In a separate PRs, i will:
sandbox-uitodevsandbox-dashboardgithub/workflows/publish-sandbox-ui-for-ui-e2e-tests.ymlAfter refactoring pairing with UI code, we need to enable the UI E2E Tests on devsandbox-dashboard. We will need to:
Related PR
codeready-toolchain/devsandbox-dashboard#9
Issue ticket number and link
SANDBOX-1511
Assisted-by: Cursor
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.