Skip to content

Conversation

@erikgb
Copy link
Member

@erikgb erikgb commented Nov 1, 2025

This is a (slightly modified) cherry-pick of #312, which was later reverted in #328. The missing piece was setting the custom-order setting to true, ref. https://golangci-lint.run/docs/formatters/configuration/#gci.

I have tested this again with golangci-lint 2.6.0, and it works now. And it seems like the golangci-lint run --fix proposed in #467 requires us to use localmodule instead of prefix to do the right thing.

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 1, 2025
@erikgb erikgb requested a review from Copilot November 1, 2025 20:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR simplifies the golangci-lint configuration generation by removing the template substitution mechanism and adopting gci's built-in localmodule section instead of a dynamically generated prefix() section.

Key changes:

  • Removed the yq substitution step that replaced {{REPO-NAME}} placeholder with the actual repository name
  • Replaced the custom prefix({{REPO-NAME}}) section with the standardized localmodule section in the gci formatter configuration

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
modules/go/01_mod.mk Removed the yq command that performed template substitution for REPO-NAME placeholder
modules/go/.golangci.override.yaml Replaced custom prefix-based import grouping with the built-in localmodule section

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@erikgb erikgb requested a review from inteon November 1, 2025 20:06
@erikgb erikgb changed the title Simplify GCI golangci-lint config with localmodule WIP: Simplify GCI golangci-lint config with localmodule Nov 1, 2025
@cert-manager-prow cert-manager-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2025
@erikgb erikgb changed the title WIP: Simplify GCI golangci-lint config with localmodule Simplify GCI golangci-lint config with localmodule Nov 1, 2025
@cert-manager-prow cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2025
@erikgb erikgb requested a review from Copilot November 1, 2025 20:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@inteon
Copy link
Member

inteon commented Nov 4, 2025

/approve
/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2025
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2025
@cert-manager-prow cert-manager-prow bot merged commit 2121d6b into cert-manager:main Nov 4, 2025
11 checks passed
@inteon
Copy link
Member

inteon commented Nov 4, 2025

@erikgb this results in a LARGE diff in the cert-manager/cert-manager repo.

@erikgb
Copy link
Member Author

erikgb commented Nov 4, 2025

@erikgb this results in a LARGE diff in the cert-manager/cert-manager repo.

Ok, that's unfortunate. According to my testing, this should not be the case. Let me take a look.

@erikgb
Copy link
Member Author

erikgb commented Nov 4, 2025

@erikgb this results in a LARGE diff in the cert-manager/cert-manager repo.

Ok, that's unfortunate. According to my testing, this should not be the case. Let me take a look.

I notice a diff, but only one diff in the main module of cert-manager, where localmodule now takes precedence over dot. The diff in the dedicated Go modules like cmd and test modules appears correct to me, as localmodule and prefix(github.com/cert-manager/cert-manager) don't always mean the same.

Should we revert this, @inteon? I would prefer keeping it, but I am also OK with a revert.

@erikgb
Copy link
Member Author

erikgb commented Nov 5, 2025

This was discussed in today's stand-up, where @SgtCoDFish, @ThatsMrTalbot, and @mladen-rusev-cyberark were present.

I want to know if we should roll forward or roll back this? Changing the import order to avoid the "templating" of the golangci-lint config is a one-time change, which can now be automated with the improved fix-golangci-lint makefile-modules target (ref. #467).

We discussed this a bit, and none raised concern with a one-time change of the imports order/grouping. I will create a PR to "manually self-upgrade" in cert-manager. If this PR is approved and merged, we will move forward with this change. If not, I will ensure that we revert this change. CC: @inteon

@inteon
Copy link
Member

inteon commented Nov 5, 2025

This was discussed in today's stand-up, where @SgtCoDFish, @ThatsMrTalbot, and @mladen-rusev-cyberark were present.

I want to know if we should roll forward or roll back this? Changing the import order to avoid the "templating" of the golangci-lint config is a one-time change, which can now be automated with the improved fix-golangci-lint makefile-modules target (ref. #467).

We discussed this a bit, and none raised concern with a one-time change of the imports order/grouping. I will create a PR to "manually self-upgrade" in cert-manager. If this PR is approved and merged, we will move forward with this change. If not, I will ensure that we revert this change. CC: @inteon

Great @erikgb, I would like to look at the cert-manager changes in more detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants