Solve name collision and document why version strings can not be const

[Adirio]

Change list:

• Avoid variable and package name collision
• Stop exporting get version function
• Document why version strings cannot be constants

/kind cleanup

[k8s-ci-robot]

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

[Adirio]

/assign @pwittrock

[camilamacedo86]

Hi @Adirio,

It is really cool to see that you have been contributed to the project 🥇.
In order to help you with the review, I added so comments. Please feel free to check it.

[Adirio]

In order to help you with the review, I added so comments. Please feel free to check it.

Done. I added some answers justifying some of the changes you asked about.

And thank you very much for the welcome.

[camilamacedo86]

Hi @Adirio,

I added some comments to clarifies my POV over it here.
Basically it shows that you have here 3 diff motivations/purposes in the same PR. So, I think it should be split like:

1. Improve version.go for a better understanding
2. Error strings should not be capitalized in order to follow the good practices
3. Make explicitly "handles" the error by not doing anything.

[Adirio]

@camilamacedo86 I answered some of your comments. The three motivations you mentioned are listed in the commit text and in the PR message, with a couple more. version.go modifications are not only aimed towards understandability: version is a package name so using it as a variable is a name collision, the other two changes, while they may increase the performance in a negligible manner, could be considered aimed towards understandability.

As the title states, it is just a cleanup, tidying up some random stuff. You may be right that some of these changes should be done for the whole project and not only the cmd package. I would like to hear other opinions about this (@droot @pwittrock @mengqiy @DirectXMan12). Expecifically, about using explicit _ for unhandled errors and using lowercase and no final dot for error messages (https://github.com/golang/go/wiki/CodeReviewComments#error-strings).

[camilamacedo86]

HI @Adirio,

It is fine. Just to clarifies the key points of my review here:

As a good practice no matter the project that you collab with you should keep a small context per PR. PR's should not address many motivations/purposes in order to keep simple and easier others understand what is going on, do the reviews, track the changes and etc..

Also, note that IMHO make no sense apply motivations like Error strings should not be capitalized in order to follow the good practices just in one file. If it should not be done then it should be changed in all repo.

Hi @droot @pwittrock @mengqiy @DirectXMan12, could you please give a hand here and let us know wdyt about these changes?

[mengqiy]

/ok-to-test

[mengqiy]

If we have cleanup all of the linter errors, it'd be great to enable the linters like controller-runtime does.
Ref: https://github.com/kubernetes-sigs/controller-runtime/blob/81842d0e78f7111f0566156189806e2801e3adf1/hack/verify.sh#L27-L45

[Adirio]

Ok, I will split this PR into the different parts following your comments on monday

[Adirio]

Simplified the PR to follow the comments and letting the formatting and error handling parts to their specific PR in a project-wide scope.

[Adirio]

If we have cleanup all of the linter errors, it'd be great to enable the linters like controller-runtime does.
Ref: https://github.com/kubernetes-sigs/controller-runtime/blob/81842d0e78f7111f0566156189806e2801e3adf1/hack/verify.sh#L27-L45

I started #1176 to track all changed that may be needed for this purpose.

[Adirio]

/test pull-kubebuilder-e2e

[mengqiy]

@Adirio Hmm, it seems it conflicts with your other PR. Can you please rebase?

[mengqiy]

Please squash the commits.

[Adirio]

I resolved the conflict in GitHub tool.

Tomorrow I will be able to do it with my normal development environment and squash the commits if you think the commit history would be cleaner, but at least with this we should be able to check that the tests pass correctly.

[mengqiy]

No rush. We can wait til tomorrow.
BTW can you update the PR description? It's a little out of date.

[camilamacedo86]

Suggested change

	if foundProject && projectVersion == project.Version1 {
	if isProjectFound && projectVersion == project.Version1 {

[camilamacedo86]

IHMO to merge this one is missing

• squash the commits
• small nit replace foundProject for isProjectFound
• update the first comment since it shows not so longer reflect the motivations of the changes performed here.

Besides the small changes above all shows fine for me 👍

[camilamacedo86]

/assign @camilamacedo86

[camilamacedo86]

/assign @mengqiy

[Adirio]

@mengqiy

No rush. We can wait til tomorrow.

👍

BTW can you update the PR description? It's a little out of date.

Done.

@camilamacedo86

• small nit replace foundProject for isProjectFound

I think we already discussed this. isProjectFound sounds really strange as it is a verb followed by a noun and then by an adjetive. If any alternative variable name was to be used hasProjectFile sounds better. The thing is that this PR is not changing version because the new name suits better, but because there is already a package called like that. So it is solving a package and variable name collision, which is a project-wide change. Yes, it is changing a variable name in the same line, but the motivation is different and I would like to enphasize that. If you want to change all boolean variable names to isXxxxx or hasXxxxx I think it makes more sense to have a separate PR do it and make it be project-wide.

[camilamacedo86]

Hi @Adirio hasProjectFile sounds better 🥇
I think it is too small to be another one and could be done here .. however.. it is fine if you didn't

[k8s-ci-robot]

@Adirio: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubebuilder-e2e	958f5ed	link	/test pull-kubebuilder-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[Adirio]

@mengqiy Done

[mengqiy]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, mengqiy

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

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