calico icon indicating copy to clipboard operation
calico copied to clipboard

Add tooling to consistently format Go imports

Open fasaxc opened this issue 1 year ago • 2 comments

Description

  • Split make fix into make fix-changed and make fix-all
    • Use fix-all in CI, it runs no slower on semaphore than the old version.
    • fix-changed should be faster when run locally.
  • make fix-changed only formats files that have changed on this branch.
    • The parent branch is guessed based on distance to the merge base with master and release branches.
    • The remote name is auto-detected from metadata.mk.
  • Remove check-fmt target in favour of running a format pass andchecking for dirty in CI (hard to do check-fmt now that two commands need to be run in sequence).

Related issues/PRs

Todos

  • [ ] Tests
  • [ ] Documentation
  • [ ] Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

fasaxc avatar Oct 14 '24 14:10 fasaxc

@fasaxc While you're in the area, I wonder if you have any thoughts about https://github.com/projectcalico/calico/pull/9277 - i.e. about why our make fix doesn't make the changes that are proposed in that PR.

nelljerram avatar Oct 17 '24 13:10 nelljerram

We use goimports, which is a superset of gofmt, but:

  • it's likely pinned to a specific version
  • perhaps we don't specify -s for "simplify".

fasaxc avatar Oct 17 '24 14:10 fasaxc