Issue 2428: Enforce comments on the codebase

[Algebra8]

What type of PR is this?

/kind support

What this PR does / why we need it:

This PR enforces comments on the codebase via revive.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2428

Special notes for your reviewer:

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

Release note:

Enforced comments on the codebase using Revive (swapped with Golint), and resolved all linter errors.

The following linter errors were resolved:
* Resolve goimports errors
* Resolve deadcode errors by removing unused code
* Resolve misspell error by correcting spelling
* Resolve staticcheck errors
* Staticcheck linter was complaining about fmt.Printf being used with dynamic
first argument and no other arguments (SA1006). This can lead to unexpected
output. To solve this fmt.Printf was switched with fmt.Println.
Also, pkg/cloudtest/cloudtest.go V(level int) returned a logr.InfoLogger which
is deprecated, which Staticcheck complained about. This was swapped with the
newer logr.Logger.
* Exclude noctx error with descriptive comments
* Resolve gosimple errors

Since comments were updated, yamls had to be regenerated using `make regenerate`.

Also, the golangci exclude line "Using the variable on range scope `(tc)|(rt)|(tt)|
(test)|(testcase)|(testCase)` in function literal" was removed because it was
blocking what seemed to be a larger net of linters than it was supposed to, and
because it became redundant after the linter issues were resolved.

[k8s-ci-robot]

Welcome @Algebra8!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @Algebra8. 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.

[Algebra8]

@sedefsavas

Here is the WIP of #2428

Some things to note:

• For the PR message I said this PR is /kind support but I'm not sure if that's correct.
• I've swapped golint for revive and will change it back if revive is not wanted.
• exclude-use-default is set to false because otherwise it will block some of the linter warnings we want for comments.
• The line Using the variable on range scope `(tc)|(rt)|(tt)|(test)|(testcase)|(testCase)` in function literal in exclude prevents some/a lot of the other linter warnings from propagating so I've removed it for now until a solution can be found.
• If golangci-lint is not run with the --fast=false flag then revive and some other linters will not be enabled, so during development I've been using the flag. Is the flag also used when running the pipeline?
• The combination of removing the exclude line, setting fast to false, and adding exclude-use-defaults: false has caused other linter warnings to propagate. I am not sure what to do with them (enable/disable). Towards the end of this PR I will get a list of them and we can figure out what to do then?

I look forward to hearing any feedback 😄

Thanks!

[richardcase]

/ok-to-test

[Algebra8]

Question:

• Should I squash my commits?

Somethings worth noting:

• I swapped Golint with Revive
• I added linters-settings for revive so that only exported is checked (since that is the scope of this PR)
• This line in .golangci.yml blocks some linters from working (maybe it is too broad?). You can test this yourself by placing an exported function with no comments somewhere in the codebase (e.g. api/), and running golangci-lint run --fast=false api/..., once with the line commented out and once without. I've commented it out until a solution has been reached.

[Algebra8]

It seems like the pipeline for pull-cluster-api-provider-aws-test is breaking:

level=error msg="Running error: no such linter revive, run 'golangci-lint linters' to see the list of supported linters"

I was under the impression that Revive is offered out of the box with golangci-lint?

[richardcase]

* Should I squash my commits?

@Algebra8 - yes please.

[Algebra8]

With regards to pull-cluster-api-provider-aws-test failing as mentioned above, it seems like the issue is that golangci-lint doesn't include Revive as a linter until v1.37.0. The release version being used in this repo is v1.32.0.

I think the two most obvious solutions to this issue are 1_ Update golangci-lint to the most recent version (v1.40.1), 2_ Use Golint instead of Revive (I believe Golint is deprecated in up-to-date versions).

Another option may be to externally install revive but I'm not at this moment sure of how or if that would work.

Any suggestions?

[sedefsavas]

@Algebra8 This PR bumps the lint version #2485 to v1.40.1, so we are good to use revive.

[Algebra8]

A few notes on recent commits:

The following commits are related to removing linter warnings:

• d26caa7
• 49bb3f0
• 41e5d98
• 07cd52d
• eff71e5
• 2c5c91a

An important one to note is Exclude noctx error with descriptive comments, because it simply excludes the warning and I'm not sure if that's what we want. However, I believe the solution falls outside the scope of the PR because the Go versions that cluster-api-provider-aws supports matter. See here for more info. (I am more than happy to create an issue and another PR for this).

Finally, commit 2083cfd should resolve the breaking pipeline for pull-cluster-api-provider-aws-verify by re-generating generated yaml files using make generate.

@sedefsavas
I have not squashed these commits yet because I want to make sure they are approved first, so I look forward to any feedback.

[Algebra8]

@CecileRobertMichon

Following the instructions of the k8s-ci-robot and tagging you!

Apologies for the large PR, but I was told beforehand that this PR was expected to be big.

However, I included some things that might fall out of the domain of the PR (please see #2443 (comment)). I would be more than happy to roll back the commit history and open up separate PRs if it was a mistake to include them (I included them because I assumed pull-cluster-api-provider-aws-test pipeline would break otherwise).

I look forward to your feedback and will assign you to this PR (as per the requests of k8s-ci-robot).

/assign @CecileRobertMichon

[CecileRobertMichon]

/unassign
/assign @sedefsavas @richardcase

[sedefsavas]

Can these be removed?

[Algebra8]

I forgot about that commented out line, thank you for bringing that to my attention.

I will set the PR as WIP again and try to figure out what that line did. As far as I can tell, that line was casting a wide net of linter exclusions - which I assumed was not desired. However, since all linters are passing now it should be ok to remove.

[sedefsavas]

Minor comment otherwise lgtm

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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:

• OWNERS [sedefsavas]

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

[sedefsavas]

Thanks @Algebra8
/lgtm