🐛 makefile: remove invalid comment

[mateusoliveira43]

Why the changes were made

Fix command documentation.

How the changes were made

When make lint command was added (46ea676#diff-e5d6d4f9d67174f1f3764674eb786688353c77cef8514e24d1323e55e5a4a7e8) I believe its documentation was copied from project Makefile (feb0c54#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52) which does run yamllint, but not command generated by kubebuilder. So, removed that part from command doc.

After applying changes, ran make generate.

How to test the changes made

Confirm the documentation is correct now.

[k8s-ci-robot]

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

[mateusoliveira43]

Note that some files need the fmt command that was run. I did not replace the fmt vet directives in Makefile to lint-fix, because it does more than those 2 commands. One of those things being finding lint issues in generated projects (which causes make generate to fail)

One other approach I would suggest is adding all generated projects to this workflow c0d25b9 and fixing the lint issues

[mateusoliveira43]

@lukas016 here is the explanation about unformatted source code

[lukas016]

@mateusoliveira43 your PR contains unformatted source code. Are you sure your suggestion for delete fmt job is correct?

[lukas016]

are you sure with that? I think it was good practice call formatting for every build/test/run. Lint is optional and fix too. So you cannot forget call this targets before pushing changes otherwise you can create many changes in commits.

I personally suggest keep fmt and don't have dependency to third party software.
it can help avoid unnecessary changes in PR.

Vet can be deleted.

[lukas016]

Additional complication can be linter config. Linter config is manage by user and he can very easy disable fmt linter. This hardcoded way in makefile looks better for me.

[mateusoliveira43]

I personally suggest keep fmt and don't have dependency to third party software.
it can help avoid unnecessary changes in PR.

Agreed, will add back!

I was thinking about directly calling go fmt inside make generate, and still remove fmt command from Makefile (without changing make generate current behavior). This way, kubebuilder CI would still prevent unnecessary changes in PRs, and users would have more control over generated code.

What you think?

[lukas016]

i am not sure. you will create dependency between formatting and generating what are independent process (they depend on the code).

Is it okay, if fmt fail and generate won't run but independently it can run?
What can be weird is make target name. It is generate so i expect it will create something but fmt just modify current state.

I am not sure and i think we are doing overengineering. Did you heard some complain about current solution?
I personally don't see big benefits in vet because it is linter and it is part of golangci-lint so it can be deleted.
But other stuff i kept as it is because i don't see big benefits and it can create just new problems.

I think opinion from someelse can be helpful.

[mateusoliveira43]

My only complain is that fmt command does not have a CI counterpart, so I created one in the past (called it fmt-isuptodate) in my project Makefile. After understanding golangci-lint, I removed fmt and fmt-isuptodate and started using lint-fix and lint respectively, in the project Makefile.

What can be weird is make target name. It is generate so i expect it will create something but fmt just modify current state.

hmmm, one other view we can have then, is updating generate command to generate the code already formatted? Looking in the generated Makefile command, the two code generation commands, generate and manifests do not run fmt (but I think Go is smart enough to ignore format in generated files by controller-gen, so this can be bad example 😬 )

[lukas016]

CI counterpart. CI can be extremely different and it depends from environment and CI/CD software. I am using gitlab and i have own ci which just call make fmt in projects, so i never had problem with ci/cd. It exists a lot of CI/CD solutions and i don't think it is good idea force some ci/cd solution into this project, which is technically template for another projects. I think our main target has to be easy usage for local development.

[mateusoliveira43]

alright! I think today I will not have time, but tomorrow I will commit changes based on our conversation 😀

[mateusoliveira43]

@lukas016 just to keep you posted: I found some problems in scripts that generate docs testdata projects. I am working on fixing them. Once I finished (this week, but not sure which day), I will open a PR for that and update this one to just remove vet command (reverting removal of fmt command)

[lukas016]

LGTM

Note for thinking:
if we want to push programmers to write better code, i suggest call linter as dependency for build target.

[camilamacedo86]

Hi @mateusoliveira43

thank you for looking it.
But I do not think that we should remove the go vet check at all, see; https://github.com/kubernetes-sigs/kubebuilder/pull/3816/files#r1544217982

The only change that seems valid here would be remove & yamllint from the comment.

I hope that makes sense.

[camilamacedo86]

The lint will validate the vet as well.
But I am not sure if we want to remove this one.
Users can still only call vet if they wish using go

Either we do not call the lint to generate the manifests and etc.

So, I would say that I think is better we keep it as it is

[mateusoliveira43]

For me is a duplication and can be misleading for developers

Example: first time on a project, I run make help and see there is fmt, vet and lint commands. Do I need to run all of them?

Since kubebuilder already generates everything needed for developers to use golangci-lint instead of fmt and vet, I would enforce developers to use it

But we can discuss in a issue for example, will undo vet removal

[camilamacedo86]

The lint is a good practice and has its purpose.
however, this dev targets using go features are used for the other targets
Note that some people might do not want the linters or those then they are able to just remove from their project

However, kubebuilder has the goal to help developer and scaffold the default common/adhered used options.
So, IMO we should keep it as it is.

[mateusoliveira43]

yeah, I though about just removing on my project, but I am afraid that can break how I update kubebuilder. I did not try out kubebuilder alpha generate yet, but the way I do today is as described here https://github.com/migtools/oadp-non-admin?tab=readme-ov-file#architecture

so I try to not change what came in Makefile, just add more commands in the end of the file

made the changes

[camilamacedo86]

It probably would work but again I would recommend keep both
It has not a really good reason to remove one or another.
Each one has its purpose, more info: #3816 (review)

[camilamacedo86]

I understand the intent behind streamlining our development tools by leveraging golangci-lint in our Makefile. However, I recommend not removing go vet and go fmt for the following reasons:

• Complementary Checks: While golangci-lint is a powerful tool that lints our project against a wide range of criteria, go vet and go fmt serve as foundational checks for code correctness and formatting. These checks play a crucial role in early problem detection and code quality maintenance.

• Developer Workflow: Retaining go vet and go fmt allows developers to quickly address basic code quality issues independently of the more comprehensive linting process. This facilitates a smoother development workflow, enabling focused and efficient corrections.

• Code Quality and Consistency: The combination of these tools ensures a high standard of code quality and consistency across the project. Each tool has its strengths, and together, they cover a broader spectrum of potential issues.

In summary, while golangci-lint enhances our code quality checks, go vet and go fmt provide essential, foundational checks that should not be overlooked. Their removal could lead to gaps in our code quality assurance process. Therefore, I advocate for keeping both, as they complement each other and collectively strengthen our development practices.

[mateusoliveira43]

@camilamacedo86 can you take a look?

[camilamacedo86]

/lgtm

@mateusoliveira43

Thank you a lot !!

I just change the title because we use it in the release notes
In this case, we use 🐛 because it change the project scaffold for the users

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, lukas016, mateusoliveira43

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 [camilamacedo86]

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

[camilamacedo86]

/ok-to-test