✨ Remove spaces from machine-readable comments

[Adirio]

Description

Machine-readable comments used for markers have spaces after the comment character (// or #), they have been removed to follow common coding practices.

Closes: #1384

Comments to reviewers

131/144 files are from the testdata directory, this PR is smaller than what it looks like by the number of files changed.

[camilamacedo86]

Hi @Adirio,

It is not passing in the CI. Could you please check this one for we get it merged?
Shows that it is missing run make generate only. Otherwise, the changes looks, great 👍

looking for to check it passing in the CI fr we are able to merge this one.

[Adirio]

I think it is a rebase what it is needed. Will try it.

[estroz]

Something like this can be done for any plugin, stable/frozen or not, since machine-readable comments with spaces are backwards compatible.

[camilamacedo86]

Hi @Adirio

What was your motivation to define that #1384 (this change) would be a breaking change and that should be done only for v3? If it is backwards compatible with v2 then I agree with @estroz in #1868 (review).

However, i imagine that you saw some scenario that would be affected. so, could you please clarify?

[Adirio]

If we apply this to v2, projects scaffolded with v2 before applyign this change will have spaces and further update operation by kubebuilder will not be able to find the placeholders. For example the ones in mian.go to register the controllers in the manager.

[estroz]

further update operation by kubebuilder will not be able to find the placeholders

If that’s true, it sounds like a bug in controller-tools’ comment lexer. Both forms are valid marker comments so should be parsed in the same manner.

[Adirio]

I mean the comments that are meant for kubebuilder itself, for example the MainUpdater inserts code fragments to register new controllers. Those fragments are inserted just before a machine-readable comment and it does use exact match.

[estroz]

What about creating a method to compare marker comments like so

func (m Marker) EqualsLine(line string) bool {
	line = strings.TrimSpace(strings.TrimPrefix(m.comment))
	return line == m.value
}

and use it here:

		line := scanner.Text()
		for marker, codeFragments := range codeFragmentsMap {
-			if strings.TrimSpace(line) == strings.TrimSpace(marker.String()) {
+			if marker.EqualsLine(line) {
				...
			}
		}

[estroz]

The above is just a suggestion btw. Your solution also works. Feel free to unhold to merge as-is.

/lgtm
/hold

[Adirio]

/unhold

@estroz I liked your suggestion, removed the hold as the lgtm label is required again

[camilamacedo86]

/retest

[camilamacedo86]

Hi @Adirio,

it is not passing in the CI. Maybe it is missing a rebase?

[Adirio]

I don't know why but seems that one of the changes in *_webhook.go files seems to be breaking the webhooks. @estroz do you know if this should be considered a parsing error from the side of controller-tools?

Edit: Nevermind, it doesn't have to do with that. Trying to debug it further.

[Adirio]

The part that is failing is this one:

kubebuilder/testdata/project-v3/api/v1/webhook_suite_test.go

Lines 126 to 136 in d501fc3

	// wait for the webhook server to get ready
	dialer := &net.Dialer{Timeout: time.Second}
	addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
	Eventually(func() error {
	conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
	if err != nil {
	return err
	}
	conn.Close()
	return nil
	}).Should(Succeed())

I'm not modifying anything that should break that check.

[prafull01]

I have looked into the code and the reason for this failure is following scaffolding code is not generated by kubebuilder:

err = (&Captain{}).SetupWebhookWithManager(mgr)
	Expect(err).NotTo(HaveOccurred())

Attaching the screenshot of failure and missing piece of code from scaffold code.

[estroz]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, estroz, prafull01

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

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