modcheck: arg parsing, custom target, downgrade intelligence#2915
modcheck: arg parsing, custom target, downgrade intelligence#29152uasimojo wants to merge 3 commits into
Conversation
The `modcheck` utility is updated to do real argument parsing. The
erstwhile `-f` option ("fix `require` discrepancies in apis/go.mod") is
now spelled `--fix` (or, apparently `-fix`, because golang) but
otherwise functions identically. As a bonus, `--help` and similar
options prints a nice usage message.
Co-Authored-By: claude
Add the ability to check different go.mod files (like test/ote/go.mod) by adding a `--gomod` option to `modcheck` whereby a path to the secondary go.mod file can be provided. This defaults to `apis/go.mod`, so the default behavior is unchanged. Co-Authored-By: claude
Previously `modcheck --fix` would blat the root go.mod's semver onto the target go.mod no matter what. With this change, it will by default refuse to downgrade the target dep. To override this default behavior and allow downgrades, a new `--allow-downgrade` option is available. To provide visual support of this new intelligence, the `XX` prefix for mismatches is now either `<<` (downgrade) or `>>` (upgrade). Co-Authored-By: claude
|
/assign @suhanime |
|
/cc @miyadav |
📝 WalkthroughWalkthroughThe module checker script ( ChangesModule Checker Flag Parsing and Reconciliation Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/modcheck.go (1)
280-280: 💤 Low valueConsider improving replacement mismatch output formatting.
The format string uses
%sto printreplacementstructs, which will output Go's default struct format like{newpath:foo newver:1.2.3}. While functional, a more user-friendly format would improve readability.Optional improvement
- fmt.Printf("%s replace %s: root(%s) target(%s)\n", indicator, path, rootrepl, targetrepl) + fmt.Printf("%s replace %s: root(%s@%s) target(%s@%s)\n", + indicator, path, rootrepl.newpath, rootrepl.newver, targetrepl.newpath, targetrepl.newver)🤖 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 `@hack/modcheck.go` at line 280, The replace mismatch print currently uses "%s" to print the replacement structs (rootrepl and targetrepl) which yields raw Go struct formatting; update the fmt.Printf call to output a clearer, user-friendly string by interpolating the struct fields (e.g., newpath and newver or whichever field names exist on the replacement type) instead of the whole struct, e.g., build a format like "%s replace %s: root(%s@%s) target(%s@%s)\n" and pass indicator, path, rootrepl.<field>, rootrepl.<field>, targetrepl.<field>, targetrepl.<field> using the actual field names from the replacement type.
🤖 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 `@hack/modcheck.go`:
- Line 74: The os.WriteFile call using opts.gomod is passing mode 0 which
creates an inaccessible file; change the file mode to a sensible default (e.g.
owner read/write and group/other read) when writing the module file so it is
readable by subsequent operations—update the os.WriteFile(opts.gomod, b, 0)
invocation to use a proper FileMode like 0644 (or a project-appropriate
permission constant) and ensure any surrounding error handling in the same
function preserves/report the error as before.
---
Nitpick comments:
In `@hack/modcheck.go`:
- Line 280: The replace mismatch print currently uses "%s" to print the
replacement structs (rootrepl and targetrepl) which yields raw Go struct
formatting; update the fmt.Printf call to output a clearer, user-friendly string
by interpolating the struct fields (e.g., newpath and newver or whichever field
names exist on the replacement type) instead of the whole struct, e.g., build a
format like "%s replace %s: root(%s@%s) target(%s@%s)\n" and pass indicator,
path, rootrepl.<field>, rootrepl.<field>, targetrepl.<field>, targetrepl.<field>
using the actual field names from the replacement type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd55bbc1-8803-46ad-a5d7-f7f4eb90c29c
📒 Files selected for processing (2)
Makefilehack/modcheck.go
| } | ||
| if err = os.WriteFile(apispath, b, 0); err != nil { | ||
| fmt.Printf("Failed to write modified %s: %s\n", apispath, err) | ||
| if err = os.WriteFile(opts.gomod, b, 0); err != nil { |
There was a problem hiding this comment.
Critical: File permissions must not be zero.
os.WriteFile is called with permissions 0, which creates a file with no read, write, or execute permissions for anyone. This will make the file inaccessible and likely cause failures in subsequent operations.
🔧 Proposed fix
- if err = os.WriteFile(opts.gomod, b, 0); err != nil {
+ if err = os.WriteFile(opts.gomod, b, 0644); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err = os.WriteFile(opts.gomod, b, 0); err != nil { | |
| if err = os.WriteFile(opts.gomod, b, 0644); err != nil { |
🤖 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 `@hack/modcheck.go` at line 74, The os.WriteFile call using opts.gomod is
passing mode 0 which creates an inaccessible file; change the file mode to a
sensible default (e.g. owner read/write and group/other read) when writing the
module file so it is readable by subsequent operations—update the
os.WriteFile(opts.gomod, b, 0) invocation to use a proper FileMode like 0644 (or
a project-appropriate permission constant) and ensure any surrounding error
handling in the same function preserves/report the error as before.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2915 +/- ##
=======================================
Coverage 50.39% 50.39%
=======================================
Files 281 281
Lines 34368 34368
=======================================
Hits 17320 17320
Misses 15696 15696
Partials 1352 1352 🚀 New features to boost your workflow:
|
|
@2uasimojo: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
hack/modcheck.go grows:
--fixreplaces-f(same functionality)--gomod path/to/go.modto designate a target other thanapis/go.mod(the default)--allow-downgradeto override and fix it anyway.Co-Authored-By: claude
Summary by CodeRabbit