SANDBOX-1561 | feature: enable debug builds for the service#568
SANDBOX-1561 | feature: enable debug builds for the service#568MikelAlejoBR wants to merge 1 commit intocodeready-toolchain:masterfrom
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 debug build flow: disables Go optimizations for dev builds, compiles Delve into debug images via new Docker build stages, adds podman targets for debug images, and extends deploy-dev.sh to optionally refresh deployments to run under Delve. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as scripts/deploy-dev.sh
participant Make as Make / make targets
participant Podman as Podman
participant Registry as Image Registry
participant K8s as Kubernetes
User->>Script: run "refresh" or "refresh debug"
Script->>Script: set DEBUG flag
Script->>Make: invoke build (build-dev when DEBUG)
Make->>Podman: podman-image(-debug)
Podman->>Registry: podman-push(-debug)
Script->>K8s: patch ToolchainConfig / CSV to set image
alt DEBUG=true
Script->>K8s: patch deployment command/env to run Delve
end
Script->>K8s: wait for deployment rollout
K8s->>User: deployment rolled out (image/command applied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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: 2
🤖 Fix all issues with AI agents
In @make/podman.mk:
- Around line 13-16: Add a .PHONY declaration for the podman-image-debug
Makefile target to prevent conflicts with any filesystem object named
"podman-image-debug"; mirror the existing pattern used for podman-push-debug by
adding podman-image-debug to the .PHONY list so the target always runs
regardless of a same-named file.
In @scripts/deploy-dev.sh:
- Around line 77-81: The oc patch JSON string for the deployment command is
missing the closing ] for the outer array, causing a JSON syntax error; fix the
patch in the DEBUG block by adding the missing closing bracket before the final
single quote in the oc patch call so the --patch value is a valid JSON array
(ensure the string passed to oc patch in the if block for DEBUG ends with
..."}]') and verify shell quoting remains correct for the command that calls dlv
and registration-service.
🧹 Nitpick comments (2)
build/Dockerfile.debug (2)
4-7: Consider validatingGOLANG_VERSIONargument.If
GOLANG_VERSIONis not provided or is empty, the build will fail with a confusing error when attempting to download Go. Consider adding a validation step.Proposed validation
# The Golang version must be provided as a build arguments, which will ensure # that Delve gets built with the same version the service's binary gets built # with, to avoid any discrepancies. ARG GOLANG_VERSION + +# Validate that GOLANG_VERSION is provided. +RUN if [ -z "${GOLANG_VERSION}" ]; then echo "ERROR: GOLANG_VERSION build argument is required" && exit 1; fi
42-43: Minor documentation inconsistency.The comment refers to "operator" but this is the "registration-service". Consider updating for clarity.
Suggested fix
-# Copy Delve to the image so that we can execute the operator with it. +# Copy Delve to the image so that we can execute the registration-service with it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/Dockerfile.debugmake/go.mkmake/podman.mkscripts/deploy-dev.sh
🔇 Additional comments (3)
make/go.mk (1)
14-23: LGTM!The
-gcflags "all=-N -l"flags correctly disable optimizations (-N) and inlining (-l) for Delve compatibility. The updated comment block accurately describes the purpose of the development build.build/Dockerfile.debug (1)
45-49: LGTM on Delve configuration.The Delve flags are well-chosen for remote debugging:
--listen=:50000exposes the debug port--headlessenables IDE attachment--continuestarts execution without waiting for debugger--accept-multiclientallows reconnection after disconnectThe exposed ports (50000 for Delve, 8080-8082 for service) are appropriate.
scripts/deploy-dev.sh (1)
34-50: LGTM on DEBUG flag logic.The conditional build logic cleanly separates debug and non-debug paths. Using string comparison
[[ "${DEBUG}" = true ]]is appropriate for this use case.
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-rs
branch
2 times, most recently
from
963c052 to
b3b517f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/deploy-dev.sh`:
- Around line 81-84: The unconditional oc wait that checks the
deployment/registration-service command equals "dlv" will fail when DEBUG is not
set; move the oc wait
--for=jsonpath='{.spec.template.spec.containers[0].command[0]}'="dlv"
"deployment/registration-service" (and the subsequent oc rollout status for
registration-service if it only relates to the debug patch) inside the DEBUG
conditional block that patches the container command (the block that checks the
DEBUG variable and applies the Delve patch), or remove the jsonpath wait
entirely and only keep rollout status when you do not patch the command,
ensuring the wait is only executed when DEBUG=true and the container command is
actually changed to "dlv".
🧹 Nitpick comments (2)
build/Dockerfile (1)
28-57: Consider pinning Delve to a specific version for reproducibility.Using
@latestfor Delve installation (line 57) means builds may produce different results over time as Delve releases new versions. For development/debugging tooling this is often acceptable, but pinning to a specific version would improve reproducibility.Example with pinned version
-RUN GOBIN=/tmp/bin go install github.com/go-delve/delve/cmd/dlv@latest +RUN GOBIN=/tmp/bin go install github.com/go-delve/delve/cmd/dlv@v1.24.0scripts/deploy-dev.sh (1)
86-99: Redundant wait logic and flow clarification needed.Lines 92-96 duplicate the wait logic from lines 81-84. Additionally, the structure is confusing:
- The outer
ifat line 89 checks ifREGISTRATION_SERVICE_COMMANDenv var exists and adds it if missing- Line 98 patches the deployment command regardless of whether the env var was added
Consider simplifying the flow - if the deployment command patch at line 98 always runs when
DEBUG=true, the inner conditional wait (lines 92-96) may be unnecessary since the final patch will trigger a new rollout anyway.Simplified flow
if [[ "${DEBUG}" = true ]] then echo "✏️ patching the deployment's command to execute the registration service with Delve instead" - if ! oc get --namespace="${HOST_NS}" "${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="${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\"]"}}]' - - # Wait for the registration service's command to have the "dlv" bit, and the rollout for its deployment to be - # complete. - echo "Waiting for the registration service's deployment to get updated..." - oc wait --namespace="${HOST_NS}" --timeout=3m --for=jsonpath='{.spec.template.spec.containers[0].command[0]}'="dlv" "deployment/registration-service" - oc rollout status --namespace="${HOST_NS}" --timeout=3m deployment/registration-service - fi + if ! oc get --namespace="${HOST_NS}" "${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="${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 oc patch deployment/registration-service --namespace "${HOST_NS}" --type='json' --patch='[{"op": "replace", "path": "/spec/template/spec/containers/0/command", "value": ["dlv", "--listen=:50000", "--headless", "--continue", "--api-version=2", "--accept-multiclient", "exec", "/usr/local/bin/registration-service"]}]' + + # Wait for the deployment to roll out with the new command + oc wait --namespace="${HOST_NS}" --timeout=3m --for=jsonpath='{.spec.template.spec.containers[0].command[0]}'="dlv" "deployment/registration-service" + oc rollout status --namespace="${HOST_NS}" --timeout=3m deployment/registration-service fi
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-rs
branch
3 times, most recently
from
df68fc5 to
8bccf7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/deploy-dev.sh`:
- Around line 85-98: The script can skip waiting for the registration-service
rollout when REGISTRATION_SERVICE_COMMAND already exists; after the
unconditional oc patch that replaces the container command (the patch applied to
deployment/registration-service), always run the same rollout wait steps (oc
wait --for=jsonpath='{.spec.template.spec.containers[0].command[0]}'="dlv"
"deployment/registration-service" and oc rollout status --namespace="${HOST_NS}"
--timeout=3m deployment/registration-service) so the deployment finishes
updating before the script continues; place these oc wait/oc rollout status
calls immediately after the oc patch that sets the dlv command to ensure the
debug command is active.
In order to be able to debug the service live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work. For that purose, a new Dockerfile has been added which can build the image that way, and the corresponding Make target make it easy to kick off the whole process with a single command. The Makefile target also enable the "toolchain-e2e" repository to easily build the "debug" image. The "scripts/deploy-dev.sh" script is also modified to be able to refresh the registration service with the debug image if so desired. SANDBOX-1561
MikelAlejoBR
force-pushed
the
SANDBOX-1561-debug-sandbox-resources-rs
branch
from
8bccf7e to
f499357
Compare
|
1 participant
In order to be able to debug the service live in the local cluster, we need the binary to be executed with Delve instead, and we need it to be built without any code optimizations or minimization so that the breakpoints work.
For that purose, a new Dockerfile has been added which can build the image that way, and the corresponding Make target make it easy to kick off the whole process with a single command.
The Makefile target also enable the "toolchain-e2e" repository to easily build the "debug" image.
The "scripts/deploy-dev.sh" script is also modified to be able to refresh the registration service with the debug image if so desired.
Related pull requetss
Jira ticket
[SANDBOX-1561]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.