Skip to content

SANDBOX-1561 | feature: Make targets for debugging services and operators #1242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MikelAlejoBR wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SANDBOX-1561 | feature: Make targets for debugging services and operators #1242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ All e2e resources (host operator, member operator, registration-service, CRDs, e

* `make dev-deploy-e2e-local` - deploys the same resources as `make test-e2e-local` in dev environment but doesn't run tests.

* `make dev-deploy-e2e-local-debug` - deploys the same resources as `make test-e2e-local` in the local OpenShift
instance, but builds the services and the operators without any code optimizations. It also ships Delve in the images,
and launches those services with it, which leaves the debugger listening on port `50000`. Then, you can simply `oc
port-forward ${POD} 50000:50000` and connect to them with your favorite IDE. The services that are built like that are:
** The registration service.
** The host operator controller manager.
** The member operator controller manager.

* `make dev-deploy-e2e` - deploys the same resources as `make test-e2e` in dev environment but doesn't run tests.

* `make deploy-single-member-e2e-latest` - deploys the same resources (using the latest and greatest images of Toolchain operators) as `make test-e2e` but with only one member and doesn't run tests.
Expand Down
45 changes: 45 additions & 0 deletions make/dev.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,51 @@ setup-dev-sso:
.PHONY: dev-deploy-e2e-local
dev-deploy-e2e-local: deploy-e2e-local-to-dev-namespaces print-reg-service-link

# Builds the services' and operators' images with the debugger in it, so that
# 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:
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

.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:

  1. All subsequent targets will run their recipes in a single shell, which changes error propagation behavior.
  2. 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.

.PHONY: dev-deploy-e2e-local-debug
dev-deploy-e2e-local-debug: export DEBUG_MODE=true
Copy link
Contributor

@mfrancisc mfrancisc Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the export needed ?

Copy link
Contributor Author

@MikelAlejoBR MikelAlejoBR Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mfrancisc mfrancisc Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MikelAlejoBR MikelAlejoBR Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try it, because I'm not sure either...

Copy link
Contributor Author

@MikelAlejoBR MikelAlejoBR Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mfrancisc mfrancisc Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha! thanks for looking into it.

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)

# Patch the host operator indicating which command the registration
# service should be run with. The command simply adds an environment
# variable to the operator which then makes sure that the registration
# service is run with Delve, in case the user wants to connect to 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\"]"}}]'

# 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="${DEFAULT_HOST_NS}" --timeout=3m --for=jsonpath='{.spec.template.spec.containers[0].command[0]}'="dlv" "deployment/registration-service"
oc rollout status --namespace="${DEFAULT_HOST_NS}" --timeout=3m deployment/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}}}}'
@echo "Waiting for the registration service to get scaled to 1 replicas..."
oc wait --namespace="${DEFAULT_HOST_NS}" --timeout=3m --for=jsonpath='{.spec.replicas}'="1" "deployment/registration-service"

# 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"]}]'
@echo "Waiting for the host operator to be ready..."
oc wait --namespace "${DEFAULT_HOST_NS}" "$${HOST_CSV_NAME}" --for=jsonpath='{.status.phase}'=Succeeded --timeout=300s

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
Comment on lines +83 to +85
Copy link

@coderabbitai coderabbitai bot Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


.PHONY: dev-deploy-e2e-local-two-members
dev-deploy-e2e-local-two-members: deploy-e2e-local-to-dev-namespaces-two-members print-reg-service-link

Expand Down
14 changes: 12 additions & 2 deletions make/test.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,11 @@ ifneq (${FORCED_TAG},"")
$(eval FORCED_TAG_PARAM = -ft ${FORCED_TAG})
endif
endif
ifneq (${DEBUG_MODE},"")
ifneq (${DEBUG_MODE},)
$(eval DEBUG_MODE_PARAM = -dm ${DEBUG_MODE})
endif
endif
ifeq ($(DEPLOY_LATEST),true)
@echo "Installing latest version of the member-operator in namespace ${MEMBER_NS}"
${KSCTL_BIN_DIR}ksctl adm install-operator member --kubeconfig "$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --namespace ${MEMBER_NS} ${KSCTL_INSTALL_TIMEOUT_PARAM} -y
Expand All @@ -340,7 +345,7 @@ ifeq ($(DEPLOY_LATEST),true)
endif
else
@echo "Installing specific version of the member-operator"
scripts/ci/manage-member-operator.sh -po ${PUBLISH_OPERATOR} -io ${INSTALL_OPERATOR} -mn ${MEMBER_NS} ${MEMBER_REPO_PATH_PARAM} -qn ${QUAY_NAMESPACE} -ds ${DATE_SUFFIX} ${MEMBER_NS_2_PARAM} ${FORCED_TAG_PARAM}
scripts/ci/manage-member-operator.sh -po ${PUBLISH_OPERATOR} -io ${INSTALL_OPERATOR} -mn ${MEMBER_NS} ${MEMBER_REPO_PATH_PARAM} -qn ${QUAY_NAMESPACE} -ds ${DATE_SUFFIX} ${MEMBER_NS_2_PARAM} ${FORCED_TAG_PARAM} ${DEBUG_MODE_PARAM}
endif

.PHONY: get-and-publish-host-operator
Expand All @@ -360,12 +365,17 @@ ifneq (${FORCED_TAG},"")
$(eval FORCED_TAG_PARAM = -ft ${FORCED_TAG})
endif
endif
ifneq (${DEBUG_MODE},"")
ifneq (${DEBUG_MODE},)
$(eval DEBUG_MODE_PARAM = -dm ${DEBUG_MODE})
endif
endif
ifeq ($(DEPLOY_LATEST),true)
@echo "Installing latest version of the host-operator"
${KSCTL_BIN_DIR}ksctl adm install-operator host --kubeconfig "$(or ${KUBECONFIG}, ${HOME}/.kube/config)" --namespace ${HOST_NS} ${KSCTL_INSTALL_TIMEOUT_PARAM} -y
else
@echo "Installing specific version of the host-operator"
scripts/ci/manage-host-operator.sh -po ${PUBLISH_OPERATOR} -io ${INSTALL_OPERATOR} -hn ${HOST_NS} ${HOST_REPO_PATH_PARAM} -ds ${DATE_SUFFIX} -qn ${QUAY_NAMESPACE} ${REG_REPO_PATH_PARAM} ${FORCED_TAG_PARAM}
scripts/ci/manage-host-operator.sh -po ${PUBLISH_OPERATOR} -io ${INSTALL_OPERATOR} -hn ${HOST_NS} ${HOST_REPO_PATH_PARAM} -ds ${DATE_SUFFIX} -qn ${QUAY_NAMESPACE} ${REG_REPO_PATH_PARAM} ${FORCED_TAG_PARAM} ${DEBUG_MODE_PARAM}
endif

###########################################################
Expand Down
10 changes: 8 additions & 2 deletions scripts/ci/manage-host-operator.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ user_help () {
echo "-rr, --reg-repo-path Path to the registation service repo"
echo "-ds, --date-suffix Date suffix to be added to some resources that are created"
echo "-ft, --forced-tag Forces a tag to be set to all built images. In the case deployment the tag is used for index image in the created CatalogSource"
echo "-dm, --debug-mode Builds and pushes the operator to quay, including a Delve debugger to be able to launch the operator with it"
echo "-h, --help To show this help text"
echo ""
exit 0
Expand Down Expand Up @@ -68,6 +69,11 @@ read_arguments() {
FORCED_TAG=$1
shift
;;
-dm|--debug-mode)
shift
DEBUG_MODE=$1
shift
;;
*)
echo "$1 is not a recognized flag!" >> /dev/stderr
user_help
Expand Down Expand Up @@ -96,7 +102,7 @@ if [[ -n "${CI}${REG_REPO_PATH}${HOST_REPO_PATH}" ]] && [[ $(echo ${REPO_NAME} |
set_tags

if [[ ${PUBLISH_OPERATOR} == "true" ]]; then
push_image
push_image "${DEBUG_MODE}"
Copy link
Contributor

@rajivnathan rajivnathan Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@MikelAlejoBR MikelAlejoBR Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, yes. I do agree that it's better to have an additional option for consistency. Refactored!

REG_SERV_IMAGE_LOC=${IMAGE_LOC}
REG_REPO_PATH=${REPOSITORY_PATH}
fi
Expand All @@ -108,7 +114,7 @@ if [[ -n "${CI}${REG_REPO_PATH}${HOST_REPO_PATH}" ]] && [[ $(echo ${REPO_NAME} |
set_tags

if [[ ${PUBLISH_OPERATOR} == "true" ]]; then
push_image
push_image "${DEBUG_MODE}"
OPERATOR_IMAGE_LOC=${IMAGE_LOC}
make -C ${REPOSITORY_PATH} publish-current-bundle INDEX_IMAGE_TAG=${BUNDLE_AND_INDEX_TAG} BUNDLE_TAG=${BUNDLE_AND_INDEX_TAG} QUAY_NAMESPACE=${QUAY_NAMESPACE} OTHER_REPO_PATH=${REG_REPO_PATH} OTHER_REPO_IMAGE_LOC=${REG_SERV_IMAGE_LOC} IMAGE=${OPERATOR_IMAGE_LOC}
fi
Expand Down
8 changes: 7 additions & 1 deletion scripts/ci/manage-member-operator.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ user_help () {
echo "-mr, --member-repo-path Path to the member operator repo"
echo "-ds, --date-suffix Date suffix to be added to some resources that are created"
echo "-ft, --forced-tag Forces a tag to be set to all built images. In the case deployment the tag is used for index image in the created CatalogSource"
echo "-dm, --debug-mode Builds and pushes the operator to quay, including a Delve debugger to be able to launch the operator with it"
echo "-h, --help To show this help text"
echo ""
exit 0
Expand Down Expand Up @@ -67,6 +68,11 @@ read_arguments() {
FORCED_TAG=$1
shift
;;
-dm|--debug-mode)
shift
DEBUG_MODE=$1
shift
;;
*)
echo "$1 is not a recognized flag!" >> /dev/stderr
user_help
Expand Down Expand Up @@ -95,7 +101,7 @@ if [[ -n "${CI}${MEMBER_REPO_PATH}" ]] && [[ $(echo ${REPO_NAME} | sed 's/"//g')
set_tags

if [[ ${PUBLISH_OPERATOR} == "true" ]]; then
push_image
push_image "${DEBUG_MODE}"

OPERATOR_IMAGE_LOC=${IMAGE_LOC}
make -C ${REPOSITORY_PATH} publish-current-bundle INDEX_IMAGE_TAG=${BUNDLE_AND_INDEX_TAG} BUNDLE_TAG=${BUNDLE_AND_INDEX_TAG} QUAY_NAMESPACE=${QUAY_NAMESPACE} IMAGE=${OPERATOR_IMAGE_LOC}
Expand Down
14 changes: 12 additions & 2 deletions scripts/ci/manage-operator.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,21 @@ set_tags() {
}

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=""
fi

# Set a default image builder in case none has been specified.
IMAGE_BUILDER="${IMAGE_BUILDER:-podman}"

GIT_COMMIT_ID=$(git --git-dir=${REPOSITORY_PATH}/.git --work-tree=${REPOSITORY_PATH} rev-parse --short HEAD)
IMAGE_LOC=quay.io/codeready-toolchain/${REPOSITORY_NAME}:${GIT_COMMIT_ID}
if is_provided_or_paired; then
IMAGE_BUILDER=${IMAGE_BUILDER:-"podman"}
make -C ${REPOSITORY_PATH} ${IMAGE_BUILDER}-push QUAY_NAMESPACE=${QUAY_NAMESPACE} IMAGE_TAG=${TAGS}
make -C ${REPOSITORY_PATH} ${IMAGE_BUILDER}-push${DEBUG_MODE_SUFFIX} QUAY_NAMESPACE=${QUAY_NAMESPACE} IMAGE_TAG=${TAGS}
IMAGE_LOC=quay.io/${QUAY_NAMESPACE}/${REPOSITORY_NAME}:${TAGS}
fi
}
Expand Down
Loading