SANDBOX-1561 | feature: Make targets for debugging services and operators#1242
SANDBOX-1561 | feature: Make targets for debugging services and operators#1242MikelAlejoBR wants to merge 2 commits intocodeready-toolchain:masterfrom
Conversation
WalkthroughAdds a local E2E debug deployment flow: new Make target Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (IDE)
participant Make as Makefile (dev-deploy-e2e-local-debug)
participant CI as CI scripts (manage-*.sh)
participant Builder as Image Builder (podman/docker)
participant OC as OpenShift / kubectl
participant Pod as Operator Pod (with Delve)
Dev->>Make: run `make dev-deploy-e2e-local-debug`
Make->>CI: invoke `manage-*-operator.sh` with `DEBUG_MODE=true`
CI->>CI: push_image(DEBUG_MODE) -> set DEBUG_MODE_SUFFIX
CI->>Builder: call `${IMAGE_BUILDER}-push${DEBUG_MODE_SUFFIX}`
Builder->>CI: image pushed
CI->>OC: apply manifests / update images
Make->>OC: patch registration/CSV to use Delve command/args and set replicas
OC->>Pod: operator pods start with Delve (listen :50000)
Dev->>Pod: port-forward 50000 -> connect debugger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 0
🧹 Nitpick comments (3)
scripts/ci/manage-operator.sh (1)
47-62: Good implementation of debug mode support.The conditional logic correctly enables debug-mode image builds by appending
-debugto the make target whenDEBUG_MODEis"true". This allows the downstream make targets to build Delve-enabled images.Optional: standardize indentation
The indentation within the if-else block is inconsistent. Consider aligning for better readability:
push_image() { # When the "${DEBUG_MODE}" argument is passed, we instruct Make to push # the "debug" images with Delve on them. if [[ $1 == "true" ]]; then - DEBUG_MODE_SUFFIX="-debug" - else - DEBUG_MODE_SUFFIX="" + DEBUG_MODE_SUFFIX="-debug" + else + DEBUG_MODE_SUFFIX="" fimake/dev.mk (2)
54-55: Consider renaming CVS to CSV for clarity.The variable names use "CVS" but the resource type is "ClusterServiceVersion," commonly abbreviated as "CSV." Using "CSV" would be clearer and avoid confusion with the legacy version control system.
Suggested naming improvement
# Get the CVSs for the host and member operators, in order to be able to # patch them. - HOST_CVS_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) - MEMBER_CVS_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion) + HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) + MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion)Then update references on lines 61, 69, and 70 accordingly.
69-70: Consider improving readability of long patch commands.The JSON patch commands are very long (200+ characters), making them difficult to review and maintain. While the logic appears correct, consider breaking them into variables or using here-documents for better readability.
Example: Extract patch JSON to variables
+ # Define patch for host operator + HOST_PATCH='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + + # Define patch for member operator + MEMBER_PATCH='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/member-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + # Patch the host-operator and member-operator CSVs to make them run with # Delve. - oc patch --namespace "${DEFAULT_HOST_NS}" "${HOST_CVS_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' - oc patch --namespace "${DEFAULT_MEMBER_NS}" "${MEMBER_CVS_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/member-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + oc patch --namespace "${DEFAULT_HOST_NS}" "${HOST_CVS_NAME}" --type='json' --patch="$${HOST_PATCH}" + oc patch --namespace "${DEFAULT_MEMBER_NS}" "${MEMBER_CVS_NAME}" --type='json' --patch="$${MEMBER_PATCH}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.adocmake/dev.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
Applied to files:
make/dev.mkREADME.adoc
🧬 Code graph analysis (2)
scripts/ci/manage-host-operator.sh (1)
scripts/ci/manage-operator.sh (1)
push_image(47-62)
scripts/ci/manage-member-operator.sh (1)
scripts/ci/manage-operator.sh (1)
push_image(47-62)
⏰ 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: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Unit Tests
🔇 Additional comments (5)
scripts/ci/manage-host-operator.sh (1)
99-99: LGTM! DEBUG_MODE propagation is correct.The
push_imagecalls now correctly propagate theDEBUG_MODEenvironment variable to enable debug-enabled image builds when set. The variable is expected to be exported by the caller (e.g.,make/dev.mk), and if unset, the function will default to the standard non-debug build path.Also applies to: 111-111
scripts/ci/manage-member-operator.sh (1)
98-98: LGTM! Consistent with host operator changes.The
push_imageinvocation correctly propagatesDEBUG_MODEto enable debug builds, matching the pattern established inmanage-host-operator.sh.README.adoc (1)
154-161: Excellent documentation for the debug target.The documentation clearly explains the new debugging workflow, including:
- Build characteristics (no optimizations, Delve included)
- Debugger port (50000)
- Port-forwarding instructions
- List of debuggable services
This will help developers quickly understand and use the debugging feature.
make/dev.mk (2)
42-50: Good use of export for DEBUG_MODE propagation.The target correctly exports
DEBUG_MODE=trueto enable debug builds in downstream make targets and shell scripts. The.ONESHELL:directive appropriately enables multi-line shell commands for the complex patching logic that follows.
61-61: Verify: Silent failure handling may hide errors.The
|| truesilently ignores failures when patching the registration service command. If the CSV doesn't exist or the patch path is invalid, this will fail silently and the registration service won't be configured for debugging.Consider whether this should fail loudly or if the silent handling is intentional (e.g., for cases where the CSV structure differs).
Would it be better to check if the patch succeeded and provide a warning, or is silent failure acceptable here?
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
bf2dec3 to
b390515
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
make/dev.mk (1)
54-55: Verify CSV retrieval when multiple CSVs exist.The command
oc get --output name ClusterServiceVersionreturns all CSVs in the namespace. If there are multiple CSVs (e.g., from previous deployments or other operators), the variable will contain multiple names, potentially causing the subsequentoc patchcommands to fail or behave unexpectedly.Consider filtering for the specific operator CSV:
Suggested improvement
- HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion) - MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion) + HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion | grep toolchain-host-operator) + MEMBER_CSV_NAME=$$(oc get --namespace "${DEFAULT_MEMBER_NS}" --output name ClusterServiceVersion | grep toolchain-member-operator)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.adocmake/dev.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/manage-host-operator.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
Applied to files:
make/dev.mkREADME.adoc
⏰ 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 & dashboard image for e2e tests
🔇 Additional comments (4)
scripts/ci/manage-member-operator.sh (1)
98-98: LGTM!The change correctly passes
DEBUG_MODEtopush_image, aligning with the pattern inmanage-host-operator.sh. WhenDEBUG_MODEis unset or empty,push_imagewill default to the non-debug push target, which is the expected behavior.README.adoc (1)
154-160: LGTM!The documentation clearly explains the new
dev-deploy-e2e-local-debugtarget, including its purpose, how to connect (port-forwarding on 50000), and which services are debuggable. This aligns with the implementation inmake/dev.mk.make/dev.mk (2)
69-70: Hardcoded container indices may be fragile.The patches use fixed indices (
containers/1for host-operator,containers/0for member-operator). If the container order in the CSV deployment spec changes, the wrong container will be patched.This is acceptable for a dev-only debugging target, but be aware that changes to the operator CSV structures could silently break this functionality.
42-50: Good use of.ONESHELLand exported variable.The
.ONESHELLdirective correctly ensures shell variables persist across recipe lines, andexport DEBUG_MODE=trueproperly propagates the flag to downstream Make invocations viadev-deploy-e2e-local.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
b390515 to
e4931d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @make/dev.mk:
- Line 63: Remove the silent-failure suppression on the registration-service
patch by deleting the trailing "|| true" from the oc patch that adds
REGISTRATION_SERVICE_COMMAND to the HOST_CSV_NAME container so failures surface;
make error handling consistent across the other oc patch invocations (the
ToolchainConfig and operator CSV patches) by ensuring they also do not use "||
true" (or explicitly handle the specific "already exists" condition), and if
needed implement an explicit check for the env entry before patching instead of
swallowing all errors.
- Line 50: Uncomment the deployment invocation so the dev target actually
deploys e2e resources: remove the leading '#' from the "$(MAKE)
dev-deploy-e2e-local" line in make/dev.mk so the target runs the deployment step
(ensuring subsequent "oc get ClusterServiceVersion" and patch commands have
resources to operate on and match the README.adoc behavior).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adocmake/dev.mkmake/test.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/ci/manage-host-operator.sh
- scripts/ci/manage-member-operator.sh
- scripts/ci/manage-operator.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
Applied to files:
make/test.mkmake/dev.mkREADME.adoc
⏰ 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: Unit Tests
🔇 Additional comments (3)
make/test.mk (2)
334-338: LGTM! Consistent parameter handling.The DEBUG_MODE_PARAM evaluation follows the established pattern used for other conditional parameters in this file (FORCED_TAG_PARAM, MEMBER_NS_2_PARAM, etc.). The double-negation checks ensure the parameter is only set when DEBUG_MODE has a non-empty value.
348-348: LGTM! Debug mode correctly integrated into non-latest deployment paths.The DEBUG_MODE_PARAM is appropriately passed only in the non-DEPLOY_LATEST branches, which makes sense since debug mode requires building custom images with Delve rather than using pre-published latest images.
Also applies to: 378-378
README.adoc (1)
154-161: Documentation is clear and informative.The documentation effectively describes the new debugging target, including the port-forwarding workflow and which services are built with Delve support. The explanation will help developers set up their debugging environment.
| # then an IDE can be connected to them. Since the targets down the line use | ||
| # the default namespaces, we can use them to patch the required CRs in order | ||
| # to launch the binaries with Delve. | ||
| .ONESHELL: |
There was a problem hiding this comment.
.ONESHELL is a global directive that affects all subsequent targets.
The .ONESHELL directive on Line 46 applies to all targets defined after it in this file, not just dev-deploy-e2e-local-debug. This can have unintended consequences:
- All subsequent targets will run their recipes in a single shell, which changes error propagation behavior.
- Other targets may not expect this behavior and could mask failures.
If .ONESHELL is needed only for variable persistence in the debug target (for HOST_CSV_NAME and MEMBER_CSV_NAME), consider moving it to a separate included file or restructuring the target to avoid the need for .ONESHELL.
♻️ Alternative approach
Instead of .ONESHELL, you could capture variables and pass them to a sub-shell:
-.ONESHELL:
.PHONY: dev-deploy-e2e-local-debug
dev-deploy-e2e-local-debug: export DEBUG_MODE=true
dev-deploy-e2e-local-debug:
- # $(MAKE) dev-deploy-e2e-local
-
- # Get the CSVs for the host and member operators, in order to be able to
- # patch them.
- HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion)
- MEMBER_CSV_NAME=$$(oc get --namespace ${DEFAULT_MEMBER_NS} --output name ClusterServiceVersion)
-
- @echo "CVSs are: $${HOST_CSV_NAME} and $${MEMBER_CSV_NAME}"
- ...
+ $(MAKE) dev-deploy-e2e-local
+ @bash -c ' \
+ set -e; \
+ HOST_CSV_NAME=$$(oc get --namespace "${DEFAULT_HOST_NS}" --output name ClusterServiceVersion); \
+ MEMBER_CSV_NAME=$$(oc get --namespace "${DEFAULT_MEMBER_NS}" --output name ClusterServiceVersion); \
+ echo "CSVs are: $$HOST_CSV_NAME and $$MEMBER_CSV_NAME"; \
+ ...'This limits the .ONESHELL-like behavior to only this target.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@MikelAlejoBR this is needed to propagate the export DEBUG_MODE=true to all makefile targets that are transitively executed, correct?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Sorry @MatousJobanek, with all the noise I missed your comments. Yes, you're right. That export helps leaving many things untouched, but I might have to give it a look again to see if I can avoid that somehow...
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
e4931d6 to
5cbf02f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
make/dev.mk (2)
46-46:.ONESHELLaffects all subsequent targets in this file.The
.ONESHELLdirective on line 46 applies globally to all targets defined after it in this file, not justdev-deploy-e2e-local-debug. This changes how Make executes recipes for all subsequent targets (they run in a single shell instead of one shell per line), which can alter error propagation and variable behavior.While
.ONESHELLis needed here for variable persistence (HOST_CSV_NAMEandMEMBER_CSV_NAME), consider either:
- Moving this target to a separate included file with its own
.ONESHELLscope- Documenting this behavior clearly for future maintainers
- Restructuring to avoid
.ONESHELL(e.g., using a bash script with-c)Based on learnings from metlos, Make recipe lines starting with
#are printed before execution, but this directive affects more fundamental execution behavior.
69-72: Consider extracting JSON patches to files for readability.The inline JSON patch strings are quite long and difficult to review. While functionally correct, consider extracting them to separate JSON files and using
--patch-filefor improved maintainability and readability.Example refactor approach
Create
patches/host-operator-delve-patch.jsonandpatches/member-operator-delve-patch.json, then reference them:- oc patch --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' + oc patch --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch-file=patches/host-operator-delve-patch.json
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adocmake/dev.mkmake/test.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- README.adoc
- scripts/ci/manage-member-operator.sh
- scripts/ci/manage-host-operator.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
Applied to files:
make/dev.mkscripts/ci/manage-operator.shmake/test.mk
📚 Learning: 2025-08-28T12:28:22.801Z
Learnt from: metlos
Repo: codeready-toolchain/toolchain-e2e PR: 1187
File: make/clean.mk:6-11
Timestamp: 2025-08-28T12:28:22.801Z
Learning: In make, recipe lines that start with `#` are printed to stdout before being passed to the shell, making them visible as output even though they're shell comments. To suppress this output, the line would need to be prefixed with `@#`.
Applied to files:
make/dev.mk
⏰ 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: GolangCI Lint
- GitHub Check: Unit Tests
🔇 Additional comments (8)
scripts/ci/manage-operator.sh (3)
48-54: LGTM! Clean DEBUG_MODE handling.The conditional logic correctly sets the DEBUG_MODE_SUFFIX based on the first argument, with clear explanatory comments.
56-57: LGTM! Sensible default.Setting a default IMAGE_BUILDER ensures the variable is always defined.
47-65: This feature has already been merged and deployed as of December 23, 2025. The commit "feature: Make targets for debugging services and operators" (5cbf02f) includes the modifiedpush_image()function and is documented in README.adoc as a working feature (make dev-deploy-e2e-local-debug). No blocking work or external dependencies remain.make/dev.mk (3)
50-50: LGTM! Deployment invocation is now active.The deployment step is correctly uncommented, fixing the critical issue flagged in previous reviews where this line was commented out and would cause subsequent
oc getcommands to fail.
52-55: LGTM! CSV retrieval logic is correct.The commands correctly fetch the CSV names for both operators using
oc get --output name, storing them in shell variables that persist across subsequent recipe lines due to.ONESHELL.
57-67: LGTM! Improved error handling with conditional check.The registration service patch now uses a conditional check (lines 61-63) to verify the environment variable doesn't already exist before patching, which is much better than the
|| truepattern flagged in previous reviews. This provides explicit error handling while avoiding duplicate patches.The ToolchainConfig patch (line 67) correctly fails loudly if it encounters errors.
make/test.mk (2)
368-378: LGTM! Consistent parameter pattern.The DEBUG_MODE_PARAM handling mirrors the existing FORCED_TAG_PARAM pattern and correctly passes the flag only in non-DEPLOY_LATEST flows. The
-dmflag is supported in manage-host-operator.sh.
334-348: LGTM! Consistent parameter pattern.The DEBUG_MODE_PARAM handling mirrors the existing FORCED_TAG_PARAM pattern and correctly passes the flag only in non-DEPLOY_LATEST flows.
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Looks good 👍
I have only few small comments.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
2 times, most recently
from
fb07219 to
3fad5df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@make/dev.mk`:
- Around line 83-85: The oc wait is targeting the wrong CSV and namespace after
patching the member operator: update the oc wait invocation (the oc wait
command) to use the MEMBER_CSV_NAME and the DEFAULT_MEMBER_NS (instead of
HOST_CSV_NAME and DEFAULT_HOST_NS) so you actually wait for the patched member
operator CSV to reach Succeeded.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.adocmake/dev.mkmake/test.mkscripts/ci/manage-host-operator.shscripts/ci/manage-member-operator.shscripts/ci/manage-operator.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/ci/manage-member-operator.sh
- scripts/ci/manage-operator.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
📚 Learning: 2025-12-11T16:29:34.403Z
Learnt from: rsoaresd
Repo: codeready-toolchain/toolchain-e2e PR: 1232
File: make/devsandbox-dashboard.mk:57-57
Timestamp: 2025-12-11T16:29:34.403Z
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.
Applied to files:
make/test.mkmake/dev.mkREADME.adoc
📚 Learning: 2025-08-28T12:28:22.801Z
Learnt from: metlos
Repo: codeready-toolchain/toolchain-e2e PR: 1187
File: make/clean.mk:6-11
Timestamp: 2025-08-28T12:28:22.801Z
Learning: In make, recipe lines that start with `#` are printed to stdout before being passed to the shell, making them visible as output even though they're shell comments. To suppress this output, the line would need to be prefixed with `@#`.
Applied to files:
make/dev.mk
🧬 Code graph analysis (1)
scripts/ci/manage-host-operator.sh (1)
scripts/ci/manage-operator.sh (1)
push_image(47-65)
🔇 Additional comments (10)
scripts/ci/manage-host-operator.sh (3)
14-14: LGTM - Help text accurately describes the debug mode flag.The help text clearly explains the purpose of the
-dm|--debug-modeflag.
72-76: LGTM - Debug mode argument parsing follows established pattern.The argument parsing is consistent with other options in this script (e.g.,
-ft|--forced-tag).
105-105: LGTM - DEBUG_MODE correctly passed to push_image.The
push_imagefunction is invoked with"${DEBUG_MODE}"which correctly handles the case when DEBUG_MODE is unset (passing an empty string). This aligns with thepush_imageimplementation inmanage-operator.shthat checksif [[ $1 == "true" ]].Also applies to: 117-117
make/dev.mk (3)
42-50: LGTM - Debug target setup is correct.The
.ONESHELLdirective is necessary here to allow shell variables likeHOST_CSV_NAMEandMEMBER_CSV_NAMEto persist across recipe lines. Theexport DEBUG_MODE=truecorrectly propagates the variable to sub-make invocations.
61-69: LGTM - Conditional patching prevents duplicate env entries.The
grep -q "REGISTRATION_SERVICE_COMMAND"check correctly prevents adding the environment variable if it already exists, which addresses the earlier review feedback about error handling.
71-75: LGTM - Replica reduction and wait logic is correct.Reducing replicas to 1 facilitates port-forwarding for debugging. The wait ensures the deployment is actually scaled before proceeding.
make/test.mk (3)
334-338: LGTM - DEBUG_MODE_PARAM definition follows established pattern.The parameter handling mirrors the existing
FORCED_TAG_PARAMpattern, maintaining consistency.
348-348: LGTM - DEBUG_MODE_PARAM correctly appended to member operator script invocation.The parameter is positioned consistently with other optional parameters.
368-372: LGTM - Host operator DEBUG_MODE handling is consistent with member operator.Both operator flows handle DEBUG_MODE_PARAM identically, ensuring consistent behavior across the codebase.
Also applies to: 378-378
README.adoc (1)
154-160: LGTM - Clear documentation for the new debug target.The documentation accurately describes the debug workflow, specifies port 50000 for Delve, and lists all affected services. The
oc port-forwardexample is helpful for users.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
54135df to
1a6c676
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@make/dev.mk`:
- Around line 61-62: The shell variable HOST_CSV_NAME is referenced incorrectly
using ${HOST_CSV_NAME} in the oc get command (Make treats single $ as a Make
variable), and the --namespace flag is missing; change the reference to use the
shell-expanded form $${HOST_CSV_NAME} (double dollar) and add the correct
--namespace="${DEFAULT_HOST_NS}" to the oc get invocation so it mirrors the oc
patch call that already uses $${HOST_CSV_NAME} and
--namespace="${DEFAULT_HOST_NS}".
In `@README.adoc`:
- Around line 154-162: Typo: in the README.adoc description for the make target
"make dev-deploy-e2e-local-debug" replace the misspelled word "architechture"
with the correct spelling "architecture" wherever it appears in that paragraph
(keep the surrounding sentence and capitalization unchanged).
1a6c676 to
adcaf33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/ci/manage-host-operator.sh`:
- Around line 14-15: The help text for --debug-mode is misleading: update the
script so the option is a true flag or clearly documents a required value.
Either change the option parsing logic (the case handling for --debug-mode /
DEBUG_MODE) to set DEBUG_MODE=true when the flag is present (and stop consuming
the next argv), or update the help lines (the two echo lines for "-dm,
--debug-mode" and the similar block at 72-75) to state that a value (e.g.,
true/false) is required. Make sure to adjust the option parsing code that reads
--debug-mode()/DEBUG_MODE so it no longer swallows the next argument if you
choose the flag approach.
e6cc36c to
73df021
Compare
Adds a new Make target which triggers build in the underlying services and operators which end up building the images without any code optimizations and with Delve on them. Once pushed and deployed to the local OpenShift cluster, it patches the host operator, the member operator and the registration service to launch them with the Delve executable, which allows debugging them on port 50000 after port-forwarding to those pods. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
73df021 to
3317f02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.adoc`:
- Around line 154-158: The sentence about default architecture uses a plural
verb with the singular noun "architecture"; update the README.adoc text in the
`make dev-deploy-e2e-local-debug` paragraph to use a singular verb (e.g., change
"the default architecture for which the binaries are built are `linux-amd64`" to
"the default architecture for which the binaries are built is `linux-amd64`")
and ensure references to `TARGET_ARCH`, `linux-amd64`, and `oc port-forward`
remain unchanged.
|
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Great job 🤩
Thanks for adding this debugging option, really useful 🚀
|
|
||
| # Patch the host-operator and member-operator CSVs to make them run with | ||
| # Delve. | ||
| oc patch --namespace="${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch='[{"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/args", "value": []}, {"op": "replace", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/host-operator", "--", "--health-probe-bind-address=:8081", "--metrics-bind-address=127.0.0.1:8080", "--leader-elect"]}]' |
There was a problem hiding this comment.
very minor - not a big fan of those "long" patch commands in the make targets. An alternative could be to have those into dedicated patch.yaml files, but it's very minor and ok for now 👍
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfrancisc, MikelAlejoBR 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 |
|
@MikelAlejoBR I see it's trying to pair this PR with the same branch in registration-service, but I see the existing PR: codeready-toolchain/registration-service#568 in there uses a different name now. I guess you haven't deleted the old branch in your registration service fork . also which repo is/should this PR paired with ? |
Yes, I had it pending to pair it with just one of the PRs either from the member or host operator, but I wanted to first address all the feedback before doing it. I think I am going to just close this one and reopen it properly paired with a PR from those repos, and add the remaining feedback to it —like your comment to have a |
|
Closing in favor of #1251. |
4 participants
Adds a new Make target which triggers build in the underlying services and operators which end up building the images without any code optimizations and with Delve on them.
Once pushed and deployed to the local OpenShift cluster, it patches the host operator, the member operator and the registration service to launch them with the Delve executable, which allows debugging them on port 50000 after port-forwarding to those pods.
Related PRs
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
New Features
Documentation
Chores