Decouple framework/ into a standalone Go module#33
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:
📝 WalkthroughWalkthroughCI and release workflows switch to root and framework wrapper targets. The root Makefile becomes root-module only, while Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
=======================================
Coverage ? 36.16%
=======================================
Files ? 111
Lines ? 6058
Branches ? 0
=======================================
Hits ? 2191
Misses ? 3702
Partials ? 165 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yaml:
- Around line 23-32: The CI workflow still uses mutable action tags, so update
every referenced GitHub Action in the workflow to a pinned full commit SHA
instead of version tags. Fix this in the workflow jobs that use
actions/checkout, actions/setup-go, golangci/golangci-lint-action in both lint
steps, and codecov/codecov-action so each uses an immutable commit reference.
Keep the existing workflow behavior and settings unchanged while replacing only
the action identifiers.
In `@Makefile`:
- Around line 66-95: Add a verify-fmt-all wrapper in the All Modules section of
the Makefile so CI can run formatting verification across both root and
framework modules. Use the existing verify-fmt target as the root prerequisite,
and mirror the pattern used by verify-tidy-all and verify-generate-all by
invoking the framework Makefile’s verify-fmt target from the new wrapper.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 20a2a02e-47d6-4b29-9c61-74c792b78c9e
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (10)
.github/workflows/ci.yaml.github/workflows/release.yamlMakefileframework/Makefileframework/api/types.goframework/api/zz_generated.deepcopy.goframework/controller/actions/render/kustomize/action_render_manifests_test.goframework/controller/reconciler/reconciler.goframework/controller/reconciler/reconciler_actions_test.goframework/go.mod
💤 Files with no reviewable changes (2)
- framework/controller/reconciler/reconciler_actions_test.go
- framework/controller/actions/render/kustomize/action_render_manifests_test.go
valdar
force-pushed
the
fixIndipendentFramework
branch
from
aa655c0 to
4816079
Compare
valdar
force-pushed
the
fixIndipendentFramework
branch
from
4816079 to
f394b4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/Makefile`:
- Line 21: The Makefile recipes are expanding user-overridable variables
directly into shell commands, which allows command injection; update the
affected targets to quote or otherwise safely pass the overridable recipe
variables before invoking the shell. Focus on the commands using
$(CONTROLLER_GEN) and the other noted recipes in the Makefile, and ensure any
variables like tool paths or output files are treated as data rather than
executable shell fragments.
- Around line 25-28: The fmt target currently calls golangci-lint via PATH
instead of the resolved binary, so it can miss the formatter on clean machines.
Update the fmt recipe in Makefile to use the same $(GOLANGCI_LINT) variable used
elsewhere, and keep the existing guard so formatting only runs when the binary
is available.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cb8a6b57-1246-4bd8-b7ee-44c140f8fd5d
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (15)
.github/workflows/ci.yaml.github/workflows/release.yamlMakefileapi/common/types_test.goapi/common/validation/validate_internal_test.goapi/common/validation/validate_test.goframework/Makefileframework/api/types.goframework/api/zz_generated.deepcopy.goframework/controller/actions/render/kustomize/action_render_manifests_test.goframework/controller/reconciler/reconciler.goframework/controller/reconciler/reconciler_actions_test.goframework/go.modpkg/controller/action/action_test.gopkg/controller/precondition/precondition_test.go
💤 Files with no reviewable changes (2)
- framework/controller/reconciler/reconciler_actions_test.go
- framework/controller/actions/render/kustomize/action_render_manifests_test.go
✅ Files skipped from review due to trivial changes (2)
- framework/api/zz_generated.deepcopy.go
- api/common/validation/validate_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/release.yaml
- framework/go.mod
- Makefile
- framework/api/types.go
- framework/controller/reconciler/reconciler.go
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 `@framework/Makefile`:
- Around line 82-83: The help target in the Makefile is vulnerable because
`help` expands `$(MAKEFILE_LIST)` directly into the shell, allowing command
injection via a crafted `MAKEFILE_LIST` value. Fix the `help` recipe by avoiding
untrusted expansion in the `awk` invocation: use a fixed, trusted filename list
or safely quote/escape each entry before passing it to `awk`. Keep the change
localized to the `help` target and preserve the existing help output behavior.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 97f814c1-c241-4ff2-ab08-de890e8a3ce4
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (15)
.github/workflows/ci.yaml.github/workflows/release.yamlMakefileapi/common/types_test.goapi/common/validation/validate_internal_test.goapi/common/validation/validate_test.goframework/Makefileframework/api/types.goframework/api/zz_generated.deepcopy.goframework/controller/actions/render/kustomize/action_render_manifests_test.goframework/controller/reconciler/reconciler.goframework/controller/reconciler/reconciler_actions_test.goframework/go.modpkg/controller/action/action_test.gopkg/controller/precondition/precondition_test.go
💤 Files with no reviewable changes (2)
- framework/controller/reconciler/reconciler_actions_test.go
- framework/controller/actions/render/kustomize/action_render_manifests_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/controller/precondition/precondition_test.go
- framework/api/zz_generated.deepcopy.go
- api/common/validation/validate_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/release.yaml
- .github/workflows/ci.yaml
- api/common/validation/validate_test.go
- api/common/types_test.go
- pkg/controller/action/action_test.go
- Makefile
- framework/controller/reconciler/reconciler.go
- framework/go.mod
- framework/api/types.go
valdar
force-pushed
the
fixIndipendentFramework
branch
from
f394b4e to
d26aba1
Compare
- Replace root-module type aliases with native definitions in framework/api - Remove root-module dependency and replace directive from framework/go.mod - Add dedicated framework/Makefile with generate/lint/test/verify targets - Remove inline `cd framework` from root Makefile; add *-all delegation targets - Update CI workflows to cover both modules independently
valdar
force-pushed
the
fixIndipendentFramework
branch
from
d26aba1 to
3b4444d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/Makefile (1)
67-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCWE-78: untrusted Make variables still reach the shell.
Severity: Major.
GOLANGCI_LINT_VERSION,CONTROLLER_GEN_VERSION, andMAKEFILE_LISTare user-overridable, somake golangci-lint GOLANGCI_LINT_VERSION='v2.5.0; <payload>'ormake help MAKEFILE_LIST='Makefile; <payload>'turns these targets into arbitrary command execution in CI or developer shells.Remediation
ifeq ($(GOLANGCI_LINT),) - go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) + go install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)" $(eval GOLANGCI_LINT = $(shell go env GOPATH)/bin/golangci-lint) endif @@ ifeq ($(CONTROLLER_GEN),) - go install sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION) + go install "sigs.k8s.io/controller-tools/cmd/controller-gen@$(CONTROLLER_GEN_VERSION)" $(eval CONTROLLER_GEN = $(shell go env GOPATH)/bin/controller-gen) endif @@ help: ## Display this help. - `@awk` 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_0-9-]+:.*?##/ { printf " \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) + `@awk` 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_0-9-]+:.*?##/ { printf " \033[36m%-15s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' MakefileAs per path instructions, "MAKEFILE SECURITY: ... Quote shell variables in targets".
Also applies to: 74-75, 81-83
🤖 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 `@framework/Makefile` around lines 67 - 68, The Makefile targets that use GOLANGCI_LINT_VERSION, CONTROLLER_GEN_VERSION, and MAKEFILE_LIST are passing user-overridable values into shell commands unsafely. Update the golangci-lint, controller-gen, and help target recipes to quote these variables when interpolating them into command invocations, and keep the existing target logic intact so the affected symbols remain easy to locate.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 `@framework/Makefile`:
- Around line 68-69: The tool install lookup in the Makefile currently assumes
GOPATH/bin, which breaks when GOBIN is set for go install targets like
golangci-lint and controller-gen. Update the install variable setup to prefer
$(shell go env GOBIN) when it is set, and only fall back to $(shell go env
GOPATH)/bin otherwise, keeping the existing golangci-lint and controller-gen
references intact.
---
Duplicate comments:
In `@framework/Makefile`:
- Around line 67-68: The Makefile targets that use GOLANGCI_LINT_VERSION,
CONTROLLER_GEN_VERSION, and MAKEFILE_LIST are passing user-overridable values
into shell commands unsafely. Update the golangci-lint, controller-gen, and help
target recipes to quote these variables when interpolating them into command
invocations, and keep the existing target logic intact so the affected symbols
remain easy to locate.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 93549ec5-d0fa-4249-b2cc-569e02ff4a70
⛔ Files ignored due to path filters (1)
framework/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (15)
.github/workflows/ci.yaml.github/workflows/release.yamlMakefileapi/common/types_test.goapi/common/validation/validate_internal_test.goapi/common/validation/validate_test.goframework/Makefileframework/api/types.goframework/api/zz_generated.deepcopy.goframework/controller/actions/render/kustomize/action_render_manifests_test.goframework/controller/reconciler/reconciler.goframework/controller/reconciler/reconciler_actions_test.goframework/go.modpkg/controller/action/action_test.gopkg/controller/precondition/precondition_test.go
💤 Files with no reviewable changes (2)
- framework/controller/reconciler/reconciler_actions_test.go
- framework/controller/actions/render/kustomize/action_render_manifests_test.go
✅ Files skipped from review due to trivial changes (2)
- framework/api/zz_generated.deepcopy.go
- api/common/validation/validate_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/release.yaml
- .github/workflows/ci.yaml
- api/common/types_test.go
- api/common/validation/validate_test.go
- pkg/controller/precondition/precondition_test.go
- framework/controller/reconciler/reconciler.go
- pkg/controller/action/action_test.go
- Makefile
- framework/api/types.go
| go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) | ||
| $(eval GOLANGCI_LINT = $(shell go env GOPATH)/bin/golangci-lint) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does go install module@versioninstall binaries intoGOBINwhenGOBINis set, otherwise into$GOPATH/bin?
💡 Result:
Yes, the go install command installs binaries into the directory specified by the GOBIN environment variable if it is set [1][2]. If GOBIN is not set, it defaults to $GOPATH/bin, or if GOPATH is not set, it defaults to $HOME/go/bin [1][2]. This behavior applies regardless of whether you are using the module-aware version of the command (e.g., go install module@version) or installing a package within a local module context [1][2]. You can verify the specific install destination on your system at any time by running the command go list -f '{{.Target}}' [3].
Citations:
- 1: https://pkg.go.dev/cmd/go
- 2: https://go.dev/doc/code
- 3: https://go.dev/doc/tutorial/compile-install
🏁 Script executed:
sed -n '1,120p' framework/MakefileRepository: opendatahub-io/odh-platform-utilities
Length of output: 2756
Use GOBIN before GOPATH/bin for tool installs. go install ...@version writes to GOBIN when it is set, so $(shell go env GOPATH)/bin misses both golangci-lint and controller-gen in overridden environments. Use $(shell go env GOBIN) with a GOPATH/bin fallback here.
🤖 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 `@framework/Makefile` around lines 68 - 69, The tool install lookup in the
Makefile currently assumes GOPATH/bin, which breaks when GOBIN is set for go
install targets like golangci-lint and controller-gen. Update the install
variable setup to prefer $(shell go env GOBIN) when it is set, and only fall
back to $(shell go env GOPATH)/bin otherwise, keeping the existing golangci-lint
and controller-gen references intact.
|
I've noticed some of the changes remove the improvements i've identified here: opendatahub-io/opendatahub-operator#3575 (comment) , I understand the changes here are to concretely align with what already exists, so I just want to make sure this wasn't accidental. |
|
Also given the fact we now have a generated deep copy with probably should add a verification command for it, could be a follow up just pointing it out. |
It was intentional, because, without this changes, porting opendatahub-io/opendatahub-operator#3575 to use this repo would result in 166 files touched and it is simply too much as a first step. Once that pr is moved to use this repo and merged, we can do improvements in a more incremental way. |
+1 on the follow up pr |
4 participants
Description
cd frameworkfrom root Makefile; add *-all delegation targetsHow Has This Been Tested?
unit tests
Merge criteria:
Summary by CodeRabbit
test-alland improved tidy verification withverify-tidy-all.Releaseversion type to an operator-based version.