SANDBOX-1561 | feature: Make targets for debugging services and operators #1242
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MikelAlejoBR The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds a local E2E debug deployment flow: new Make target exports DEBUG_MODE=true, propagates debug flag through test and CI scripts, builds/pushes debug-tagged images, patches registration service and operator CSVs to run Delve (debugger on port 50000), and reduces replicas for IDE port-forwarding. 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 (oc/kubectl)
participant Pod as Operator Pod (with Delve)
Dev->>Make: run `dev-deploy-e2e-local-debug`
Make->>CI: invoke `manage-*-operator.sh` with DEBUG_MODE
CI->>CI: push_image(DEBUG_MODE) → determine 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 reduce 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 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
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.
|
|
||
| if [[ ${PUBLISH_OPERATOR} == "true" ]]; then | ||
| push_image | ||
| push_image "${DEBUG_MODE}" |
There was a problem hiding this comment.
Wonder whether this should be added as an option for consistency with other options or it's fine to treat it as a special environment variable.
@MatousJobanek and @rsoaresd may have thoughts on this
There was a problem hiding this comment.
You are right, yes. I do agree that it's better to have an additional option for consistency. Refactored!
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.
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.
| # to launch the binaries with Delve. | ||
| .ONESHELL: | ||
| .PHONY: dev-deploy-e2e-local-debug | ||
| dev-deploy-e2e-local-debug: export DEBUG_MODE=true |
There was a problem hiding this comment.
why is the export needed ?
There was a problem hiding this comment.
It is needed so that the targets in test.mk can append the --debug-mode script parameter to the manage-(host-operator|member-operator|operator).sh scripts. That way the debug builds are triggered for the different services.
There was a problem hiding this comment.
right, but isn't the variable propagated even without the export keyword ?
So basically just writing: dev-deploy-e2e-local-debug: DEBUG_MODE=true ?
Maybe not, I'm not a big Makefile expert.
There was a problem hiding this comment.
Let me try it, because I'm not sure either...
There was a problem hiding this comment.
When the export keyword is missing the DEBUG_MODE variable ends up affecting the recipe, but the other make targets do not receive the variable... So in order for the variable to be present in all of them we need the export in there.
There was a problem hiding this comment.
gotcha! thanks for looking into it.
| if ! oc get "${HOST_CSV_NAME}" --output jsonpath="{.spec.install.spec.deployments[0].spec.template.spec.containers[1].env}" | grep -q "REGISTRATION_SERVICE_COMMAND"; then \ | ||
| oc patch --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --type='json' --patch='[{"op": "add", "path": "/spec/install/spec/deployments/0/spec/template/spec/containers/1/env/-", "value": {"name": "REGISTRATION_SERVICE_COMMAND", "value": "[\"dlv\", \"--listen=:50000\", \"--headless\", \"--continue\", \"--api-version=2\", \"--accept-multiclient\", \"exec\", \"/usr/local/bin/registration-service\"]"}}]' | ||
| fi | ||
|
|
||
| # Reduce the registration service's replicas to 1, in order to facilitate | ||
| # the port forwarding and debugging of the service. | ||
| oc patch --namespace "${DEFAULT_HOST_NS}" ToolchainConfig config --type='merge' --patch='{"spec":{"host":{"registrationService":{"replicas":1}}}}' | ||
|
|
||
| # 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"]}]' | ||
| oc patch --namespace "${DEFAULT_MEMBER_NS}" "$${MEMBER_CSV_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"]}]' | ||
|
|
There was a problem hiding this comment.
after patching the CSVs should we wait for the new pods/deployments to be up and running, before proceeding/exiting ?
There was a problem hiding this comment.
I have tried adding oc wait for the different resources but I am struggling to add it to when I patch the operators in lines 71-72. After patching the CSVs, the deployment is automatically patched and the replica sets are different, so I am trying to figure out if there's anything else I can monitor for the wait...
There was a problem hiding this comment.
maybe you could try with something like:
echo "Waiting for host-operator pod to be ready with debug configuration..."
oc wait --for=condition=Ready pod -l name=host-operator -n ${DEFAULT_HOST_NS} --timeout=300s
echo "Waiting for member-operator pod to be ready with debug configuration..."
oc wait --for=condition=Ready pod -l name=member-operator -n ${DEFAULT_MEMBER_NS} --timeout=300s
or
# Wait for deployments to roll out after CSV patches
oc rollout status deployment/host-operator -n ${DEFAULT_HOST_NS} --timeout=300s
oc rollout status deployment/member-operator -n ${DEFAULT_MEMBER_NS} --timeout=300s
There was a problem hiding this comment.
Yes, I have tried both approaches but:
- When
waiting for the pods, the command instantly returns because it takes a little bit for the controllers to propagate the changes down the line, so I'd probably have to introduce asleepcommand which seems a bit ugly to leave there... - Regarding the
deployments, since they get updated by the operator that owns them, therollout statusalso says that the conditions have been met and returns instantly.
I am scratching my head with this one because it seems like I need to introduce some ugly sleeps here but...
There was a problem hiding this comment.
When waiting for the pods, the command instantly returns because it takes a little bit for the controllers to propagate the changes down the line, so I'd probably have to introduce a sleep command which seems a bit ugly to leave there...
Regarding the deployments, since they get updated by the operator that owns them, the rollout status also says that the conditions have been met and returns instantly.
You mean that those exit because the update didn't yet started ? Interesting ...
There was a problem hiding this comment.
how about checking the CSV phase, something like:
oc wait --for=jsonpath='{.status.phase}'=Succeeded --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --timeout=300s
There was a problem hiding this comment.
I have tried that one and it seems to work better. I am still testing it back and forth because sometimes it is still so fast that it doesn't see the changes yet 🙃
There was a problem hiding this comment.
However I have seen that in the vast majority of the test it works fine
There was a problem hiding this comment.
I think it's fine for now, we don't need to complicate it too much if not needed 👍
There was a problem hiding this comment.
Perfect, I'll close this PR then and open another one with all these changes to pair it with one of the operator's PRs. Thanks for the reviews!
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources
branch
from
b1763ac to
fb07219
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
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.
| oc patch --namespace="${DEFAULT_MEMBER_NS}" "$${MEMBER_CSV_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"]}]' | ||
| @echo "Waiting for the member operator to be ready..." | ||
| oc wait --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --for=jsonpath='{.status.phase}'=Succeeded --timeout=300s |
There was a problem hiding this comment.
Bug: Wait command uses wrong CSV name for member operator.
After patching the member operator CSV (MEMBER_CSV_NAME), line 85 waits on HOST_CSV_NAME instead of MEMBER_CSV_NAME. This means the wait won't actually verify the member operator is ready.
🐛 Proposed fix
oc patch --namespace="${DEFAULT_MEMBER_NS}" "$${MEMBER_CSV_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"]}]'
`@echo` "Waiting for the member operator to be ready..."
- oc wait --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --for=jsonpath='{.status.phase}'=Succeeded --timeout=300s
+ oc wait --namespace "${DEFAULT_MEMBER_NS}" "$${MEMBER_CSV_NAME}" --for=jsonpath='{.status.phase}'=Succeeded --timeout=300s🤖 Prompt for AI Agents
In `@make/dev.mk` around lines 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.
3 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
✏️ Tip: You can customize this high-level summary in your review settings.