🌱 Improve Go formatting in CAPI codebase

[abhay-krishna]

Before opening #8240, I ran the gofumpt tool as I do on all my Go changes, which made the following formatting fixes. Opened a separate PR for these changes in favor of separation of concerns.

[k8s-ci-robot]

Hi @abhay-krishna. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign killianmuldoon for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[abhay-krishna]

/retitle 🌱 Improve Go formatting in CAPI codebase

[k8s-ci-robot]

@abhay-krishna: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle 🌱 Improve Go formatting in CAPI codebase

Instructions 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/test-infra repository.

[sbueringer]

/retest

[abhay-krishna]

/retest

[k8s-ci-robot]

@abhay-krishna: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions 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/test-infra repository.

[abhay-krishna]

I'm not able to reproduce the diff in the pull-cluster-api-verify-main presubmit. It expects the generated file to be the older unformatted code. I'm also sure how the jobs ran without an /ok-to-test comment. AFAIK the retest comment should only be re-running failed jobs but it seems to have triggered the jobs themselves...

[killianmuldoon]

Thanks for proposing this @abhay-krishna. I think if we want to include this we should at the least also use a linter - like we do for gofmt - to ensure the code stays to these standards.

I'm going to go through the actual changes made by the code, but the list of rules is at https://github.com/mvdan/gofumpt#added-rules

/ok-to-test

[killianmuldoon]

I'm not able to reproduce the diff in the pull-cluster-api-verify-main presubmit

You should be able to fix this by running make generate locally if I understand right. After quickly looking through the changes I think this is an improvement - what do you think about adding gofumpt to the CAPI .golangci.yml? This would integrate the fixes here with the existing lint flow. It also supports autofix to fix most things automatically.

Ref: https://golangci-lint.run/usage/linters/#gofumpt

[abhay-krishna]

Thanks for the feedback @killianmuldoon! Yes, I intended to follow up with adding it to CI after getting initial feedback about this PR. I was going to suggest either adding it to golangci (so that it's covered in CI) and/or run gofumpt as a (pre-commit hook) [https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks] in Git (so that gofumpt is automatically run on every code change even if author might forget). We already use gofumpt in our golangci over on the EKS Anywhere repo so i can confirm it is fully supported and go ahead and add it in CAPI too.

Interestingly enough, I ran make generate several times on a clean env but never got any net new diff, so I'm wondering what I'm missing.

[killianmuldoon]

Interesting - when I run make generate on my env with this PR checked out I get the following:

        modified:   api/v1beta1/zz_generated.openapi.go
        modified:   exp/runtime/hooks/api/v1alpha1/zz_generated.openapi.go

These from the make generate-go-openapi target. Can you check if that runs to completion for this PR on your env? Could you have any custom gitignore rules or anything else that could be making a difference to git's understanding of the diff? What system are you trying to run this on (I'm on linux/amd64).

[sbueringer]

Adding it to golangci-lint probably works around the issue as make lint-fix should ignore the generated files (as they are on the ignore list of golangci-lint).

Is there any way we can break this PR down to apply the fixes incrementally (by finding)? Would make it easier to decide if we agree with the opinions of gofumpt and also make the review flow easier (currently the PR touches a lot of files and it becames more fun when we have to rebase)

[abhay-krishna]

Adding it to golangci-lint probably works around the issue as make lint-fix should ignore the generated files (as they are on the ignore list of golangci-lint).

@sbueringer Adding it to golangci did avoid linting the generated files, thanks!

Is there any way we can break this PR down to apply the fixes incrementally (by finding)?

I would like to, but I am not sure what incrementally means in this context. When I run make lint, I see only gofumpt issues and make lint-fix changes it to the format you see in this PR.

[killianmuldoon]

Unfortunately gofumpt doesn't seem to have a way to enable only a subset of its rules so I'm not sure how we would do this incrementally without making it a lot of work for everyone.

[abhay-krishna]

/retest

[fabriziopandini]

I suggest putting this on hold until the next release is cut.

Also, on top of the concern about the size of the PR, let's consider that enforcing formatting rules on top of what the go community defines as standard introduces a sort of friction for contributors not used to those rules, so It would be better if we can pick them selectively/after consider the value each rules brings in vs buy in all of them in one shot

[abhay-krishna]

I suggest putting this on hold until the next release is cut.

Also, on top of the concern about the size of the PR, let's consider that enforcing formatting rules on top of what the go community defines as standard introduces a sort of friction for contributors not used to those rules, so It would be better if we can pick them selectively/after consider the value each rules brings in vs buy in all of them in one shot

Apologies for the size, I thought having all the relevant changes within a single PR would reduce churn and context switching for reviewers, and for the most part, the changes in this PR can be categorized into similar groups such as coalescing variable declaration blocks, newline additions between successive functions, short variable instantiations, etc, so I hope these changes would be less tedious to review than actual code changes.

It's true that gofumpt does enforce a stricter set of formatting rules compared to gofmt but I don't believe these changes transcend the formatting decisions that any Go developer could make or are covered in the Effective Go guide; it's a widely used tool for Go developers, also supported out of the box by IDEs like VS Code. It just takes gofmt as a baseline and applies these rules on top of it, while being backwards compatible. Drilling into technicalities, both gofumpt and gofmt have a direct dependency on the go/printer library that implements the canonical Go formatting. The gofumpt project was written with the idea of porting some or many of its features to go/printer so that they may become the source of truth for gofmt as well.

I do understand the friction that this could cause because, as Effective Go puts it,

Formatting issues are the most contentious but the least consequential.

But in favor of making the code more readable and keeping formatting consistent across all developers, I think we should enforce it in CI.

[sbueringer]

Seems like the way forward is to go through the documented rules here: https://github.com/mvdan/gofumpt#added-rules and check if we agree with all of them.

It was nice in case of other linters where we could just opt-in / opt-out if we didn't agree.

IIRC I had a good experience with gofumpt in the past, but it has been a while

[abhay-krishna]

Seems like the way forward is to go through the documented rules here: https://github.com/mvdan/gofumpt#added-rules and check if we agree with all of them.

It was nice in case of other linters where we could just opt-in / opt-out if we didn't agree.

IIRC I had a good experience with gofumpt in the past, but it has been a while

That makes sense.

For the context of this PR, the changes come under the following rules:

• No empty lines before a simple error check.
• No empty lines around a lone statement (or comment) in a block.
• Composite literals should use newlines consistently.
• Multiline top-level declarations must be separated by empty lines.
• Single var declarations should not be grouped with parentheses.
• Contiguous top-level declarations should be grouped together.
• Simple var-declaration statements should use short assignments.
• Octal integer literals should use the 0o prefix on modules using Go 1.13 and later.
• Comments which aren't Go directives should start with a whitespace.
• Composite literals should not have leading or trailing empty lines.
• Adjacent parameters with the same type should be grouped together.

Most or all of these guidelines seem natural and reasonable to me since they fall under the formatting/linting practices that I (and others in my team) have been implementing, albeit manually from the start, later enforced by gofumpt. But I'll let the reviewers take their pick here.

[killianmuldoon]

I suggest putting this on hold until the next release is cut.

I think this makes sense - we should hold off merging this until after the 1.4 release ~March 28th. It shouldn't have much impact on this PR as I think the changes are, for the most part, automated.

With regard to the rules - they seem to make sense, but I need to do a full review of this - especially given #8289 (comment)

[abhay-krishna]

I suggest putting this on hold until the next release is cut.

I think this makes sense - we should hold off merging this until after the 1.4 release ~March 28th. It shouldn't have much impact on this PR as I think the changes are, for the most part, automated.

With regard to the rules - they seem to make sense, but I need to do a full review of this - especially given #8289 (comment)

I'm cool with that. Until then I'll take care of rebasing it and fixing merge conflicts from time to time.

[abhay-krishna]

I think this makes sense - we should hold off merging this until after the 1.4 release ~March 28th

@killianmuldoon This is ready for another review, thanks!

[killianmuldoon]

Thanks for the bump! It will be a few days before I get around to this - I'll try to organise it so we have a couple of reviewers to do a short loop so this doesn't end up in rebase hell 😄

[abhay-krishna]

Thanks for the bump! It will be a few days before I get around to this - I'll try to organise it so we have a couple of reviewers to do a short loop so this doesn't end up in rebase hell 😄

Thank you!! It hasn't been too bad, but I understand and appreciate your concern.

[k8s-ci-robot]

PR needs rebase.

Instructions 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/test-infra repository.

[sbueringer]

I think let's do the following.

Let's get some basic agreement from a few folks that the rules described here are fine: https://github.com/mvdan/gofumpt#added-rules

Once we have that, let's rebase the PR and do a quick round of review by 1-2 folks to make sure there are no suprises and then merge it.

I personally just took a look and I'm fine with the rules gofumpt is enforcing. I think it makes the code more readable. It also pretty much matches my personal style, probably because I used gofumpt for a few years in the past :). I also think a few more rules are not a problem for new contributors as like today they can just run make lint-fix to apply the rules (or run linters via their IDE).

We had some discussions in the past about (too many) linters, let's see what other folks are saying.

@CecileRobertMichon @enxebre @fabriziopandini @killianmuldoon @vincepri Any strong opinions either way?

[killianmuldoon]

I personally just took a look and I'm fine with the rules gofumpt is enforcing. I think it makes the code more readable. It also pretty much matches my personal style, probably because I used gofumpt for a few years in the past :). I also think a few more rules are not a problem for new contributors as like today they can just run make lint-fix to apply the rules (or run linters via their IDE).

Agreed with all of this - I think gofumpt enforces a number of things that we tend to pick out in reviews - e.g. new lines. I'm +1 for including it.

[fabriziopandini]

I'm 0 (neutral) on this PR; as I said in #8289 we should consider the toil for new contributors and it is not ideal to enable a block of controls without being able to pick them selectively/increamentally
But I'm ok with what the consensus will decide

[vincepri]

Closing due to inactivity. For posterity, we generally don't consider useful PRs that change just the style of the code which can be very subjective.

/close

[k8s-ci-robot]

@vincepri: Closed this PR.

In response to this:

Closing due to inactivity. For posterity, we generally don't consider useful PRs that change just the style of the code which can be very subjective.

/close

Instructions 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/test-infra repository.