test: improve how we set PUBLISH_UI#1232
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
WalkthroughPUBLISH_UI default now depends on CLONEREFS_OPTIONS: if CLONEREFS_OPTIONS is non-empty PUBLISH_UI=false, otherwise PUBLISH_UI=true. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
make/devsandbox-dashboard.mk(2 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: Unit Tests
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (1)
make/devsandbox-dashboard.mk (1)
56-57: Simplified local test target relies on conditional defaults.The removal of explicit
PUBLISH_UI=true DEPLOY_UI=trueoverrides is cleaner and aligns with the new conditional logic. However, this target now depends on the conditionalPUBLISH_UIassignment (lines 10-15) behaving correctly.For typical local development (where
CLONEREFS_OPTIONSis empty),PUBLISH_UIwill betrue, matching the previous explicit behavior. However, ifCLONEREFS_OPTIONSis inadvertently set in a local environment,PUBLISH_UIwould befalse, which differs from the previous explicit override.Ensure that the conditional logic in lines 10-15 correctly handles all expected scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
make/devsandbox-dashboard.mk(2 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). (2)
- GitHub Check: Unit Tests
- GitHub Check: GolangCI Lint
🔇 Additional comments (1)
make/devsandbox-dashboard.mk (1)
10-15: Past review concern addressed.The conditional assignment operator (
?=) is now used instead of immediate assignment (:=), which allows environment variable overrides while preserving the conditional default behavior. This matches the behavior ofDEPLOY_UIat line 16 and resolves the inconsistency flagged in the previous review.
| .PHONY: test-devsandbox-dashboard-e2e-local | ||
| test-devsandbox-dashboard-e2e-local: | ||
| $(MAKE) get-and-publish-devsandbox-dashboard e2e-run-devsandbox-dashboard UI_REPO_PATH=${PWD}/../devsandbox-dashboard PUBLISH_UI=true DEPLOY_UI=true | ||
| $(MAKE) get-and-publish-devsandbox-dashboard e2e-run-devsandbox-dashboard UI_REPO_PATH=${PWD}/../devsandbox-dashboard |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the Makefile structure to understand the defaults and the conditional for PUBLISH_UI
echo "=== Examining make/devsandbox-dashboard.mk structure ==="
cat -n make/devsandbox-dashboard.mk | head -120
echo -e "\n=== Checking the test-devsandbox-dashboard-e2e-local target definition ==="
sed -n '50,65p' make/devsandbox-dashboard.mkRepository: codeready-toolchain/toolchain-e2e
Length of output: 6879
🏁 Script executed:
#!/bin/bash
# Examine the management script to see what PUBLISH_UI controls
echo "=== Examining scripts/ci/manage-devsandbox-dashboard.sh for PUBLISH_UI usage ==="
cat -n scripts/ci/manage-devsandbox-dashboard.sh | grep -A5 -B5 'PUBLISH_UI'
echo -e "\n=== Checking for image publishing/push logic ==="
cat -n scripts/ci/manage-devsandbox-dashboard.sh | grep -A3 -B3 'push\|publish.*image\|registry'Repository: codeready-toolchain/toolchain-e2e
Length of output: 2325
🏁 Script executed:
#!/bin/bash
# Check for any git history or related targets that might show intended behavior
echo "=== Looking for other targets that set PUBLISH_UI to understand pattern ==="
rg -n 'PUBLISH_UI' make/devsandbox-dashboard.mk
echo -e "\n=== Checking all targets that invoke manage-devsandbox-dashboard ==="
rg -n '\$\(MAKE\).*devsandbox-dashboard' make/devsandbox-dashboard.mk | head -20Repository: codeready-toolchain/toolchain-e2e
Length of output: 954
Set explicit PUBLISH_UI=false and DEPLOY_UI=false for local testing.
This target now defaults to PUBLISH_UI=true (since CLONEREFS_OPTIONS is empty in a local context) and DEPLOY_UI=true, causing it to push the dashboard image to quay during local testing with a local UI repository. This is inconsistent with the target's purpose—the test-devsandbox-dashboard-in-container target (line 85) explicitly sets PUBLISH_UI=true only for staging purposes, then overrides it to PUBLISH_UI=false (line 101) during the actual test run.
For local e2e testing, add explicit overrides to prevent unintended registry operations:
$(MAKE) get-and-publish-devsandbox-dashboard e2e-run-devsandbox-dashboard UI_REPO_PATH=${PWD}/../devsandbox-dashboard PUBLISH_UI=false DEPLOY_UI=false
🤖 Prompt for AI Agents
In make/devsandbox-dashboard.mk around line 57 the make invocation defaults to
PUBLISH_UI=true and DEPLOY_UI=true in local contexts which causes the dashboard
image to be pushed to quay during local e2e testing; change the invocation to
explicitly set PUBLISH_UI=false and DEPLOY_UI=false for local runs by appending
those variable overrides to the existing command so the target becomes: $(MAKE)
get-and-publish-devsandbox-dashboard e2e-run-devsandbox-dashboard
UI_REPO_PATH=${PWD}/../devsandbox-dashboard PUBLISH_UI=false DEPLOY_UI=false to
prevent unintended registry pushes.
There was a problem hiding this comment.
In the case we are running locally, DEPLOY_UI=true and PUBLISH_UI=true (since we are not running in CI or GH action)
The behaviour is different comparing to test-devsandbox-dashboard-in-container target because inside a container we can not push an image
There was a problem hiding this comment.
@rsoaresd, thank you for the clarification! I understand now—the test-devsandbox-dashboard-e2e-local target is designed to publish and deploy when running directly on a local machine (where image pushing is available), while test-devsandbox-dashboard-in-container explicitly sets PUBLISH_UI=false because it cannot push images from inside the container environment.
The conditional logic correctly defaults to PUBLISH_UI=true for local runs, which is the intended behavior. I appreciate you explaining the distinction between these two testing scenarios!
🐰📚
✏️ Learnings added
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:29.356Z
Learning: In make/devsandbox-dashboard.mk, the test-devsandbox-dashboard-e2e-local target intentionally allows PUBLISH_UI=true and DEPLOY_UI=true to enable publishing and deploying when running locally (outside a container), while test-devsandbox-dashboard-in-container sets PUBLISH_UI=false because image pushing is not available from inside the container.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
1 participant
Description
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.