Update go to 1.26#4
Conversation
Export the entire nixpkgs-unstable rather than just its go package so that our nix setup can use arbitrary packages from unstable as needed. Matches the change in crossplane/crossplane commit 744190edf. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Upcoming developer experience changes require golang 1.26. Do the update separately and resolve new lint issues so that we don't mix the changes into the bigger DevEx PR. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
📝 WalkthroughWalkthroughThis PR systematically migrates pointer construction from the ChangesPointer Helper Migration
Code Cleanup & Tooling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/crossplane/render/xr/cmd.go (1)
273-281: ⚡ Quick winConsider adding a deprecation warning when
--extra-resourcesis usedThanks for simplifying this merge path — the direct append is clearer. Could we also print a one-time warning when
c.ExtraResources != ""to guide users to--required-resources? That keeps behavior backward-compatible while improving migration UX.Suggested tweak
if c.ExtraResources != "" { + log.Info("Flag --extra-resources is deprecated; use --required-resources instead.") ers, err := render.LoadRequiredResources(c.fs, c.ExtraResources) if err != nil { return errors.Wrapf(err, "cannot load extra resources from %q", c.ExtraResources) } // Merge extra resources into required resources. rrs = append(rrs, ers...) }As per coding guidelines: "
**/cmd/**: Review CLI commands for proper flag handling, help text, and error messages. Ensure commands follow Crossplane CLI conventions. Ask about backward compatibility and user experience."🤖 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 `@cmd/crossplane/render/xr/cmd.go` around lines 273 - 281, When c.ExtraResources != "" is used we should emit a one-time deprecation/migration warning to guide users toward --required-resources; update the block around the call to render.LoadRequiredResources in cmd.go (the code that currently appends ers to rrs) to log a clear warn message (e.g., "the --extra-resources flag is deprecated; please use --required-resources instead") before loading/merging, using the CLI logger or stderr so it appears to users; keep the existing load and append behavior to preserve backward compatibility and ensure the message is only printed once for this invocation..golangci.yml (2)
99-100: 💤 Low valueConsider naming the replacement linter in the comment.
According to the golangci-lint docs,
gomodguardhas a "New major version" and the recommendation is to "Usegomodguard_v2instead." Naminggomodguard_v2explicitly in the comment makes it clearer to future contributors what linter is providing the coverage:✏️ Suggested comment update
- # Deprecated and replaced by a new version, but still on by default. - - gomodguard + # Deprecated and replaced by gomodguard_v2, which is active via `default: all`. + - gomodguardThat said, is
gomodguard_v2actually being relied on here, or is module-dependency enforcement handled entirely bydepguard(which already has rules configured in thesettingsblock)? Ifgomodguard_v2isn't needed at all, it might be worth disabling it too rather than silently inheriting it.🤖 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 @.golangci.yml around lines 99 - 100, Update the comment that mentions the deprecated linter `gomodguard` to explicitly reference the replacement `gomodguard_v2`, and then decide whether to keep or disable it: inspect the repository's lint configuration for `depguard` rules (the `settings` block) and if `depguard` fully covers module-dependency enforcement, remove or disable `gomodguard`/`gomodguard_v2` so it isn't silently inherited; otherwise, leave `gomodguard_v2` enabled and update the comment to read something like "Deprecated and replaced by `gomodguard_v2`" to make the replacement explicit.
99-100: ⚡ Quick winUpdate the comment to name the replacement linter explicitly.
The rationale comment is helpful, but naming the replacement linter would make it clearer for future contributors.
gomodguardis deprecated and replaced bygomodguard_v2(deprecated since v2.12.0). Consider updating the comment to:- # Deprecated and replaced by a new version, but still on by default. + # Deprecated; use gomodguard_v2 instead. Still enabled by default. - gomodguardThis gives contributors immediate context about which linter supersedes it and where to look if they need to adjust configuration.
🤖 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 @.golangci.yml around lines 99 - 100, Update the explanatory comment above the disabled linter entry for gomodguard to explicitly name its replacement gomodguard_v2; locate the entry where "- gomodguard" is listed in .golangci.yml and change the comment text to mention that gomodguard is deprecated and replaced by gomodguard_v2 (deprecated since v2.12.0) so future contributors know which linter supersedes it and where to look for configuration changes.
🤖 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.
Nitpick comments:
In @.golangci.yml:
- Around line 99-100: Update the comment that mentions the deprecated linter
`gomodguard` to explicitly reference the replacement `gomodguard_v2`, and then
decide whether to keep or disable it: inspect the repository's lint
configuration for `depguard` rules (the `settings` block) and if `depguard`
fully covers module-dependency enforcement, remove or disable
`gomodguard`/`gomodguard_v2` so it isn't silently inherited; otherwise, leave
`gomodguard_v2` enabled and update the comment to read something like
"Deprecated and replaced by `gomodguard_v2`" to make the replacement explicit.
- Around line 99-100: Update the explanatory comment above the disabled linter
entry for gomodguard to explicitly name its replacement gomodguard_v2; locate
the entry where "- gomodguard" is listed in .golangci.yml and change the comment
text to mention that gomodguard is deprecated and replaced by gomodguard_v2
(deprecated since v2.12.0) so future contributors know which linter supersedes
it and where to look for configuration changes.
In `@cmd/crossplane/render/xr/cmd.go`:
- Around line 273-281: When c.ExtraResources != "" is used we should emit a
one-time deprecation/migration warning to guide users toward
--required-resources; update the block around the call to
render.LoadRequiredResources in cmd.go (the code that currently appends ers to
rrs) to log a clear warn message (e.g., "the --extra-resources flag is
deprecated; please use --required-resources instead") before loading/merging,
using the CLI logger or stderr so it appears to users; keep the existing load
and append behavior to preserve backward compatibility and ensure the message is
only printed once for this invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a87864dc-1dbf-4d3e-a249-61fa45f93911
⛔ Files ignored due to path filters (5)
go.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonenix/apps.nixis excluded by none and included by nonenix/build.nixis excluded by none and included by nonenix/checks.nixis excluded by none and included by none
📒 Files selected for processing (15)
.golangci.ymlcmd/crossplane/common/package.gocmd/crossplane/common/resource/xpkg/client_test.gocmd/crossplane/render/op/load.gocmd/crossplane/render/op/load_test.gocmd/crossplane/render/xr/cmd.gocmd/crossplane/trace/internal/printer/default.gocmd/crossplane/validate/image.gocmd/crossplane/validate/unknown_fields.gocmd/crossplane/validate/validate_test.gocmd/crossplane/xpkg/build.gocmd/crossplane/xpkg/init.gocmd/crossplane/xpkg/template_funcs.goflake.nixinternal/docker/docker.go
Description of your changes
Upcoming developer experience changes require golang 1.26. Do the update separately and resolve new lint issues so that we don't mix the changes into the bigger DevEx PR.
We have to get Go 1.26 and the relevant golangci-lint version from nixpkgs-unstable. To simplify this, export the entire nixpkgs-unstable rather than just its go package so that our nix setup can use arbitrary packages from unstable as needed. This matches the change in crossplane/crossplane commit 744190edf.
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.