refactor: modernize make/generate.mk with versioned tool binaries and modernize project docs - #519
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🔀 Multi-repo contextNo 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. WalkthroughThe pull request updates Make-based generation tooling, API generation comments, generated OpenAPI schemas, and repository documentation. It replaces ChangesGeneration and repository guidance
Estimated code review effort: 2 (Simple) | ~10 minutes 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.
| LOCALBIN ?= $(shell pwd)/bin | ||
| $(LOCALBIN): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
out="$(make -n generate-openapi LOCALBIN="$tmp/bin" 2>&1)"
printf '%s\n' "$out"
grep -F "$tmp/bin/openapi-gen" <<<"$out"
out="$(make -n gen-crd-ref-docs LOCALBIN="$tmp/bin" 2>&1)"
printf '%s\n' "$out"
grep -F "GOBIN=$tmp/bin" <<<"$out"Repository: codeready-toolchain/api
Length of output: 165
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- make/generate.mk ---'
cat -n make/generate.mk | sed -n '1,90p'
printf '%s\n' '--- Makefile includes and relevant targets ---'
rg -n -C 3 'generate\.mk|LOCALBIN|CRD_REF_DOCS|OPENAPI_GEN|generate-openapi|gen-crd-ref-docs|crd-ref-docs|openapi-gen' Makefile make . 2>/dev/null | head -240
printf '%s\n' '--- available make command ---'
command -v make || true
make --version 2>&1 | head -2 || trueRepository: codeready-toolchain/api
Length of output: 22079
Use $(LOCALBIN) for all local tool paths.
When LOCALBIN is overridden, openapi-gen is installed in one directory but moved or invoked from $(PROJECT_DIR)/bin. crd-ref-docs has the same mismatch and lacks a $(LOCALBIN) prerequisite. Derive both paths from $(LOCALBIN), add the prerequisite, and use GOBIN=$(LOCALBIN).
🤖 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 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.
Source: Path instructions
| CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen | ||
| PATH_TO_CRD_BASES=config/crd/bases | ||
| CONTROLLER_TOOLS_VERSION ?= v0.18.0 | ||
| OPERATOR_SDK_VERSION ?= v1.42.0 | ||
|
|
||
| PROJECT_DIR := $(shell pwd) | ||
| .PHONY: controller-gen | ||
| controller-gen: $(CONTROLLER_GEN) ## Download controller-gen locally if necessary. | ||
| $(CONTROLLER_GEN): $(LOCALBIN) | ||
| $(call go-install-tool,$(CONTROLLER_GEN),sigs.k8s.io/controller-tools/cmd/controller-gen,$(CONTROLLER_TOOLS_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
tmp="$(mktemp -d)"
printf 'old' > "$tmp/controller-gen-v0.17.0"
ln -s controller-gen-v0.17.0 "$tmp/controller-gen"
out="$(make -n controller-gen LOCALBIN="$tmp" CONTROLLER_TOOLS_VERSION=v0.18.0)"
printf '%s\n' "$out"
grep -F 'controller-gen-v0.18.0' <<<"$out"Repository: codeready-toolchain/api
Length of output: 203
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- make/generate.mk ---'
cat -n make/generate.mk | sed -n '1,70p'
printf '%s\n' '--- installer definitions and related targets ---'
rg -n -C 5 'go-install-tool|CONTROLLER_GEN|LOCALBIN|controller-gen|manifests|generate' Makefile make config 2>/dev/null | sed -n '1,240p'Repository: codeready-toolchain/api
Length of output: 6874
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("make/generate.mk")
text = p.read_text()
assert re.search(r"^\.PHONY: controller-gen$", text, re.M)
assert re.search(r"^controller-gen:\s+\$\(CONTROLLER_GEN\)", text, re.M)
assert re.search(r"^\$\(CONTROLLER_GEN\):\s+\$\(LOCALBIN\)", text, re.M)
assert re.search(r'^\s*@\[ -f "\$\(1\)-\$\(3\)" \] \|\|', text, re.M)
assert re.search(r'^\s*ln -sf \$\(1\)-\$\(3\) \$\(1\)$', text, re.M)
controller_target = re.search(
r"^controller-gen:\s+([^\n]+)\n(?:.*\n)*?\$\(CONTROLLER_GEN\):\s+([^\n]+)",
text, re.M
)
install_guard = re.search(r'\[ -f "\$\(1\)-\$\(3\)" \]', text)
print("controller-gen is phony:", bool(re.search(r"^\.PHONY: controller-gen$", text, re.M)))
print("controller-gen prerequisite:", controller_target.group(1).strip())
print("binary rule prerequisite:", controller_target.group(2).strip())
print("installer freshness guard:", install_guard.group(0))
print("version is not a Make prerequisite:", "CONTROLLER_TOOLS_VERSION" not in controller_target.group(2))
print("installer recipe is on binary target:", bool(re.search(
r"^\$\(CONTROLLER_GEN\):[^\n]*\n\s+\$\(call go-install-tool",
text, re.M
)))
PYRepository: codeready-toolchain/api
Length of output: 455
Make CONTROLLER_TOOLS_VERSION affect target freshness.
If CONTROLLER_TOOLS_VERSION changes, Make can treat the existing symlink as current and skip installation. Generation can then use the old controller-gen binary. Move the installer recipe to the phony controller-gen target, or add the versioned binary to the target graph.
🤖 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 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.
Source: Path instructions
| 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
| The API types are defined in the `api/v1alpha1/*_types.go` files. After modifying these files, you must regenerate the derived files (deepcopy, OpenAPI, CRD manifests, and API reference docs) by running: | ||
|
|
||
| ```sh | ||
| make generate | ||
| ``` | ||
|
|
||
| This command runs the following steps: | ||
|
|
||
| 1. **Generate deepcopy and CRDs** — uses [controller-gen](https://github.com/kubernetes-sigs/controller-tools) to regenerate `zz_generated.deepcopy.go` and the CRD manifests in `config/crd/bases/`. | ||
| 2. **Generate OpenAPI** — uses [openapi-gen](https://github.com/kubernetes/kube-openapi) to regenerate `zz_generated.openapi.go`. | ||
| 3. **Generate API reference docs** — uses [crd-ref-docs](https://github.com/elastic/crd-ref-docs) to regenerate `api/v1alpha1/docs/apiref.adoc`. | ||
| 4. **Dispatch CRDs** — copies the generated CRD `.yaml` files to the `host-operator` and `member-operator` repositories. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document make manifests as a separate required step.
The supplied make/generate.mk contract separates generate from manifests: make generate runs controller-gen object, while make manifests generates CRDs. This README currently tells contributors that make generate regenerates CRDs, so API changes can leave config/crd/bases/ stale. Update the commands and numbered steps to reflect both targets, or make generate depend on manifests before documenting that behavior.
🧰 Tools
🪛 LanguageTool
[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)
🤖 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 `@README.md` around lines 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/.
| @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?
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!
| @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: generate-object generate-crd gen-crd-ref-docs generate-openapi dispatch-crds ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("make/generate.mk").read_text()
match = re.search(r"^dispatch-crds:\s*(.+)$", text, re.M)
assert match, "dispatch-crds target not found"
dependencies = match.group(1).split()
assert "generate-crd" in dependencies, (
"dispatch-crds must depend on generate-crd"
)
print("CRD dispatch is ordered after CRD generation.")
PYRepository: codeready-toolchain/api
Length of output: 292
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- make/generate.mk ---'
sed -n '1,180p' make/generate.mk
printf '%s\n' '--- dispatch-crds references ---'
rg -n -C 4 'dispatch-crds|generate-crd|config/crd/bases|prepare-host-operator|prepare-member-operator' Makefile make scripts .github 2>/dev/null || trueRepository: codeready-toolchain/api
Length of output: 11431
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
text = Path("make/generate.mk").read_text()
generate_crd = re.search(
r"^generate-crd:\s*(?P<deps>[^\n]+)\n(?P<body>(?:\t.*\n)+)",
text, re.M
)
dispatch = re.search(
r"^dispatch-crds:\s*(?P<deps>[^\n]+)\n(?P<body>(?:\t.*\n)+)",
text, re.M
)
assert generate_crd and dispatch, "required targets are missing"
generate_crd_deps = generate_crd.group("deps").split()
dispatch_deps = dispatch.group("deps").split()
generate_crd_body = generate_crd.group("body")
dispatch_body = dispatch.group("body")
assert "config/crd/bases" in generate_crd_body
assert "config/crd/bases" in dispatch_body
assert "generate-crd" not in dispatch_deps
print("generate-crd writes config/crd/bases.")
print("dispatch-crds reads, copies, and removes files in config/crd/bases.")
print("dispatch-crds has no generate-crd prerequisite.")
print("The generate prerequisite graph does not order CRD generation before dispatch.")
PYRepository: codeready-toolchain/api
Length of output: 250
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
lines = Path("make/generate.mk").read_text().splitlines()
def target_line(name):
prefix = f"{name}:"
for i, line in enumerate(lines):
if line.startswith(prefix):
deps = line[len(prefix):].split()
body = []
for next_line in lines[i + 1:]:
if next_line and not next_line.startswith(("\t", " ")):
break
if next_line.startswith("\t"):
body.append(next_line)
return deps, "\n".join(body)
raise AssertionError(f"{name} target is missing")
generate_crd_deps, generate_crd_body = target_line("generate-crd")
dispatch_deps, dispatch_body = target_line("dispatch-crds")
assert "config/crd/bases" in generate_crd_body
assert "config/crd/bases" in dispatch_body
assert "generate-crd" not in dispatch_deps
generate_deps, _ = target_line("generate")
assert "generate-crd" in generate_deps
assert "dispatch-crds" in generate_deps
print("generate-crd writes config/crd/bases.")
print("dispatch-crds reads, copies, and removes files in config/crd/bases.")
print("generate lists both targets as independent prerequisites.")
print("dispatch-crds has no generate-crd prerequisite.")
PYRepository: codeready-toolchain/api
Length of output: 373
Serialize CRD dispatch after CRD generation.
When make -j generate runs, generate-crd and dispatch-crds can run concurrently. This can copy stale or incomplete CRDs and remove files while generation is active.
Proposed fix
-dispatch-crds: prepare-host-operator prepare-member-operator
+dispatch-crds: generate-crd prepare-host-operator prepare-member-operator🤖 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 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.
Source: Path instructions
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
Developer Tooling
Generated Artifacts