refactor: modernize make/generate.mk with versioned tool binaries and modernize project docs - #519
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:
WalkthroughThe PR replaces the deprecated Operator SDK generation command with ChangesGeneration workflow and guidance
Commit workflow
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
72dd6cc to
d79026c
Compare
xcoulon
changed the title
make/generate.mk with versioned tool binaries and modernize project docs
…nd modernize project docs - Update code generation instructions in API type comments from `"operator-sdk generate k8s"` to `"make generate"` - Delete `cmd/manager/main.go` dummy entrypoint that only existed for `operator-sdk generate csv` - Replace `README.adoc` with `README.md` and update build instructions - Adopt the Kubebuilder v4 pattern for local tool management: - Add `LOCALBIN` and `go-install-tool` helper for version-pinned binaries - Pin `controller-gen` to `v0.18.0`, `operator-sdk` to `v1.42.0` - Split `generate` (DeepCopy code) and `manifests` (CRD/webhook) targets - Regenerate `zz_generated.openapi.go` with updated tooling Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
d79026c to
4b29578
Compare
coderabbitai
Bot
added
agentic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
make/generate.mk (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
OPERATOR_SDK_VERSIONdeclaration. No Makefile target or recipe consumes it. Add an install target only if this dependency is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@make/generate.mk` around lines 17 - 18, Remove the unused OPERATOR_SDK_VERSION declaration from the Makefile. Do not add an install target unless the dependency is actually required by an existing target or recipe.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/commands/commit-with-message.md:
- Line 25: Update the commit command in the commit-with-message instructions to
use cryptographic signing with git commit -S or git commit --gpg-sign when
signed commits are required, rather than relying on -s, which only adds a
Signed-off-by trailer.
- Around line 16-25: Update the commit execution flow after user confirmation to
write the approved message to a temporary file, invoke git commit via `git
commit -s --file "$message_file"` without interpolating the message into shell
text, and remove the temporary file after the commit attempt, including on
failure.
In `@make/generate.mk`:
- Around line 9-10: Update the openapi-gen and crd-ref-docs targets to derive
installation, move, and invocation paths from LOCALBIN, use GOBIN=$(LOCALBIN),
and add $(LOCALBIN) as the crd-ref-docs prerequisite so overridden LOCALBIN
values remain consistent.
- Around line 113-114: Replace the malformed PHONY declaration with a .PHONY
declaration covering both prepare-host-operator and dispatch-crds, ensuring Make
always evaluates these preparation targets even when same-named files exist.
- Around line 15-23: Update the controller-gen build flow so changes to
CONTROLLER_TOOLS_VERSION trigger installation of the matching tool instead of
reusing the existing binary. Move the go-install-tool recipe from
$(CONTROLLER_GEN) to the phony controller-gen target, or otherwise include the
version in the dependency graph while preserving the existing controller-gen
invocation.
- Around line 33-46: Update the crd-ref-docs and openapi-gen targets to use
immutable generator versions rather than latest or master. Define explicit
version variables, install crd-ref-docs via go-install-tool with its version
included, and keep openapi-gen’s go-install-tool invocation tied to the pinned
OPENAPI_GEN_VERSION.
In `@README.md`:
- Around line 16-27: Update the README API-generation instructions to reflect
the make/generate.mk contract: document make generate for deepcopy and OpenAPI
generation, and make manifests as the required CRD-generation step, including
both commands and corrected numbered steps. Ensure the instructions no longer
claim that make generate alone regenerates config/crd/bases/.
---
Nitpick comments:
In `@make/generate.mk`:
- Around line 17-18: Remove the unused OPERATOR_SDK_VERSION declaration from the
Makefile. Do not add an install target unless the dependency is actually
required by an existing target or recipe.
🪄 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: Enterprise
Run ID: 8e6073b2-a77b-45a2-b20d-74803dd9e613
📒 Files selected for processing (19)
.claude/commands/commit-with-message.mdREADME.adocREADME.mdapi/v1alpha1/banneduser_types.goapi/v1alpha1/idler_types.goapi/v1alpha1/masteruserrecord_types.goapi/v1alpha1/memberstatus_types.goapi/v1alpha1/notification_types.goapi/v1alpha1/nstemplateset_types.goapi/v1alpha1/nstemplatetier_types.goapi/v1alpha1/proxyplugin_types.goapi/v1alpha1/socialevent_types.goapi/v1alpha1/toolchainstatus_types.goapi/v1alpha1/useraccount_types.goapi/v1alpha1/usersignup_types.goapi/v1alpha1/usertier_types.goapi/v1alpha1/zz_generated.openapi.gocmd/manager/main.gomake/generate.mk
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
💤 Files with no reviewable changes (2)
- cmd/manager/main.go
- README.adoc
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Verify Dependencies
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
api/v1alpha1/usertier_types.goapi/v1alpha1/nstemplateset_types.goapi/v1alpha1/socialevent_types.goapi/v1alpha1/nstemplatetier_types.goapi/v1alpha1/notification_types.goapi/v1alpha1/idler_types.goapi/v1alpha1/banneduser_types.goapi/v1alpha1/useraccount_types.goapi/v1alpha1/toolchainstatus_types.goapi/v1alpha1/masteruserrecord_types.goapi/v1alpha1/usersignup_types.goREADME.mdapi/v1alpha1/memberstatus_types.goapi/v1alpha1/proxyplugin_types.goapi/v1alpha1/zz_generated.openapi.gomake/generate.mk
🪛 LanguageTool
README.md
[style] ~26-~26: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...enerate zz_generated.openapi.go. 3. Generate API reference docs — uses [crd-ref-do...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.23.1)
README.md
[warning] 6-6: Link text should be descriptive
(MD059, descriptive-link-text)
[warning] 10-10: Link text should be descriptive
(MD059, descriptive-link-text)
[warning] 30-30: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🔇 Additional comments (18)
make/generate.mk (3)
25-31: LGTM!
58-73: LGTM!
88-90: LGTM!api/v1alpha1/banneduser_types.go (1)
23-23: LGTM!api/v1alpha1/idler_types.go (1)
26-26: LGTM!Also applies to: 36-36
api/v1alpha1/masteruserrecord_types.go (1)
51-51: LGTM!Also applies to: 86-86
api/v1alpha1/useraccount_types.go (1)
41-41: LGTM!Also applies to: 58-58
api/v1alpha1/usersignup_types.go (1)
215-215: LGTM!Also applies to: 284-284
api/v1alpha1/usertier_types.go (1)
24-24: LGTM!api/v1alpha1/zz_generated.openapi.go (1)
334-1450: LGTM!Also applies to: 1582-2360, 2569-3444, 3641-4458, 4606-5227
api/v1alpha1/memberstatus_types.go (1)
12-12: LGTM!Also applies to: 21-21, 81-81, 96-96
api/v1alpha1/notification_types.go (1)
40-40: LGTM!Also applies to: 75-75
api/v1alpha1/nstemplateset_types.go (1)
38-38: LGTM!api/v1alpha1/nstemplatetier_types.go (1)
19-19: LGTM!Also applies to: 89-89
api/v1alpha1/proxyplugin_types.go (1)
14-14: LGTM!Also applies to: 28-28
api/v1alpha1/socialevent_types.go (1)
25-25: LGTM!Also applies to: 62-62
api/v1alpha1/toolchainstatus_types.go (1)
59-59: LGTM!Also applies to: 68-68, 122-122, 150-150
README.md (1)
1-15: LGTM!Also applies to: 28-31
| - Surround folder names, file names, variable names, function and method names by backticks (`) | ||
| - Use the Conventional Commits from https://www.conventionalcommits.org/en/v1.0.0/ | ||
| - Include the "Assisted-by:" trail with the name of the current model | ||
|
|
||
| Show the suggested message and prompt the user to confirm that we shall proceed with committing the staged changes | ||
| Abort if the user is not happy with the message | ||
|
|
||
| 3. Commit | ||
|
|
||
| run the `git commit -s` command with the message prepared above No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n .claude/commands/commit-with-message.md
printf '%s\n' '--- related command instructions ---'
rg -n -C 3 'commit-with-message|git commit|Assisted-by|sign' .claude README.md .github 2>/dev/null || true
printf '%s\n' '--- repository status and recent metadata ---'
git status --short
git log -1 --format='%H%n%s'Repository: codeready-toolchain/api
Length of output: 2861
🏁 Script executed (no clone):
#!/bin/bash
set -eu
printf '%s\n' '--- git commit option semantics ---'
git commit --help 2>&1 | sed -n '/^-s, --signoff/,/^-S, --gpg-sign/p' || true
git commit -h 2>&1 | rg -n -- '-s|--signoff|-S|--gpg-sign|--file'
Length of output: 513
🏁 Script executed (no clone):
#!/bin/bash
set -eu
python3 - <<'PY'
import subprocess
message = "feat: preserve `printf INJECTED`"
# Unsafe interpolation into a shell command performs command substitution.
unsafe = subprocess.run(
f'printf "%s\\n" "{message}"',
shell=True, capture_output=True, text=True, check=True
).stdout.rstrip()
# Passing the message as an argument preserves the backticks as data.
safe = subprocess.run(
["printf", "%s\n", message],
capture_output=True, text=True, check=True
).stdout.rstrip()
print(f"unsafe={unsafe!r}")
print(f"safe={safe!r}")
PY
Length of output: 646
🏁 Script executed (no clone):
#!/bin/bash
set -eu
python3 - <<'PY'
import subprocess
message = "feat: preserve `printf INJECTED`"
unsafe = subprocess.run(
["/bin/bash", "-c", f'printf "%s\\n" "{message}"'],
capture_output=True, text=True, check=True
).stdout.rstrip()
safe = subprocess.run(
["/bin/bash", "-c", 'printf "%s\n" "$1"', "_", message],
capture_output=True, text=True, check=True
).stdout.rstrip()
print(f"unsafe={unsafe!r}")
print(f"safe={safe!r}")
PY
Length of output: 198
Pass the commit message as data, not shell text.
When the message is interpolated into a shell command, backticks and other shell metacharacters can execute commands or alter arguments. Write the confirmed message to a temporary file and use git commit -s --file "$message_file" without interpolation. Remove the temporary file after the commit attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/commit-with-message.md around lines 16 - 25, Update the
commit execution flow after user confirmation to write the approved message to a
temporary file, invoke git commit via `git commit -s --file "$message_file"`
without interpolating the message into shell text, and remove the temporary file
after the commit attempt, including on failure.
Source: Path instructions
|
|
||
| 3. Commit | ||
|
|
||
| run the `git commit -s` command with the message prepared above No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -u
git commit -h 2>&1 | grep -E -- '(-s, --signoff|-S, --gpg-sign)' || trueRepository: codeready-toolchain/api
Length of output: 161
🏁 Script executed:
#!/bin/sh
set -u
printf '%s\n' '--- target file ---'
cat -n .claude/commands/commit-with-message.md | sed -n '1,80p'
printf '%s\n' '--- signing-related references ---'
rg -n -i --hidden --glob '!/.git/**' 'signed|sign[- ]off|gpg|commit -s|commit -S|cryptograph' .claude README* .github 2>/dev/null || true
printf '%s\n' '--- local Git option behavior ---'
git commit --help 2>&1 | grep -E -- '(-s, --signoff|-S, --gpg-sign)' || true
git commit -h 2>&1 | sed -n '/signoff\|gpg-sign/p'Repository: codeready-toolchain/api
Length of output: 1568
Use cryptographic signing when required.
git commit -s adds a Signed-off-by trailer. If the PR requires cryptographically signed commits, use git commit -S or git commit --gpg-sign.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/commit-with-message.md at line 25, Update the commit
command in the commit-with-message instructions to use cryptographic signing
with git commit -S or git commit --gpg-sign when signed commits are required,
rather than relying on -s, which only adds a Signed-off-by trailer.
| CRD_REF_DOCS = $(PROJECT_DIR)/bin/crd-ref-docs | ||
| crd-ref-docs: ## Download crd-ref-docs locally if necessary. | ||
| GOBIN=$(PROJECT_DIR)/bin $(GO) install github.com/elastic/crd-ref-docs@latest | ||
|
|
||
| .PHONY: gen-crd-ref-docs | ||
| gen-crd-ref-docs: crd-ref-docs | ||
| @echo "Re-generating the api doc ref: ./api/$(API_VERSION)/docs/apiref.adoc " | ||
| $(CRD_REF_DOCS) --source-path ./api/$(API_VERSION) --config ./crdrefdocs.config.yaml --output-path ./api/$(API_VERSION)/docs/apiref.adoc | ||
|
|
||
| OPENAPI_GEN = $(PROJECT_DIR)/bin/openapi-gen | ||
| OPENAPI_GEN_VERSION ?= master | ||
|
|
||
| openapi-gen: ## Download openapi-gen locally if necessary. | ||
| $(call go-install-tool,$(OPENAPI_GEN),k8s.io/kube-openapi/cmd/openapi-gen,$(OPENAPI_GEN_VERSION)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -n '@(latest|master)' make/generate.mk; then
echo "Mutable generator versions remain."
exit 1
fi
rg -n 'CRD_REF_DOCS_VERSION|OPENAPI_GEN_VERSION' make/generate.mkRepository: codeready-toolchain/api
Length of output: 278
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- make/generate.mk ---'
cat -n make/generate.mk | sed -n '1,120p'
printf '%s\n' '--- helper definitions and generator references ---'
rg -n -C 3 'go-install-tool|crd-ref-docs|openapi-gen|OPENAPI_GEN_VERSION|CRD_REF_DOCS' Makefile make .github hack 2>/dev/null || true
printf '%s\n' '--- generated artifacts and related targets ---'
rg -n -C 2 'gen-crd-ref-docs|openapi-gen|apiref\.adoc|generated.*openapi|openapi' --glob 'Makefile' --glob '*.mk' --glob '*.yaml' --glob '*.yml' --glob '*.go' . 2>/dev/null | head -300Repository: codeready-toolchain/api
Length of output: 30094
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- shared generator patterns ---'
rg -n -C 3 'go-install-tool|crd-ref-docs|openapi-gen|OPENAPI_GEN_VERSION|CRD_REF_DOCS' Makefile make .github hack 2>/dev/null || trueRepository: codeready-toolchain/toolchain-common
Length of output: 208
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- consumer and generator patterns ---'
rg -n -C 3 'go-install-tool|crd-ref-docs|openapi-gen|OPENAPI_GEN_VERSION|CRD_REF_DOCS' Makefile make .github hack 2>/dev/null || trueRepository: codeready-toolchain/host-operator
Length of output: 211
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- generator patterns ---'
rg -n -C 3 'go-install-tool|crd-ref-docs|openapi-gen|OPENAPI_GEN_VERSION|CRD_REF_DOCS' Makefile make .github hack 2>/dev/null || trueRepository: codeready-toolchain/toolchain-e2e
Length of output: 198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- build entry points and overrides ---'
rg -n -C 3 'generate\.mk|OPENAPI_GEN_VERSION|CONTROLLER_TOOLS_VERSION|OPERATOR_SDK_VERSION|gen-crd-ref-docs|generate-openapi' \
--glob '!vendor/**' --glob '!bin/**' . | head -400
printf '%s\n' '--- module versions for generator packages ---'
rg -n -C 2 'k8s\.io/kube-openapi|elastic/crd-ref-docs|controller-tools' go.mod go.sum tools.go 2>/dev/null || true
printf '%s\n' '--- tracked generated artifacts ---'
git ls-files | rg '(^|/)(zz_generated\.openapi\.go|apiref\.adoc|config/crd/bases/)' | head -200
printf '%s\n' '--- relevant repository history summary ---'
git log --oneline -8 -- make/generate.mk go.mod tools.goRepository: codeready-toolchain/api
Length of output: 4820
🏁 Script executed (no clone):
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("/tmp/nonexistent").read_text() if False else Path(".").read_text() if False else ""
PY
Length of output: 125
Pin the generators for committed artifacts.
crd-ref-docs@latest and OPENAPI_GEN_VERSION ?= master resolve mutable upstream code, although k8s.io/kube-openapi is pinned in go.mod. Use exact versions or immutable commits for both generators. Install crd-ref-docs through go-install-tool so its cache key includes the version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@make/generate.mk` around lines 33 - 46, Update the crd-ref-docs and
openapi-gen targets to use immutable generator versions rather than latest or
master. Define explicit version variables, install crd-ref-docs via
go-install-tool with its version included, and keep openapi-gen’s
go-install-tool invocation tied to the pinned OPENAPI_GEN_VERSION.
Source: Path instructions
| PHONY: prepare-host-operator | ||
| prepare-host-operator: ../host-operator |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if rg -n '^PHONY:' make/generate.mk; then
exit 1
fi
rg -n '^\.(PHONY): (prepare-host-operator|prepare-member-operator)' \
make/generate.mkRepository: codeready-toolchain/api
Length of output: 229
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- make/generate.mk relevant sections ---'
cat -n make/generate.mk | sed -n '90,140p'
printf '%s\n' '--- target and variable references ---'
rg -n 'prepare-(host|member)-operator|CRD|pending-change|PHONY' Makefile make .github 2>/dev/null || trueRepository: codeready-toolchain/api
Length of output: 6721
🏁 Script executed (no clone):
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import os
import subprocess
import tempfile
makefile = r'''
PHONY: prepare-host-operator
prepare-host-operator:
`@echo` ran
'''
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, 'Makefile')
with open(path, 'w') as f:
f.write(makefile)
open(os.path.join(d, 'prepare-host-operator'), 'w').close()
result = subprocess.run(
['make', '-C', d, '-B', 'prepare-host-operator'],
text=True, capture_output=True
)
print('make_available:', result.returncode == 0)
print(result.stdout)
print(result.stderr)
PY
Length of output: 644
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- target-name files and references ---'
git ls-files | rg '(^|/)(prepare-host-operator|prepare-member-operator)$' || true
rg -n --hidden --glob '!.git' '(^|[^[:alnum:]_-])prepare-(host|member)-operator([^[:alnum:]_-]|$)' . || trueRepository: codeready-toolchain/api
Length of output: 521
🏁 Script executed (no clone):
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- available make implementations ---'
command -v make || true
command -v gmake || true
command -v busybox || true
if command -v busybox >/dev/null 2>&1; then
busybox | sed -n '1,3p'
busybox make --help 2>&1 | sed -n '1,8p' || true
fi
Length of output: 164
🌐 Web query:
GNU make manual phony targets target file exists prerequisite newer behavior
💡 Result:
In GNU make, a phony target is a target that does not represent a real file, but rather a name for a recipe to be executed [1][2]. Because phony targets are not real files, they do not have modification times, and make treats them differently regarding prerequisites and existence [1][3]. Key behaviors include: 1. Unconditional Execution: When a target is declared as phony (using the.PHONY special target), make will execute its recipe every time the target is requested, regardless of whether a file of the same name exists or its modification time [1][3]. 2. Prerequisite Behavior: A phony target should not be used as a prerequisite for a real target file [1][2]. If a real target file lists a phony target as a prerequisite, the real target's recipe will be executed every time make updates that file, because make treats the phony prerequisite as always being out-of-date [1][2]. 3. Order-only Prerequisites: To execute a recipe (like a setup task) without forcing a dependent target to rebuild whenever that task is completed, you can use the phony target as an order-only prerequisite (prefixed by a pipe symbol |) [4]. With an order-only prerequisite, the dependent target will only be rebuilt if its normal, non-order-only prerequisites have changed [4]. If you need a target to perform an action only when necessary based on timestamps (rather than unconditionally), you should use a real file target instead of a phony target [3]. A common pattern for this is the "empty target" or "dummy file," where the recipe includes a command (typically touch) to update the file's modification time after the action is completed [3]. This allows make to compare the file's timestamp against its dependencies in subsequent runs [3].
Citations:
- 1: https://www.gnu.org.cach3.com/software/make/manual/html_node/Phony-Targets.html
- 2: http://gnu.ist.utl.pt/software/make/manual/html_node/Phony-Targets.html
- 3: https://stackoverflow.com/questions/13852535/makefile-dependencies-dont-work-for-phony-target
- 4: https://stackoverflow.com/questions/46823844/how-to-create-non-dependent-phony-targets-in-a-gnu-makefile
Use .PHONY for both preparation targets.
PHONY defines a target named PHONY. It does not mark either preparation target as phony. If a same-named file exists, Make can skip the checks and cleanup required by dispatch-crds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@make/generate.mk` around lines 113 - 114, Replace the malformed PHONY
declaration with a .PHONY declaration covering both prepare-host-operator and
dispatch-crds, ensuring Make always evaluates these preparation targets even
when same-named files exist.
Source: Path instructions
| @echo "Re-generating the deepcopy go file & the Toolchain CRD files... " | ||
| $(Q)$(CONTROLLER_GEN) crd \ | ||
| object paths="./..." output:crd:artifacts:config=$(PATH_TO_CRD_BASES) | ||
| generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. |
There was a problem hiding this comment.
previously, the generate target executed all steps
generate-deepcopy-and-crds generate-openapi gen-crd-ref-docs dispatch-crds
but currently it runs only controller-gen
this doesn't with what is in README and is regression in terms of functionality of the same target
There was a problem hiding this comment.
ohh damn'it, I missed that! 🤦♂️
thanks for spotting this regression @MatousJobanek!
restored in 84267dd
| generate: generate-deepcopy-and-crds generate-openapi gen-crd-ref-docs dispatch-crds | ||
|
|
||
| .PHONY: generate-deepcopy-and-crds | ||
| generate-deepcopy-and-crds: remove-config controller-gen |
There was a problem hiding this comment.
previously, before generating the deepcopy & CRDs, it executed the remove-config which I don't see it executed anywhere - is this an obsolete cleanup that can be removed, or is it still needed to ensure we use clean environment?
| CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen | ||
| PATH_TO_CRD_BASES=config/crd/bases | ||
| CONTROLLER_TOOLS_VERSION ?= v0.18.0 | ||
| OPERATOR_SDK_VERSION ?= v1.42.0 |
There was a problem hiding this comment.
the OPERATOR_SDK_VERSION doesn't seem to be used
There was a problem hiding this comment.
ah right, I forgot to remove that one
| # go-install-tool will 'go install' any package with custom target and name of binary, if it doesn't exist | ||
| # $1 - target path with name of binary | ||
| # $2 - package url which can be installed | ||
| # $3 - specific version of package | ||
| define go-install-tool | ||
| @[ -f "$(1)-$(3)" ] || { \ | ||
| set -e; \ | ||
| package=$(2)@$(3) ;\ | ||
| echo "Downloading $${package}" ;\ | ||
| rm -f $(1) || true ;\ | ||
| GOBIN=$(LOCALBIN) go install $${package} ;\ | ||
| mv $(1) $(1)-$(3) ;\ | ||
| } ;\ | ||
| ln -sf $(1)-$(3) $(1) | ||
| endef |
There was a problem hiding this comment.
do we actually need this at all?
couldn't we define the needed libraries in go.mod as tools?
tool (
sigs.k8s.io/controller-tools/cmd/controller-gen
k8s.io/kube-openapi/cmd/openapi-gen
github.com/elastic/crd-ref-docs
)
and then install them via go install tool in a similar way as it is done now?
Lines 96 to 106 in 734640c
we just need to move this
https://github.com/codeready-toolchain/api/blob/master/tools.go
into go.mod and probably clean it up a bit
There was a problem hiding this comment.
I used the code that is now generated by Operator SDK (see https://github.com/codeready-toolchain/claw-operator/blob/master/Makefile#L486-L514)
There was a problem hiding this comment.
let's discuss what we prefer (tools.go or plain makefile), and let's remove what we don't want to keep, shall we?
There was a problem hiding this comment.
We used to have something like that there before (at that time, I modified the makefile target that was generated by Operator SDK to something more robust and flexible).
We already had discussion about the makefile target (I cannot find the reference to the discussion, maybe it was in a call) and you suggested to drop it and use go.mod & standard go install instead. My PR that dropped the target #406
IIRC, the main reason for doing that was having only one place for updating the dependencies - go.mod so we don't have to remember that we need to update also the makefile. And it's also simpler.
It's been working fine till now, so if there is nothing to fix or improve in terms of flexibility/maintainability, then I would vote for keeping it as it is.
and yes, fully agree with removing what is obsolete and not used 👍
There was a problem hiding this comment.
ok, sounds good, let's keep it as it is then
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@make/generate.mk`:
- Line 29: Update the generate target dependency order so dispatch-crds runs
only after generate-crd completes, while preserving the other generation
prerequisites and existing target behavior. Adjust the target dependencies
around generate-crd and dispatch-crds rather than changing their
implementations.
🪄 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: Enterprise
Run ID: 1e067a00-2167-4988-a587-eec13b477b4c
📒 Files selected for processing (1)
make/generate.mk
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Verify Dependencies
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
make/generate.mk
🔀 Multi-repo context codeready-toolchain/host-operator, codeready-toolchain/toolchain-e2e
Linked repositories findings
codeready-toolchain/host-operator
- Host Operator pins
github.com/codeready-toolchain/apito the PR’s commit and also pinscontroller-toolstov0.18.0(go.mod:5,go.mod:31). [::codeready-toolchain/host-operator::] - Its generated CRDs use
controller-genv0.18.0 (config/crd/bases/toolchain.dev.openshift.com_usersignups.yaml:6), matching the API PR’s tooling update. [::codeready-toolchain/host-operator::] PROJECTreferences the API repository’sapi/v1alpha1path for every resource (for example,PROJECT:15-16), confirming generation depends on the API checkout layout. [::codeready-toolchain/host-operator::]- Existing CRDs retain field defaults (for example,
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml:152), while this PR’s OpenAPI regeneration removes redundant defaults only fromzz_generated.openapi.go; no cross-repository CRD default change was observed. [::codeready-toolchain/host-operator::]
codeready-toolchain/toolchain-e2e
- E2E pins the same API commit (
go.mod:4) and importsapi/v1alpha1throughout setup and tests, making it a broad compile/runtime consumer of the API types. [::codeready-toolchain/toolchain-e2e::] - No references to the removed
generate-deepcopy-and-crdstarget or API repository generation targets were found. [::codeready-toolchain/toolchain-e2e::]
🔇 Additional comments (4)
make/generate.mk (4)
31-37: MakeCONTROLLER_TOOLS_VERSIONaffect target freshness.This repeats the existing review finding.
generate-objectandgenerate-crddepend on the unversioned$(CONTROLLER_GEN)path. If that symlink already exists, changingCONTROLLER_TOOLS_VERSIONcan leave the old binary selected.Make the versioned binary or a version stamp part of the prerequisite graph, or invoke a phony installer target that refreshes the symlink.
#!/usr/bin/env bash set -euo pipefail rg -n -C 4 \ 'CONTROLLER_TOOLS_VERSION|CONTROLLER_GEN|go-install-tool|controller-gen:' \ make/generate.mk
64-81: UseLOCALBINfor thecrd-ref-docspath.This repeats the existing review finding. The new installer uses
$(LOCALBIN), butCRD_REF_DOCSstill points to$(PROJECT_DIR)/bin/crd-ref-docs. IfLOCALBINis overridden, the tool can be installed in one directory and invoked from another.Derive
CRD_REF_DOCS, its prerequisite, andGOBINfrom$(LOCALBIN).-CRD_REF_DOCS = $(PROJECT_DIR)/bin/crd-ref-docs +CRD_REF_DOCS = $(LOCALBIN)/crd-ref-docs#!/usr/bin/env bash set -euo pipefail rg -n -C 4 \ 'LOCALBIN|CRD_REF_DOCS|crd-ref-docs|GOBIN|go-install-tool' \ make/generate.mk
119-141: Keep both preparation targets phony.This repeats the existing review finding. Ensure
prepare-host-operatorandprepare-member-operatoreach have a.PHONYdeclaration. Otherwise, a same-named file can cause Make to skip pending-change validation, cleanup, and directory creation.#!/usr/bin/env bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re text = Path("make/generate.mk").read_text() for target in ("prepare-host-operator", "prepare-member-operator"): assert re.search( rf"^\.PHONY:.*\b{re.escape(target)}\b", text, re.M, ), f"{target} is not declared phony" print("Both preparation targets are phony.") PY
94-96: LGTM!
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
make/generate.mk (1)
75-75: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize CRD dispatch after CRD generation.
Line 75 makes
dispatch-crdsdepend only on the preparation targets. The dispatch recipe reads, copies, and removes files underconfig/crd/bases, while the CRD generation target writes that directory. Ifgenerateexposes both targets as independent prerequisites,make -j generatecan dispatch stale or incomplete CRDs.Add
generate-crdas a prerequisite ofdispatch-crds, or enforce the same ordering in thegeneraterecipe.Proposed fix
-dispatch-crds: prepare-host-operator prepare-member-operator +dispatch-crds: generate-crd prepare-host-operator prepare-member-operator#!/usr/bin/env bash set -euo pipefail python3 - <<'PY' from pathlib import Path lines = Path("make/generate.mk").read_text().splitlines() line = next(line for line in lines if line.startswith("dispatch-crds:")) dependencies = line.split(":", 1)[1].split() assert "generate-crd" in dependencies, line PYAs per path instructions, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@make/generate.mk` at line 75, Update the dispatch-crds target prerequisites so generate-crd runs before dispatch-crds, ensuring CRD files are fully generated before they are read, copied, or removed during parallel make execution. Preserve the existing prepare-host-operator and prepare-member-operator prerequisites.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@make/generate.mk`:
- Around line 26-27: Update the crd-ref-docs installation in the $(CRD_REF_DOCS)
target to use a reviewed, immutable version or commit instead of `@latest`. Define
that pinned version as a Make variable and include it in the target/dependency
graph so Make rebuilds or reuses only the matching binary, while preserving the
existing local installation path and execution flow.
- Around line 18-24: Update the controller-gen installation rule associated with
CONTROLLER_TOOLS_VERSION to pass that version explicitly in the module install
target, and make the target graph depend on the version so changing it refreshes
the binary instead of reusing a stale output. Apply the same freshness strategy
to the OPENAPI_GEN target using its pinned version, or remove the standalone
controller-tools variable and consistently derive both versions from go.mod.
---
Outside diff comments:
In `@make/generate.mk`:
- Line 75: Update the dispatch-crds target prerequisites so generate-crd runs
before dispatch-crds, ensuring CRD files are fully generated before they are
read, copied, or removed during parallel make execution. Preserve the existing
prepare-host-operator and prepare-member-operator prerequisites.
🪄 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: Enterprise
Run ID: 00f84fdc-b618-4f33-a612-ecbf4b25dc11
📒 Files selected for processing (2)
make/generate.mktools.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
💤 Files with no reviewable changes (1)
- tools.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Verify Dependencies
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
make/generate.mk
🔀 Multi-repo context
No additional cross-repository breaking changes were found. Existing consumers pin the API commit and compatible controller-tools version; no references to the removed Make target were observed.
🔇 Additional comments (1)
make/generate.mk (1)
16-17: LGTM!Also applies to: 29-30, 45-57, 70-72
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
just fix the missing env var; otherwise, looks good 👍
| $(OPENAPI_GEN): go.mod | $(LOCALBIN) ## install openapi-gen locally if necessary. Version is pinned in go.mod. | ||
| GOBIN=$(LOCALBIN) $(GO) install k8s.io/kube-openapi/cmd/openapi-gen | ||
|
|
||
| $(CRD_REF_DOCS): go.mod | $(LOCALBIN) ## install crd-ref-docs locally if necessary. Version is pinned in go.mod. |
There was a problem hiding this comment.
I don't see the CRD_REF_DOCS var to be defined anywhere
There was a problem hiding this comment.
ah good catch! 🤦♂️
thanks!
fixed in b97b57c
| ## Tool Binaries | ||
| CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen | ||
| OPENAPI_GEN ?= $(LOCALBIN)/openapi-gen | ||
| PATH_TO_CRD_BASES=config/crd/bases |
There was a problem hiding this comment.
related to the other comment, I guess you are missing
| PATH_TO_CRD_BASES=config/crd/bases | |
| CRD_REF_DOCS ?= $(LOCALBIN)/crd-ref-docs | |
| PATH_TO_CRD_BASES=config/crd/bases |
| # how to dispatch the CRD files per repository (space-separated lists) | ||
| HOST_CLUSTER_CRDS:=masteruserrecords nstemplatetiers usersignups bannedusers notifications spaces spacebindings socialevents tiertemplates tiertemplaterevisions toolchainstatuses toolchainclusters toolchainconfigs usertiers proxyplugins spacerequests spacebindingrequests spaceprovisionerconfigs | ||
| MEMBER_CLUSTER_CRDS:=useraccounts nstemplatesets memberstatuses idlers toolchainclusters memberoperatorconfigs spacerequests workspaces spacebindingrequests | ||
| PROJECT_DIR := $(shell pwd) |
There was a problem hiding this comment.
nitpick: and this can be probably removed too - doesn't seem to be used anymore 👻
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
thanks for the review, @MatousJobanek! 🙌 |
…pdate and k8s module updates) Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
|
3 participants
"operator-sdk generate k8s"to"make generate"cmd/manager/main.godummy entrypoint that only existed foroperator-sdk generate csvREADME.adocwithREADME.mdand update build instructionsLOCALBINandgo-install-toolhelper for version-pinned binariescontroller-gentov0.18.0,operator-sdktov1.42.0generate(DeepCopy code) andmanifests(CRD/webhook) targetszz_generated.openapi.gowith updated toolingAssisted-by: Claude Opus 4.6 (1M context)
Signed-off-by: Xavier Coulon xcoulon@redhat.com
Summary by CodeRabbit
Documentation
make generateworkflow.Chores