Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 (CLI, deploy-image/v1alpha1, go/v4) Ensure consistent spacing in marker annotations #3904

Merged
merged 1 commit into from
May 24, 2024

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented May 12, 2024

This commit addresses the inconsistency in marker annotations within the kubebuilder project. Previously, kubebuilder markers did not have a space between // and +marker, unlike those in controller-tools and other related tools see;

This inconsistency was causing confusion for end users.

To resolve this, all kubebuilder markers are now updated to include a space, ensuring uniformity across the project. After merging this commit, you can run make all to regenerate the markers. If any issues arise, use find/replace to change //+ to // +.

Motivates by: #3801

c/c @mateusoliveira43

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2024
@camilamacedo86 camilamacedo86 force-pushed the space-marker branch 2 times, most recently from 9ec10d6 to 652f519 Compare May 12, 2024 15:09
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2024
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 12, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 Ensure space between comment and marker 🐛 (CLI, deploy-image/v1alpha1, go/v4) Ensure space between comment and marker May 12, 2024
@mateusoliveira43
Copy link
Contributor

@camilamacedo86 reviewed, looks good 😄

I would add comment-spacings in linters-settings.revive.rules of .golangci.yml to enforce this in the future. Adding it in top of this PR gets only this 2 errors

pkg/rescaffold/migrate.go:333:2: comment-spacings: no space between comment delimiter and comment text (revive)
	//Copy all the contents to the desitination file
	^
pkg/cli/alpha.go:38:3: comment-spacings: no space between comment delimiter and comment text (revive)
		//TODO: If we need to create alpha commands please add a new file for each command
		^

(so the primary problem was fixed!)

If you need help fixing the CI, I can take a look

@camilamacedo86

This comment was marked as outdated.

@mateusoliveira43

This comment was marked as outdated.

@camilamacedo86

This comment was marked as outdated.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 17, 2024
@camilamacedo86 camilamacedo86 force-pushed the space-marker branch 2 times, most recently from 8836636 to 6e669c6 Compare May 17, 2024 21:28
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2024
@@ -54,7 +54,7 @@ func NewMarkerFor(path string, value string) Marker {

// String implements Stringer
func (m Marker) String() string {
return m.comment + prefix + m.value
return m.comment + " " + prefix + m.value
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See that we are changing the CLI here to ensure a space always

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2024
@camilamacedo86 camilamacedo86 changed the title 🐛 (CLI, deploy-image/v1alpha1, go/v4) Ensure space between comment and marker 🐛 (CLI, deploy-image/v1alpha1, go/v4) Ensure consistent spacing in marker annotations May 18, 2024
@camilamacedo86 camilamacedo86 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2024
@camilamacedo86
Copy link
Member Author

Hi @mateusoliveira43

#3904 (comment)
That we can do in a follow up after this one get merged.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Hey @camilamacedo86 - this looks good! Just curious, looks like the space has been introduced in all the places? Is it necessary because iirc c-t does not parse both the cases ?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, varshaprasad96

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,varshaprasad96]

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

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 21, 2024

Hey @camilamacedo86 - this looks good! Just curious, looks like the space has been introduced in all the places? Is it necessary because iirc c-t does not parse both the cases ?

I think it might be a convention because the user that raised it out complains about some lints checks that raise issues related to it see:

cmd/main.go:52:2: comment-spacings: no space between comment delimiter and comment text (revive)
	//+kubebuilder:scaffold:scheme
	^

However, my main motivation is make it easier for users. It is hard to know what marker should have space or not so since all places has space lets keep the space. That was my thought.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2024
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@camilamacedo86
Copy link
Member Author

I also checked that controller-runtime either is using space for all marker comments , https://github.com/search?q=repo%3Akubernetes-sigs%2Fcontroller-runtime++%22%2F%2F+%2B%22&type=code so that seems the right way to move forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-blocker size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants