🌱 Enabled more golangci linters #1536

kashifest · 2024-02-01T11:42:24Z

These linters still were the low hanging fruits. The rest 5 linters which are currently disabled, are expected to create much more noise and code change and I plan to work on them on a followup.

kashifest · 2024-02-01T11:45:21Z

/hold
Seems some more fixing is needed

kashifest · 2024-02-02T12:45:00Z

/test metal3-bmo-e2e-test-pull

kashifest · 2024-02-02T12:46:27Z

/test-centos-e2e-integration-main

kashifest · 2024-02-02T12:50:16Z

/test metal3-bmo-e2e-test-pull

kashifest · 2024-02-02T12:25:56Z

apis/metal3.io/v1alpha1/baremetalhost_types.go

-		return ""
-	}
-	return host.Labels[name]
-}


these two fuctions are unused

kashifest · 2024-02-02T12:29:00Z

apis/metal3.io/v1alpha1/preprovisioningimage_types.go

 type PreprovisioningImageStatus struct {
 	// imageUrl is the URL from which the built image can be downloaded.
-	ImageUrl string `json:"imageUrl,omitempty"`
+	ImageUrl string `json:"imageUrl,omitempty"` //nolint:revive,stylecheck


ideally this field name should have been ImageURL as asked by the linters, I was not sure if I should do it here since this will cause an API change. I am up for suggestions.

I would suggest at least a separate PR for this if the consensus is to change .

For the time being the exclusion is justified IMO.

kashifest · 2024-02-02T12:29:18Z

apis/metal3.io/v1alpha1/preprovisioningimage_types.go


 	// kernelUrl is the URL from which the kernel of the image can be downloaded.
 	// Only makes sense for initrd images.
 	// +optional
-	KernelUrl string `json:"kernelUrl,omitempty"`
+	KernelUrl string `json:"kernelUrl,omitempty"` //nolint:revive,stylecheck


ideally this type name should have been KernelURL as asked by the linters, I was not sure if I should do it here since this will cause an API change. I am up for suggestions.

same as above

kashifest · 2024-02-02T12:31:16Z

controllers/metal3.io/action_result.go

@@ -85,7 +85,7 @@ func (r actionComplete) Dirty() bool {
 // deleteComplete is a result indicating that the deletion action has
 // completed, and that the resource has now been deleted.
 type deleteComplete struct {
-	actionComplete
+	actionComplete //nolint:unused


I am a bit unsure about this piece of code and how is it used elsewhere. Hence I suppressed the linter.

kashifest · 2024-02-02T12:37:09Z

controllers/metal3.io/bmceventsubscription_controller.go

-func (r *BMCEventSubscriptionReconciler) secretManager(log logr.Logger) secretutils.SecretManager {
-	return secretutils.NewSecretManager(log, r.Client, r.APIReader)
-}
-


unused code

kashifest · 2024-02-02T12:41:44Z

pkg/provisioner/demo/demo.go

-
-	// result.RequeueAfter = deprovisionRequeueDelay
+	p.log.Info("deprovisioning host")
+	return result, nil


I was not sure how to get rid of this default switch-case only issue, hence I am putting a log message. Open for discussion/suggestions.

I don't really get why was this a switch case to begin with...

Is there any significance of accessing the p.objectMeta.Name field, could that have some effect? (I think no but maybe there is some GO magic that I am no considering)

me neither and I am not sure if theres any go magic.

This is for the "demo" provisioner, so it was probably meant as a way for developers to play with it. You could add specific cases for what you wanted to happen to the hosts. This is also why there is commented out code below. Just a simple way to make it do different things.
That said, I think we can safely remove it, otherwise it will just rot

Yeah, I think it was just copy-and-paste.
(The purpose of the demo provisioner was to test a web UI, by allowing you to create a host that would sit in a certain state. The state it ended up in was determined by the host name.)

kashifest · 2024-02-02T12:41:58Z

pkg/provisioner/demo/demo.go

-
-	// return result, nil
+	p.log.Info("powering on host")
+	return result, nil


I was not sure how to get rid of this default case only issue, hence I am putting a log message, open for discussion.

same as above

kashifest · 2024-02-02T12:42:04Z

pkg/provisioner/demo/demo.go

-
-	// return result, nil
+	p.log.Info("powering off host")
+	return result, nil


I was not sure how to get rid of this default case only issue, hence I am putting a log message, open for discussion.

same as above

pkg/provisioner/ironic/testserver/server.go

apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go

pkg/provisioner/ironic/testserver/server.go

kashifest · 2024-02-02T13:59:58Z

/test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

kashifest · 2024-02-02T14:00:52Z

/cc @zaneb @dtantsur @honza @tuminoid @lentzi90 @Rozzii

kashifest · 2024-02-02T14:01:11Z

/hold cancel

kashifest · 2024-02-02T14:33:45Z

/test metal3-bmo-e2e-test-pull

kashifest · 2024-02-05T12:56:36Z

/test metal3-bmo-e2e-test-pull

Rozzii

just few questions and comments in general great work!

Rozzii · 2024-02-05T14:30:21Z

apis/metal3.io/v1alpha1/preprovisioningimage_types.go

 type PreprovisioningImageStatus struct {
 	// imageUrl is the URL from which the built image can be downloaded.
-	ImageUrl string `json:"imageUrl,omitempty"`
+	ImageUrl string `json:"imageUrl,omitempty"` //nolint:revive,stylecheck


I would suggest at least a separate PR for this if the consensus is to change .

Rozzii · 2024-02-05T14:30:45Z

apis/metal3.io/v1alpha1/preprovisioningimage_types.go

 type PreprovisioningImageStatus struct {
 	// imageUrl is the URL from which the built image can be downloaded.
-	ImageUrl string `json:"imageUrl,omitempty"`
+	ImageUrl string `json:"imageUrl,omitempty"` //nolint:revive,stylecheck


For the time being the exclusion is justified IMO.

Rozzii · 2024-02-05T14:30:55Z

apis/metal3.io/v1alpha1/preprovisioningimage_types.go


 	// kernelUrl is the URL from which the kernel of the image can be downloaded.
 	// Only makes sense for initrd images.
 	// +optional
-	KernelUrl string `json:"kernelUrl,omitempty"`
+	KernelUrl string `json:"kernelUrl,omitempty"` //nolint:revive,stylecheck


same as above

cmd/make-virt-host/main.go

Rozzii · 2024-02-05T14:40:34Z

pkg/provisioner/demo/demo.go

-
-	// return result, nil
+	p.log.Info("powering on host")
+	return result, nil


same as above

Rozzii · 2024-02-05T14:42:25Z

pkg/provisioner/demo/demo.go

-
-	// return result, nil
+	p.log.Info("powering off host")
+	return result, nil


same as above

pkg/provisioner/ironic/testserver/ironic.go

test/e2e/bmc.go

Rozzii · 2024-02-05T14:52:42Z

pkg/provisioner/demo/demo.go

-
-	// result.RequeueAfter = deprovisionRequeueDelay
+	p.log.Info("deprovisioning host")
+	return result, nil


Is there any significance of accessing the p.objectMeta.Name field, could that have some effect? (I think no but maybe there is some GO magic that I am no considering)

zaneb

Thanks so much for doing this!

pkg/provisioner/demo/demo.go

zaneb · 2024-02-06T23:36:03Z

pkg/provisioner/demo/demo.go

-
-	// result.RequeueAfter = deprovisionRequeueDelay
+	p.log.Info("deprovisioning host")
+	return result, nil


Yeah, I think it was just copy-and-paste.
(The purpose of the demo provisioner was to test a web UI, by allowing you to create a host that would sit in a certain state. The state it ended up in was determined by the host name.)

controllers/metal3.io/baremetalhost_controller.go

kashifest · 2024-02-07T07:13:47Z

/test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

zaneb

/approve

apis/metal3.io/v1alpha1/baremetalhost_types.go

metal3-io-bot · 2024-02-07T07:19:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zaneb

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~~ [zaneb]

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

kashifest · 2024-02-07T07:27:07Z

/test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

kashifest · 2024-02-07T07:35:20Z

/test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

Signed-off-by: Kashif Khan <kashif.khan@est.tech>

kashifest · 2024-02-07T07:41:45Z

/test metal3-bmo-e2e-test-pull
/test-centos-e2e-integration-main

kashifest · 2024-02-07T07:49:37Z

/test metal3-bmo-e2e-test-pull

kashifest · 2024-02-07T08:55:22Z

/cc @tuminoid @lentzi90

lentzi90

/lgtm

kashifest · 2024-03-06T12:15:02Z

/cherry-pick release-0.5

metal3-io-bot · 2024-03-06T12:15:39Z

@kashifest: #1536 failed to apply on top of branch "release-0.5":

Applying: Enabled more golangci linters
Using index info to reconstruct a base tree...
M	Makefile
A	apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go
A	apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go
A	config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml
M	config/render/capm3.yaml
A	test/e2e/bmc.go
M	test/e2e/common.go
M	test/e2e/e2e_suite_test.go
M	test/e2e/live_iso_test.go
M	test/e2e/provisioning_and_annotation_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/provisioning_and_annotation_test.go
CONFLICT (content): Merge conflict in test/e2e/provisioning_and_annotation_test.go
Auto-merging test/e2e/live_iso_test.go
CONFLICT (content): Merge conflict in test/e2e/live_iso_test.go
Auto-merging test/e2e/e2e_suite_test.go
CONFLICT (content): Merge conflict in test/e2e/e2e_suite_test.go
Auto-merging test/e2e/common.go
CONFLICT (modify/delete): test/e2e/bmc.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of test/e2e/bmc.go left in tree.
Auto-merging config/render/capm3.yaml
CONFLICT (modify/delete): config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml left in tree.
CONFLICT (modify/delete): apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go left in tree.
CONFLICT (modify/delete): apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go left in tree.
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Enabled more golangci linters
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.5

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.

kashifest · 2024-03-06T13:35:20Z

/cherry-pick release-0.5

metal3-io-bot · 2024-03-06T13:35:56Z

@kashifest: #1536 failed to apply on top of branch "release-0.5":

Applying: Enabled more golangci linters
Using index info to reconstruct a base tree...
M	Makefile
A	apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go
A	apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go
A	config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml
M	config/render/capm3.yaml
A	test/e2e/bmc.go
M	test/e2e/common.go
M	test/e2e/e2e_suite_test.go
M	test/e2e/live_iso_test.go
M	test/e2e/provisioning_and_annotation_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/provisioning_and_annotation_test.go
CONFLICT (content): Merge conflict in test/e2e/provisioning_and_annotation_test.go
Auto-merging test/e2e/live_iso_test.go
CONFLICT (content): Merge conflict in test/e2e/live_iso_test.go
Auto-merging test/e2e/e2e_suite_test.go
CONFLICT (content): Merge conflict in test/e2e/e2e_suite_test.go
Auto-merging test/e2e/common.go
CONFLICT (modify/delete): test/e2e/bmc.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of test/e2e/bmc.go left in tree.
Auto-merging config/render/capm3.yaml
CONFLICT (modify/delete): config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of config/base/crds/bases/metal3.io_hostfirmwarecomponents.yaml left in tree.
CONFLICT (modify/delete): apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of apis/metal3.io/v1alpha1/hostfirmwarecomponents_types_test.go left in tree.
CONFLICT (modify/delete): apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go deleted in HEAD and modified in Enabled more golangci linters. Version Enabled more golangci linters of apis/metal3.io/v1alpha1/hostfirmwarecomponents_types.go left in tree.
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Enabled more golangci linters
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.5

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.

dtantsur · 2024-03-06T14:36:59Z

pkg/hardwareutils/bmc/ilo4.go

@@ -89,9 +88,8 @@ func (a *iLOAccessDetails) BIOSInterface() string {
 func (a *iLOAccessDetails) BootInterface() string {
 	if a.useVirtualMedia {
 		return "ilo-virtual-media"


Why not constants here?

dtantsur · 2024-03-06T14:37:32Z

pkg/hardwareutils/bmc/redfish.go

@@ -8,7 +8,7 @@ import (

 func init() {
 	schemes := []string{"http", "https"}
-	RegisterFactory("redfish", newRedfishAccessDetails, schemes)
+	RegisterFactory(redfish, newRedfishAccessDetails, schemes)
 	RegisterFactory("ilo5-redfish", newRedfishAccessDetails, schemes)
 	RegisterFactory("idrac-redfish", newRedfishiDracAccessDetails, schemes)


Same question, it's weird to have one thing as a constant and the two other as plain strings.

I'm going to guess it is because the linter complains only after you have N occurrences of the same string.

I only fixed those where the linter was unhappy. And linter usually complain when there is around 3 or more mention of the same string.

Yep, understood. Still, I'd prefer us to use constants consistently, regardless of what the linter forces us to.

True, I will send a patch

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 1, 2024

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2024

kashifest force-pushed the enable/gonalangci-linter-2 branch 2 times, most recently from 70a0905 to 61b921d Compare February 2, 2024 10:24

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 2, 2024

kashifest force-pushed the enable/gonalangci-linter-2 branch 2 times, most recently from 7cc48c2 to 69d5397 Compare February 2, 2024 12:24

kashifest commented Feb 2, 2024

View reviewed changes

tuminoid reviewed Feb 2, 2024

View reviewed changes

apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go Show resolved Hide resolved

pkg/provisioner/ironic/testserver/server.go Outdated Show resolved Hide resolved

kashifest force-pushed the enable/gonalangci-linter-2 branch from 69d5397 to 99b7720 Compare February 2, 2024 13:55

metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 2, 2024

metal3-io-bot requested review from dtantsur, honza, lentzi90, Rozzii, tuminoid and zaneb February 2, 2024 14:00

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2024

Rozzii reviewed Feb 5, 2024

View reviewed changes

zaneb reviewed Feb 6, 2024

View reviewed changes

kashifest force-pushed the enable/gonalangci-linter-2 branch from 8a83e07 to 187b69f Compare February 7, 2024 06:47

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

kashifest force-pushed the enable/gonalangci-linter-2 branch from 187b69f to 00f4320 Compare February 7, 2024 07:11

zaneb reviewed Feb 7, 2024

View reviewed changes

apis/metal3.io/v1alpha1/baremetalhost_types.go Outdated Show resolved Hide resolved

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024

kashifest force-pushed the enable/gonalangci-linter-2 branch 2 times, most recently from 7b306ef to 48cae68 Compare February 7, 2024 07:24

Enabled more golangci linters

314fc48

Signed-off-by: Kashif Khan <kashif.khan@est.tech>

kashifest force-pushed the enable/gonalangci-linter-2 branch from 48cae68 to 314fc48 Compare February 7, 2024 07:41

metal3-io-bot requested a review from lentzi90 February 7, 2024 08:55

lentzi90 reviewed Feb 9, 2024

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024

metal3-io-bot merged commit f06b87b into metal3-io:main Feb 9, 2024
20 checks passed

metal3-io-bot deleted the enable/gonalangci-linter-2 branch February 9, 2024 07:51

lentzi90 mentioned this pull request Feb 9, 2024

make lint only checks root and test folders #1387

Closed

dtantsur reviewed Mar 6, 2024

View reviewed changes

kashifest mentioned this pull request Mar 27, 2024

🌱 Enabled more golangci linters #1638

Merged

🌱 Enabled more golangci linters #1536

🌱 Enabled more golangci linters #1536

Conversation

kashifest commented Feb 1, 2024 • edited

kashifest commented Feb 1, 2024

kashifest commented Feb 2, 2024

kashifest commented Feb 2, 2024

kashifest commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest Feb 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Feb 2, 2024

kashifest commented Feb 2, 2024

kashifest commented Feb 2, 2024

kashifest commented Feb 2, 2024

kashifest commented Feb 5, 2024

Rozzii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Feb 7, 2024

zaneb left a comment

Choose a reason for hiding this comment

metal3-io-bot commented Feb 7, 2024

kashifest commented Feb 7, 2024

kashifest commented Feb 7, 2024

kashifest commented Feb 7, 2024

kashifest commented Feb 7, 2024

kashifest commented Feb 7, 2024

lentzi90 left a comment

Choose a reason for hiding this comment

kashifest commented Mar 6, 2024

metal3-io-bot commented Mar 6, 2024

kashifest commented Mar 6, 2024

metal3-io-bot commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Feb 1, 2024 •

edited

kashifest Feb 2, 2024 •

edited