Standardize dev workflow with Makefile includes and developer documentation#335
Open
lucas-benedito wants to merge 2 commits intoansible:mainfrom
Open
Standardize dev workflow with Makefile includes and developer documentation#335lucas-benedito wants to merge 2 commits intoansible:mainfrom
lucas-benedito wants to merge 2 commits intoansible:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces shell-based dev scripts with Makefile-driven workflows (shared and operator-specific make fragments), updates CONTRIBUTING/README/docs to a contributor-focused guide, adjusts an example EDA CR (AWX URL and CA bundle), and changes kustomize namespace to Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 92-97: The docker-buildx target currently suppresses failures by
prefixing the build command(s) with '-', which hides build errors in CI; update
the Makefile's docker-buildx target to remove the leading '-' from the `docker
buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile .` (and
any other `docker buildx` lines that start with '-') so failures propagate,
ensure the target still creates/uses/removes the builder via `docker buildx
create --name project-v3-builder`, `docker buildx use project-v3-builder`, and
`docker buildx rm project-v3-builder` without ignoring their exit codes.
- Around line 184-190: The bundle/catalog push targets use a hardcoded
docker-push command causing failures when CONTAINER_TOOL is set to podman;
update the Makefile targets bundle-push and catalog-push to invoke
$(CONTAINER_TOOL)-push instead of docker-push so they follow the existing
pattern used by bundle-build and catalog-build and respect the CONTAINER_TOOL
variable.
In `@makefiles/common.mk`:
- Around line 258-286: The dev-build target calls a non-existent
$(CONTAINER_TOOL)-buildx-push (expands to docker-buildx-push) when HOST and
CLUSTER arch differ; update the invocation in makefiles/common.mk inside the
dev-build recipe so it uses the existing $(CONTAINER_TOOL)-buildx target (or
otherwise ensure a docker-buildx-push target is provided). Specifically, replace
the $(MAKE) $(CONTAINER_TOOL)-buildx-push IMG=... PLATFORMS=linux/$$CLUSTER_ARCH
call (referenced in the dev-build rule) with a call to $(MAKE)
$(CONTAINER_TOOL)-buildx IMG=... PLATFORMS=linux/$$CLUSTER_ARCH so Docker uses
the defined buildx target, or add a docker-buildx-push target mirroring
podman-buildx-push if push semantics are required.
- Around line 35-36: The validation in _require-img currently forces any DEV_IMG
pointing at quay.io to require QUAY_USER and to match quay.io/$(QUAY_USER)/...,
which breaks explicit DEV_IMG overrides; change _require-img to first check if
DEV_IMG is explicitly set and, if so, parse its registry and namespace (e.g.,
split on '/' to get registry and user/repo) and validate the format
independently instead of requiring equality with QUAY_USER, allowing
DEV_IMG=quay.io/myuser/... without QUAY_USER; keep existing QUAY_USER-based
checks only when DEV_IMG is not provided and continue to honor REGISTRIES
fallback logic.
- Around line 290-298: The patch-pull-policy target currently only greps for
'imagePullPolicy: IfNotPresent' so once the file is changed the rule won't match
again; update the grep and sed expressions in the patch-pull-policy recipe to
match any current policy value (i.e., search for lines beginning with
'imagePullPolicy:' regardless of the value) and replace only the value portion
with the new _POLICY (use a regex like matching 'imagePullPolicy: .*' and a sed
replace that writes 'imagePullPolicy: $$_POLICY' via the existing $(_SED_I)
invocation). Ensure IMAGE_PULL_POLICY, _POLICY and the file loop
(config/manager/manager.yaml) remain unchanged otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 783e8f89-3df2-4f13-9cb7-ab1434cec28e
📒 Files selected for processing (9)
CONTRIBUTING.mdDEPRECATED_down.shDEPRECATED_up.shMakefileREADME.mddev/eda-cr/eda-openshift-cr.ymldocs/development.mdmakefiles/common.mkmakefiles/operator.mk
lucas-benedito
force-pushed
the
standardization-makefile
branch
from
e515b7f to
cfde1af
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
Makefile (2)
92-97:⚠️ Potential issue | 🟠 MajorStop masking
docker buildxfailures.The
-prefixes on Lines 94, 96, and 97 let setup/build errors pass as a successful target, which can leave CI green without a published image.Suggested fix
docker-buildx: ## Build and push docker image for the manager for cross-platform support - - docker buildx create --name project-v3-builder - docker buildx use project-v3-builder - - docker buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile . - - docker buildx rm project-v3-builder + `@set` -e; \ + docker buildx inspect project-v3-builder >/dev/null 2>&1 || docker buildx create --name project-v3-builder; \ + trap 'docker buildx rm project-v3-builder >/dev/null 2>&1 || true' EXIT; \ + docker buildx use project-v3-builder; \ + docker buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 92 - 97, The Makefile target "docker-buildx" is masking failures by prefixing the docker buildx commands with '-' so errors are ignored; remove the leading '-' characters from the commands "docker buildx create --name project-v3-builder", "docker buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile .", and "docker buildx rm project-v3-builder" in the docker-buildx target so build/create/remove failures surface and the CI fails on errors.
188-190:⚠️ Potential issue | 🟠 MajorBundle/catalog push targets still hardcode Docker.
With
CONTAINER_TOOL ?= podman, Lines 190 and 230 break on podman-only setups because they always dispatch todocker-push.Suggested fix
bundle-push: ## Push the bundle image. - $(MAKE) docker-push IMG=$(BUNDLE_IMG) + $(MAKE) $(CONTAINER_TOOL)-push IMG=$(BUNDLE_IMG) @@ catalog-push: ## Push a catalog image. - $(MAKE) docker-push IMG=$(CATALOG_IMG) + $(MAKE) $(CONTAINER_TOOL)-push IMG=$(CATALOG_IMG)Also applies to: 228-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 188 - 190, The make targets currently call a hardcoded docker-push; update the bundle-push (and the analogous catalog-push) target(s) to invoke the push recipe dynamically using the CONTAINER_TOOL variable instead of "docker" so podman setups work. Replace uses of "$(MAKE) docker-push IMG=$(BUNDLE_IMG)" (and the catalog equivalent using CATALOG_IMG) with "$(MAKE) $(CONTAINER_TOOL)-push IMG=$(BUNDLE_IMG)" (and "$(MAKE) $(CONTAINER_TOOL)-push IMG=$(CATALOG_IMG)"), keeping the IMG argument intact and leaving CONTAINER_TOOL's defaulting logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 184-186: The bundle-build target bypasses the shared container
wrapper and loses PODMAN_CONNECTION and BUILD_ARGS; update the bundle-build
recipe so it uses the shared wrapper variable $(_CONTAINER_CMD) (instead of raw
$(CONTAINER_TOOL)) and passes $(BUILD_ARGS) when invoking build, still
referencing bundle.Dockerfile and $(BUNDLE_IMG); specifically modify the
bundle-build target to call $(_CONTAINER_CMD) build -f bundle.Dockerfile
$(BUILD_ARGS) -t $(BUNDLE_IMG) . so PODMAN_CONNECTION and other wrapper behavior
are preserved.
- Around line 92-96: The docker-buildx Makefile target is missing the BUILD_ARGS
when invoking docker buildx build; update the docker-buildx rule (target name:
docker-buildx) to include $(BUILD_ARGS) in the build command so it mirrors the
podman path and forwards build-time args (keep --push --platform=$(PLATFORMS)
--tag ${IMG} -f Dockerfile . and insert $(BUILD_ARGS) among the docker buildx
build flags).
- Around line 149-153: Update the curl invocations in the Makefile that download
operator binaries to include the -f flag so HTTP 4xx/5xx responses cause curl to
fail and trigger set -e; specifically, change the curl calls that write to
$(ANSIBLE_OPERATOR) and the other two download targets that use
OPERATOR_SDK_VERSION, OS and ARCH (the three curl -sSLo invocations mentioned)
to use curl -sSLo -f (or curl -fsSL o style) so a failed HTTP response won't
produce an HTML error file and will abort the recipe before chmod runs.
---
Duplicate comments:
In `@Makefile`:
- Around line 92-97: The Makefile target "docker-buildx" is masking failures by
prefixing the docker buildx commands with '-' so errors are ignored; remove the
leading '-' characters from the commands "docker buildx create --name
project-v3-builder", "docker buildx build --push --platform=$(PLATFORMS) --tag
${IMG} -f Dockerfile .", and "docker buildx rm project-v3-builder" in the
docker-buildx target so build/create/remove failures surface and the CI fails on
errors.
- Around line 188-190: The make targets currently call a hardcoded docker-push;
update the bundle-push (and the analogous catalog-push) target(s) to invoke the
push recipe dynamically using the CONTAINER_TOOL variable instead of "docker" so
podman setups work. Replace uses of "$(MAKE) docker-push IMG=$(BUNDLE_IMG)" (and
the catalog equivalent using CATALOG_IMG) with "$(MAKE) $(CONTAINER_TOOL)-push
IMG=$(BUNDLE_IMG)" (and "$(MAKE) $(CONTAINER_TOOL)-push IMG=$(CATALOG_IMG)"),
keeping the IMG argument intact and leaving CONTAINER_TOOL's defaulting logic
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6eae0f1-4ba1-44cf-80d6-7c165f67f6d1
📒 Files selected for processing (10)
CONTRIBUTING.mdDEPRECATED_down.shDEPRECATED_up.shMakefileREADME.mdconfig/default/kustomization.yamldev/eda-cr/eda-openshift-cr.ymldocs/development.mdmakefiles/common.mkmakefiles/operator.mk
✅ Files skipped from review due to trivial changes (2)
- config/default/kustomization.yaml
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- dev/eda-cr/eda-openshift-cr.yml
- docs/development.md
- makefiles/common.mk
- makefiles/operator.mk
lucas-benedito
force-pushed
the
standardization-makefile
branch
2 times, most recently
from
160b18a to
26bb93e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development.md`:
- Around line 85-86: The example uses DEV_IMG with a tag causing a double-tag
when makefiles/operator.mk composes $(DEV_IMG):$(DEV_TAG); update the docs so
DEV_IMG is tagless (e.g., quay.io/myuser/eda-server-operator) and set the tag in
DEV_TAG (e.g., DEV_TAG=latest) or show the combined env usage
(DEV_IMG=quay.io/myuser/eda-server-operator DEV_TAG=latest make up) to match how
makefiles/operator.mk constructs $(DEV_IMG):$(DEV_TAG).
In `@Makefile`:
- Around line 118-120: The undeploy recipe calls $(KUSTOMIZE) directly but
doesn't ensure the kustomize binary is bootstrapped; update the undeploy target
to depend on the kustomize bootstrap target (add kustomize as a prerequisite to
the undeploy target) so the kustomize binary is prepared before running the
command (i.e., change the undeploy target declaration to include the kustomize
prerequisite).
In `@makefiles/common.mk`:
- Around line 177-185: The ns-wait Make target currently loops forever if a
namespace stays in Terminating; modify the ns-wait recipe to implement a bounded
timeout by adding a max-wait variable (e.g., NS_WAIT_TIMEOUT or MAX_ATTEMPTS)
and either a counter or elapsed-time check inside the while loop that breaks and
exits with a non-zero status if the timeout is reached; use the existing KUBECTL
and NAMESPACE symbols to poll as now, but after the timeout print a clear error
like "Namespace $(NAMESPACE) still terminating after $TIMEOUT seconds" and
return failure so make up can fail fast instead of hanging.
In `@makefiles/operator.mk`:
- Around line 45-50: The _operator-deploy target currently mutates
config/default/kustomization.yaml by running "cd config/default && $(KUSTOMIZE)
edit set namespace $(NAMESPACE)"; instead avoid in-place edits and instead
build/apply with the namespace at runtime (or pass NAMESPACE into the deploy
target). Replace the edit line with a non-mutating flow such as using
"$(KUSTOMIZE) build config/default | kubectl apply -n $(NAMESPACE) -f -" or
change the deploy target to accept NAMESPACE and use "$(KUSTOMIZE) build
config/default" piped to kubectl with "-n $(NAMESPACE)"; ensure references to
KUSTOMIZE, NAMESPACE, _operator-deploy, deploy, DEV_IMG and DEV_TAG are updated
so no file edits to config/default/kustomization.yaml occur.
- Around line 35-43: The _operator-build-and-push target is unconditionally
calling $(_CONTAINER_CMD) push $(DEV_IMG):$(DEV_TAG) even though dev-build may
already push via docker-buildx or podman-buildx-push; change
_operator-build-and-push to only run the explicit push when dev-build did not
already push (e.g. check a flag like BUILD_PUSHED != "1"), and update the
multi-arch build targets (docker-buildx, podman-buildx-push and the
auto-detected docker-buildx-push path) to set BUILD_PUSHED=1 when they perform a
push so the final push is skipped for those paths. Ensure references remain to
_operator-build-and-push, dev-build, docker-buildx, podman-buildx-push,
docker-buildx-push, $(_CONTAINER_CMD), DEV_IMG and DEV_TAG.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1da6dbfc-bc4e-4dcf-ae08-1f3ce8bf48f5
📒 Files selected for processing (10)
CONTRIBUTING.mdDEPRECATED_down.shDEPRECATED_up.shMakefileREADME.mdconfig/default/kustomization.yamldev/eda-cr/eda-openshift-cr.ymldocs/development.mdmakefiles/common.mkmakefiles/operator.mk
✅ Files skipped from review due to trivial changes (1)
- config/default/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
makefiles/operator.mk (1)
102-106: Use$(IMAGE_TAG_BASE)instead of hardcoded image path.Line 104 hardcodes
quay.io/ansible/eda-server-operatorwhileIMAGE_TAG_BASEis already defined on line 8 with the same value. Using the variable ensures consistency if the base changes.Suggested fix
.PHONY: generate-operator-yaml generate-operator-yaml: kustomize ## Generate operator.yaml with image tag $(VERSION) - `@cd` config/manager && $(KUSTOMIZE) edit set image controller=quay.io/ansible/eda-server-operator:${VERSION} + `@cd` config/manager && $(KUSTOMIZE) edit set image controller=$(IMAGE_TAG_BASE):$(VERSION) @$(KUSTOMIZE) build config/default > ./operator.yaml `@echo` "Generated operator.yaml with image tag $(VERSION)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/operator.mk` around lines 102 - 106, The make target generate-operator-yaml hardcodes the image base instead of using the existing IMAGE_TAG_BASE variable; update the recipe so the image set command uses $(IMAGE_TAG_BASE):$(VERSION) rather than the literal quay.io/ansible/eda-server-operator:${VERSION}. Specifically, edit the generate-operator-yaml target (function/target name: generate-operator-yaml) replacing the literal in the KUSTOMIZE edit set image invocation with $(IMAGE_TAG_BASE):$(VERSION) and keep the rest of the lines (KUSTOMIZE build and echo) unchanged so IMAGE_TAG_BASE and VERSION are used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@makefiles/operator.mk`:
- Around line 102-106: The make target generate-operator-yaml hardcodes the
image base instead of using the existing IMAGE_TAG_BASE variable; update the
recipe so the image set command uses $(IMAGE_TAG_BASE):$(VERSION) rather than
the literal quay.io/ansible/eda-server-operator:${VERSION}. Specifically, edit
the generate-operator-yaml target (function/target name: generate-operator-yaml)
replacing the literal in the KUSTOMIZE edit set image invocation with
$(IMAGE_TAG_BASE):$(VERSION) and keep the rest of the lines (KUSTOMIZE build and
echo) unchanged so IMAGE_TAG_BASE and VERSION are used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faf8a4cf-f8be-42c8-835a-5acb00cd04de
📒 Files selected for processing (10)
CONTRIBUTING.mdMakefileREADME.mdconfig/default/kustomization.yamldev/eda-cr/eda-openshift-cr.ymldocs/development.mddown.shmakefiles/common.mkmakefiles/operator.mkup.sh
💤 Files with no reviewable changes (2)
- down.sh
- up.sh
✅ Files skipped from review due to trivial changes (3)
- config/default/kustomization.yaml
- dev/eda-cr/eda-openshift-cr.yml
- docs/development.md
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- makefiles/common.mk
lucas-benedito
force-pushed
the
standardization-makefile
branch
from
26bb93e to
1ac4521
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (10)
Makefile (4)
124-126:⚠️ Potential issue | 🟠 MajorBootstrap
kustomizebeforeundeploy.
undeployshells out to$(KUSTOMIZE)but has nokustomizeprerequisite, so it fails on a clean checkout.Suggested fix
.PHONY: undeploy -undeploy: ## Undeploy controller from the K8s cluster specified in ~/.kube/config. +undeploy: kustomize ## Undeploy controller from the K8s cluster specified in ~/.kube/config. $(KUSTOMIZE) build config/default | kubectl delete --ignore-not-found=$(ignore-not-found) -f -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 124 - 126, The undeploy Makefile target calls $(KUSTOMIZE) but has no prerequisite, so add the kustomize bootstrap target as a dependency for the undeploy rule: update the undeploy target (named "undeploy") to depend on the existing kustomize bootstrap target (or create one if missing) so that kustomize is prepared before running the command that uses $(KUSTOMIZE).
99-103:⚠️ Potential issue | 🟠 MajorPropagate
docker buildxfailures.The leading
-on Line 102 makes cross-platform builds look successful even when nothing was pushed, so later deploy steps can keep running against a stale tag.Suggested fix
docker-buildx: ## Build and push docker image for the manager for cross-platform support - docker buildx create --name project-v3-builder docker buildx use project-v3-builder - - docker buildx build --push $(BUILD_ARGS) --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile . + docker buildx build --push $(BUILD_ARGS) --platform=$(PLATFORMS) --tag ${IMG} -f Dockerfile . - docker buildx rm project-v3-builder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 99 - 103, The docker-buildx recipe currently swallows failures because the commands under the docker-buildx target are prefixed with a leading '-' (silencing errors); update the docker-buildx Makefile target (docker-buildx) to remove the leading '-' on commands such as `docker buildx build --push ...` and `docker buildx rm project-v3-builder` so any buildx failures propagate and stop the pipeline; ensure `docker buildx use project-v3-builder` remains and consider leaving `docker buildx create --name project-v3-builder` (without a leading '-') if present so creation errors also fail fast.
150-159:⚠️ Potential issue | 🟠 MajorFail binary downloads on HTTP errors.
curl -sSLostill exits 0 on HTTP 4xx/5xx and writes the error body to disk. The followingchmod +xleaves a broken tool inbin/.Suggested fix
- curl -sSLo $(ANSIBLE_OPERATOR) https://github.com/operator-framework/ansible-operator-plugins/releases/download/$(OPERATOR_SDK_VERSION)/ansible-operator_$(OS)_$(ARCH) ;\ + curl -fsSLo $(ANSIBLE_OPERATOR) https://github.com/operator-framework/ansible-operator-plugins/releases/download/$(OPERATOR_SDK_VERSION)/ansible-operator_$(OS)_$(ARCH) ;\ @@ - curl -sSLo $(OPERATOR_SDK) https://github.com/operator-framework/operator-sdk/releases/download/$(OPERATOR_SDK_VERSION)/operator-sdk_$(OS)_$(ARCH) ;\ + curl -fsSLo $(OPERATOR_SDK) https://github.com/operator-framework/operator-sdk/releases/download/$(OPERATOR_SDK_VERSION)/operator-sdk_$(OS)_$(ARCH) ;\ @@ - curl -sSLo $(OPM) https://github.com/operator-framework/operator-registry/releases/download/v1.55.0/$(OS)-$(ARCH)-opm ;\ + curl -fsSLo $(OPM) https://github.com/operator-framework/operator-registry/releases/download/v1.55.0/$(OS)-$(ARCH)-opm ;\Also applies to: 166-175, 198-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 150 - 159, The download commands in the ansible-operator target (and the similar targets for other binaries) use curl -sSLo which does not fail on HTTP 4xx/5xx and can leave a corrupted binary; change the invocation to use curl --fail (e.g. -fSLo) or otherwise check the HTTP status and write to a temporary file first, only moving it into $(ANSIBLE_OPERATOR) and running chmod +x after a successful download, and ensure you remove the temp file on failure; apply the same fix to the other download rules that reference OPERATOR_SDK_VERSION, OS, and ARCH to avoid leaving broken tools in bin/.
190-196:⚠️ Potential issue | 🟠 MajorFinish the
CONTAINER_TOOLmigration for bundle/catalog images.
bundle-buildbypasses$(_CONTAINER_CMD), soPODMAN_CONNECTIONandBUILD_ARGSare lost, and the push targets still calldocker-push. On the default podman path, the newly documented bundle workflow still breaks.Suggested fix
bundle-build: ## Build the bundle image. - $(CONTAINER_TOOL) build -f bundle.Dockerfile -t $(BUNDLE_IMG) . + $(_CONTAINER_CMD) build $(BUILD_ARGS) -f bundle.Dockerfile -t $(BUNDLE_IMG) . @@ bundle-push: ## Push the bundle image. - $(MAKE) docker-push IMG=$(BUNDLE_IMG) + $(MAKE) $(CONTAINER_TOOL)-push IMG=$(BUNDLE_IMG) @@ catalog-push: ## Push a catalog image. - $(MAKE) docker-push IMG=$(CATALOG_IMG) + $(MAKE) $(CONTAINER_TOOL)-push IMG=$(CATALOG_IMG)Also applies to: 229-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 190 - 196, bundle-build and bundle-push are bypassing the _CONTAINER_CMD wrapper (and still call docker-push), losing PODMAN_CONNECTION and BUILD_ARGS; change the bundle-build target to invoke $(_CONTAINER_CMD) build -f bundle.Dockerfile -t $(BUNDLE_IMG) . (so BUILD_ARGS and PODMAN_CONNECTION are applied) and change bundle-push to call the generic container push target instead of docker-push (e.g., $(MAKE) container-push IMG=$(BUNDLE_IMG) or the existing push target that uses $(_CONTAINER_CMD)); update the other duplicate block (lines 229-236) the same way and keep the IMG=$(BUNDLE_IMG) argument.docs/development.md (1)
85-86:⚠️ Potential issue | 🟠 MajorUse
DEV_TAGfor existing tagged images.This example currently expands to
quay.io/myuser/eda-server-operator:latest:dev, and under the currentquay.io/...validation it also fails becauseQUAY_USERis omitted.Suggested doc fix
# Deploy without building (use an existing image) -BUILD_IMAGE=false DEV_IMG=quay.io/myuser/eda-server-operator:latest make up +BUILD_IMAGE=false QUAY_USER=myuser DEV_TAG=latest make up🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development.md` around lines 85 - 86, The example uses DEV_IMG which your tooling appends a :dev suffix and also expects QUAY_USER, causing an invalid image like quay.io/myuser/eda-server-operator:latest:dev; update the docs to use DEV_TAG instead of DEV_IMG and show the correct environment variables (e.g., BUILD_IMAGE=false DEV_TAG=quay.io/myuser/eda-server-operator:latest QUAY_USER=myuser make up) so the image value isn't double-suffixed and QUAY_USER is provided.makefiles/common.mk (5)
129-133:⚠️ Potential issue | 🟠 MajorDon't persist
NAMESPACEintoconfig/default/kustomization.yaml.Line 132 edits a tracked file in place, so
make up NAMESPACE=...leaves the repo dirty and changes later deploys until the file is reset. Apply the built manifest with the namespace at runtime instead ofkustomize edit set namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/common.mk` around lines 129 - 133, The target _operator-deploy should not mutate the tracked kustomization.yaml; remove the in-place `cd config/default && $(KUSTOMIZE) edit set namespace $(NAMESPACE)` step and instead generate/apply the manifest with the namespace at runtime: run `$(KUSTOMIZE) build config/default` and pipe the output into `kubectl apply -n $(NAMESPACE) -f -` (or use `kubectl apply --namespace=$(NAMESPACE) -f -`) before invoking `$(MAKE) deploy IMG=$(DEV_IMG):$(DEV_TAG)` so the namespace is applied during manifest application without editing tracked files; update the _operator-deploy recipe accordingly.
387-413:⚠️ Potential issue | 🟠 MajorAllow explicit
DEV_IMGoverrides onquay.io.Once the user sets
DEV_IMG, this block still requiresQUAY_USERand forcesDEV_IMGto matchquay.io/$(QUAY_USER)/.... That rejects valid overrides likeDEV_IMG=quay.io/myuser/eda-server-operator make up, which this file header and the docs advertise. Only enforce theQUAY_USERmatch whenDEV_IMGcame from the default assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/common.mk` around lines 387 - 413, The quay.io checks in the _require-img target are too strict because they always require QUAY_USER and force DEV_IMG to match quay.io/$(QUAY_USER)/... even when the user explicitly set DEV_IMG; wrap the quay.io-specific validation in a make conditional that only runs when DEV_IMG came from the Makefile default (e.g., ifeq ($(origin DEV_IMG),default)) so explicit environment overrides (DEV_IMG=quay.io/whatever) are allowed, and keep the existing echo/error behavior inside that conditional; reference the _require-img target and variables DEV_IMG and QUAY_USER when making the change.
356-364:⚠️ Potential issue | 🟠 MajorMake
IMAGE_PULL_POLICYre-patchable.After the first run changes
IfNotPresent, later runs no longer match the hardcoded grep/sed pattern, so switching betweenAlwaysandNeversilently stops working.Suggested fix
patch-pull-policy: ## Patch imagePullPolicy in manager config (default: Always, override with IMAGE_PULL_POLICY) `@_POLICY`="$(if $(IMAGE_PULL_POLICY),$(IMAGE_PULL_POLICY),Always)"; \ for file in config/manager/manager.yaml; do \ - if [ -f "$$file" ] && grep -q 'imagePullPolicy: IfNotPresent' "$$file"; then \ + if [ -f "$$file" ] && grep -Eq 'imagePullPolicy: (IfNotPresent|Always|Never)' "$$file"; then \ echo "Patching imagePullPolicy to $$_POLICY in $$file"; \ - $(_SED_I) "s|imagePullPolicy: IfNotPresent|imagePullPolicy: $$_POLICY|g" "$$file"; \ + $(_SED_I) -E "s|imagePullPolicy: (IfNotPresent|Always|Never)|imagePullPolicy: $$_POLICY|g" "$$file"; \ fi; \ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/common.mk` around lines 356 - 364, The patch-pull-policy make target currently greps and sed-replaces only the literal 'imagePullPolicy: IfNotPresent', causing subsequent runs to miss files after the value was changed; update the logic in the patch-pull-policy recipe so it derives _POLICY from IMAGE_PULL_POLICY as now, then match any existing imagePullPolicy value (e.g. grep for 'imagePullPolicy:' or use a regex like 'imagePullPolicy:[[:space:]]*.*') and replace the entire line with 'imagePullPolicy: $$_POLICY' so switching between Always/Never/IfNotPresent works idempotently; update the grep and $(_SED_I) invocation in the patch-pull-policy target accordingly.
240-248:⚠️ Potential issue | 🟠 MajorBound
ns-wait.If the namespace stays
Terminating,make uphangs forever here. A timeout with non-zero exit makes the failure recoverable.Suggested fix
ns-wait: ## Wait for namespace to finish terminating `@if` $(KUBECTL) get namespace $(NAMESPACE) 2>/dev/null | grep -q 'Terminating'; then \ echo "Namespace $(NAMESPACE) is terminating. Waiting..."; \ - while $(KUBECTL) get namespace $(NAMESPACE) 2>/dev/null | grep -q 'Terminating'; do \ + ATTEMPTS=0; \ + while $(KUBECTL) get namespace $(NAMESPACE) 2>/dev/null | grep -q 'Terminating'; do \ + if [ $$ATTEMPTS -ge 60 ]; then \ + echo "ERROR: Namespace $(NAMESPACE) is still terminating after 5 minutes." >&2; \ + exit 1; \ + fi; \ + ATTEMPTS=$$((ATTEMPTS + 1)); \ sleep 5; \ done; \ echo "Namespace $(NAMESPACE) terminated."; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/common.mk` around lines 240 - 248, The ns-wait target can hang forever if the namespace stays in Terminating; modify the ns-wait recipe to add a bounded timeout (e.g., loop with a max retries counter or use the timeout utility) while polling $(KUBECTL) for namespace $(NAMESPACE), sleep between attempts, and if the max wait is exceeded exit non-zero and log an error; update the block around the ns-wait target that uses $(KUBECTL) so it returns failure on timeout instead of looping indefinitely.
119-127:⚠️ Potential issue | 🟠 MajorUse one push contract for
dev-build.
_operator-build-and-pushalways pushes afterdev-build, butdev-buildalready routes some Docker paths throughdocker-buildx --push, and Line 344 still expands to a non-existentdocker-buildx-push. Today that means Docker cross-arch builds either fail immediately or hit a redundant second push, and the “local build without push” branch is never actually local.Also applies to: 324-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@makefiles/common.mk` around lines 119 - 127, _operator-build-and-push currently always runs a second push after calling dev-build, which conflicts with dev-build's own docker-buildx --push paths and references a non-existent docker-buildx-push; remove the unconditional push (the echo "Pushing $(DEV_IMG):$(DEV_TAG)..." and the $(_CONTAINER_CMD) push line) and instead delegate push responsibility to dev-build by invoking it with an explicit push flag when needed (e.g. call "$(MAKE) dev-build BUILD_PUSH=true" or pass BUILD_IMAGE through), or otherwise rely on dev-build's internal logic so only one push contract exists; update _operator-build-and-push to only call dev-build and not perform a separate push (references: target _operator-build-and-push, target dev-build, variables BUILD_IMAGE, DEV_IMG, DEV_TAG, and docker-buildx --push / docker-buildx-push).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@makefiles/operator.mk`:
- Around line 49-56: The awx-url make target currently only queries OpenShift
Route (uses $(KUBECTL) get route) which breaks plain Kubernetes/minikube users;
update the awx-url recipe in makefiles/operator.mk (target name: awx-url,
variables: $(KUBECTL), AWX_NAMESPACE) to first try Route, then fall back to
checking Ingress (querying ingress host or IP), then fall back to Service
(NodePort/LoadBalancer clusterIP+port) and return a usable URL, and if none
found emit the existing error; alternatively, if you prefer not to implement the
fallback, update documentation to clearly state awx-url requires OpenShift Route
and instruct users how to get the URL on plain Kubernetes (ingress/service).
---
Duplicate comments:
In `@docs/development.md`:
- Around line 85-86: The example uses DEV_IMG which your tooling appends a :dev
suffix and also expects QUAY_USER, causing an invalid image like
quay.io/myuser/eda-server-operator:latest:dev; update the docs to use DEV_TAG
instead of DEV_IMG and show the correct environment variables (e.g.,
BUILD_IMAGE=false DEV_TAG=quay.io/myuser/eda-server-operator:latest
QUAY_USER=myuser make up) so the image value isn't double-suffixed and QUAY_USER
is provided.
In `@Makefile`:
- Around line 124-126: The undeploy Makefile target calls $(KUSTOMIZE) but has
no prerequisite, so add the kustomize bootstrap target as a dependency for the
undeploy rule: update the undeploy target (named "undeploy") to depend on the
existing kustomize bootstrap target (or create one if missing) so that kustomize
is prepared before running the command that uses $(KUSTOMIZE).
- Around line 99-103: The docker-buildx recipe currently swallows failures
because the commands under the docker-buildx target are prefixed with a leading
'-' (silencing errors); update the docker-buildx Makefile target (docker-buildx)
to remove the leading '-' on commands such as `docker buildx build --push ...`
and `docker buildx rm project-v3-builder` so any buildx failures propagate and
stop the pipeline; ensure `docker buildx use project-v3-builder` remains and
consider leaving `docker buildx create --name project-v3-builder` (without a
leading '-') if present so creation errors also fail fast.
- Around line 150-159: The download commands in the ansible-operator target (and
the similar targets for other binaries) use curl -sSLo which does not fail on
HTTP 4xx/5xx and can leave a corrupted binary; change the invocation to use curl
--fail (e.g. -fSLo) or otherwise check the HTTP status and write to a temporary
file first, only moving it into $(ANSIBLE_OPERATOR) and running chmod +x after a
successful download, and ensure you remove the temp file on failure; apply the
same fix to the other download rules that reference OPERATOR_SDK_VERSION, OS,
and ARCH to avoid leaving broken tools in bin/.
- Around line 190-196: bundle-build and bundle-push are bypassing the
_CONTAINER_CMD wrapper (and still call docker-push), losing PODMAN_CONNECTION
and BUILD_ARGS; change the bundle-build target to invoke $(_CONTAINER_CMD) build
-f bundle.Dockerfile -t $(BUNDLE_IMG) . (so BUILD_ARGS and PODMAN_CONNECTION are
applied) and change bundle-push to call the generic container push target
instead of docker-push (e.g., $(MAKE) container-push IMG=$(BUNDLE_IMG) or the
existing push target that uses $(_CONTAINER_CMD)); update the other duplicate
block (lines 229-236) the same way and keep the IMG=$(BUNDLE_IMG) argument.
In `@makefiles/common.mk`:
- Around line 129-133: The target _operator-deploy should not mutate the tracked
kustomization.yaml; remove the in-place `cd config/default && $(KUSTOMIZE) edit
set namespace $(NAMESPACE)` step and instead generate/apply the manifest with
the namespace at runtime: run `$(KUSTOMIZE) build config/default` and pipe the
output into `kubectl apply -n $(NAMESPACE) -f -` (or use `kubectl apply
--namespace=$(NAMESPACE) -f -`) before invoking `$(MAKE) deploy
IMG=$(DEV_IMG):$(DEV_TAG)` so the namespace is applied during manifest
application without editing tracked files; update the _operator-deploy recipe
accordingly.
- Around line 387-413: The quay.io checks in the _require-img target are too
strict because they always require QUAY_USER and force DEV_IMG to match
quay.io/$(QUAY_USER)/... even when the user explicitly set DEV_IMG; wrap the
quay.io-specific validation in a make conditional that only runs when DEV_IMG
came from the Makefile default (e.g., ifeq ($(origin DEV_IMG),default)) so
explicit environment overrides (DEV_IMG=quay.io/whatever) are allowed, and keep
the existing echo/error behavior inside that conditional; reference the
_require-img target and variables DEV_IMG and QUAY_USER when making the change.
- Around line 356-364: The patch-pull-policy make target currently greps and
sed-replaces only the literal 'imagePullPolicy: IfNotPresent', causing
subsequent runs to miss files after the value was changed; update the logic in
the patch-pull-policy recipe so it derives _POLICY from IMAGE_PULL_POLICY as
now, then match any existing imagePullPolicy value (e.g. grep for
'imagePullPolicy:' or use a regex like 'imagePullPolicy:[[:space:]]*.*') and
replace the entire line with 'imagePullPolicy: $$_POLICY' so switching between
Always/Never/IfNotPresent works idempotently; update the grep and $(_SED_I)
invocation in the patch-pull-policy target accordingly.
- Around line 240-248: The ns-wait target can hang forever if the namespace
stays in Terminating; modify the ns-wait recipe to add a bounded timeout (e.g.,
loop with a max retries counter or use the timeout utility) while polling
$(KUBECTL) for namespace $(NAMESPACE), sleep between attempts, and if the max
wait is exceeded exit non-zero and log an error; update the block around the
ns-wait target that uses $(KUBECTL) so it returns failure on timeout instead of
looping indefinitely.
- Around line 119-127: _operator-build-and-push currently always runs a second
push after calling dev-build, which conflicts with dev-build's own docker-buildx
--push paths and references a non-existent docker-buildx-push; remove the
unconditional push (the echo "Pushing $(DEV_IMG):$(DEV_TAG)..." and the
$(_CONTAINER_CMD) push line) and instead delegate push responsibility to
dev-build by invoking it with an explicit push flag when needed (e.g. call
"$(MAKE) dev-build BUILD_PUSH=true" or pass BUILD_IMAGE through), or otherwise
rely on dev-build's internal logic so only one push contract exists; update
_operator-build-and-push to only call dev-build and not perform a separate push
(references: target _operator-build-and-push, target dev-build, variables
BUILD_IMAGE, DEV_IMG, DEV_TAG, and docker-buildx --push / docker-buildx-push).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c99e9b35-2db5-429e-a263-c56b626f1752
📒 Files selected for processing (10)
CONTRIBUTING.mdMakefileREADME.mdconfig/default/kustomization.yamldev/eda-cr/eda-openshift-cr.ymldocs/development.mddown.shmakefiles/common.mkmakefiles/operator.mkup.sh
💤 Files with no reviewable changes (2)
- down.sh
- up.sh
✅ Files skipped from review due to trivial changes (2)
- config/default/kustomization.yaml
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- dev/eda-cr/eda-openshift-cr.yml
Introduce modular Makefile system (common.mk + operator.mk) for consistent dev workflows. Standardize CONTRIBUTING.md and docs/development.md to follow community conventions with clear separation: contributing guidelines for process, development guide for technical setup. - Add common.mk with shared dev workflow targets (make up/down) - Add operator.mk with EDA-specific variables and targets - Restructure CONTRIBUTING.md: process, testing requirements, community links - Expand docs/development.md: customization options table, teardown options, bundle generation via make targets - Simplify README.md contributing section Assisted-by: Claude Signed-off-by: Lucas Benedito <lbenedit@redhat.com>
lucas-benedito
force-pushed
the
standardization-makefile
branch
from
1ac4521 to
c80f3fc
Compare
AlexSCorey
reviewed
Apr 10, 2026
Member
AlexSCorey
left a comment
AlexSCorey
left a comment
There was a problem hiding this comment.
@lucas-benedito just a few questions and a1 area of confusion for me regarding DEV_CR vs _EDA_DEV_CR.
|
|
||
| # CA Bundle | ||
| bundle_cacert_secret: my-custom-certs | ||
| # bundle_cacert_secret: my-custom-certs |
Member
Author
There was a problem hiding this comment.
We've been keeping some of these parameters commented out in the example dev cr, as a new user can easily apply the parameter if they need it. We only do that for more commonly used parameters.
I would probably keep the commented out value here just for future reference.
|
|
||
| # Default ENGINE for building the operator (default docker) | ||
| ENGINE ?= docker | ||
| # VERSION ?= 0.0.1 # Set in operator.mk |
Member
Author
There was a problem hiding this comment.
For now, I will keep it. The next Makefile update that we do (it will happen on all operators) then I can drop the commented out value from the operator-sdk sourced Makefile.
I only kept to ensure we know where that value will be set as most developers familiar with the standard Makefile would check the value set here
Comment on lines
+31
to
+32
| # example.com/temp-operator-bundle:$VERSION and example.com/temp-operator-catalog:$VERSION. | ||
| # IMAGE_TAG_BASE ?= quay.io/<org>/<operator-name> # Set in operator.mk |
Member
Author
There was a problem hiding this comment.
For now, I will keep it. The next Makefile update that we do (it will happen on all operators) then I can drop the commented out value from the operator-sdk sourced Makefile.
I only kept to ensure we know where that value will be set as most developers familiar with the standard Makefile would check the value set here
- Rename _EDA_DEV_CR to DEV_CR in operator.mk for consistency with awx-operator and galaxy-operator - Add ingress fallback to awx-url target for k8s/minikube support - Fix DEV_IMG docs example to avoid double-tag issue Assisted-by: Claude Signed-off-by: Lucas Benedito <lbenedit@redhat.com>
|
rooftopcellist
approved these changes
Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Standardizes the dev workflow by introducing shared Makefile includes and restructuring the developer documentation.
Changes:
makefiles/common.mkwith shared targets:make up,make down,make lint, registry login, namespace management, secrets, and build helpersmakefiles/operator.mkwith EDA-specific deploy flow, AWX URL discovery (make awx-url), and connection configurationCONTRIBUTING.mdwith standard community sectionsdocs/development.mdwith prerequisites, registry setup, customization options table, teardown options table, and testing instructionsREADME.mdContributing section to link both fileshttps://awx.example.com) replaced at deploy time viaAUTOMATION_SERVER_URLenv varbundle_cacert_secretin dev CR (references non-existent secret)edaup.sh/down.sh(replaced bymake up/make down)EDA requires a running AWX instance. The deploy flow validates
AUTOMATION_SERVER_URLis set and providesmake awx-urlto discover it from the cluster.ISSUE TYPE
ADDITIONAL INFORMATION
Tested on OCP 4.20 SNO cluster:
AUTOMATION_SERVER_URL=$(make awx-url) QUAY_USER=username make up— operator deploys, all pods running, reconciliation successfulmake down— clean teardownmake awx-url AWX_NAMESPACE=awx— discovers AWX route URLSummary by CodeRabbit
Documentation
Chores
Configuration