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.

Sign up for GitHub

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

Jump to bottom

🌱 Enabled last batch of linters #1642

Merged
merged 1 commit into from
May 2, 2024
Merged

🌱 Enabled last batch of linters #1642

merged 1 commit into from
May 2, 2024

Conversation

kashifest
Copy link
Member

This PR adds the following last set of golangci linters

  • asasalint
  • bidichk
  • dupword
  • durationcheck
  • errcheck
  • errchkjson
  • gci
  • ginkgolinter
  • goprintffuncname
  • loggercheck
  • nakedret
  • nilerr
  • noctx
  • nosprintfhostport
  • unparam
  • usestdlibvars

I was also eager to enabled containedctx, however it had some issues with recently introduced structs with the following error

  pkg/provisioner/ironic/ironic.go:112:2: found a struct that contains a context.Context field (containedctx)
        ctx context.Context

Since this needs discussion, this is currently disabled.

@metal3-io-bot metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 28, 2024
@kashifest
Copy link
Member Author

/hold
Seems few issues are not caught in local runs

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2024
@kashifest kashifest force-pushed the update-golangci-linters branch from 02af350 to af37f85 Compare March 28, 2024 14:30
@kashifest
Copy link
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2024
@kashifest
Copy link
Member Author

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

@tuminoid
Copy link
Member

tuminoid commented Apr 2, 2024

E2E fixture keeps failing: https://github.com/metal3-io/baremetal-operator/actions/runs/8468903278/job/23328588042?pr=1642

@kashifest
Copy link
Member Author

E2E fixture keeps failing: https://github.com/metal3-io/baremetal-operator/actions/runs/8468903278/job/23328588042?pr=1642

Yes, seems like it is induced by the PR. I am trying to make sense out of the error message currently.

@kashifest kashifest force-pushed the update-golangci-linters branch 4 times, most recently from 80bb236 to 38c1139 Compare April 2, 2024 06:18
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

E2E fixture keeps failing: https://github.com/metal3-io/baremetal-operator/actions/runs/8468903278/job/23328588042?pr=1642

Should be fixed now

tuminoid
tuminoid reviewed Apr 2, 2024
View reviewed changes
.golangci.yaml Outdated Show resolved Hide resolved
tuminoid
tuminoid reviewed Apr 3, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2024
@kashifest
Copy link
Member Author

/cc @lentzi90 @dtantsur

dtantsur
dtantsur reviewed Apr 3, 2024
View reviewed changes
pkg/provisioner/ironic/dependencies.go
@@ -36,14 +35,15 @@ func (p *ironicProvisioner) checkIronicConductor() (ready bool, err error) {
}

driverCount := 0
pager.EachPage(p.ctx, func(_ context.Context, page pagination.Page) (bool, error) {
_ = pager.EachPage(p.ctx, func(_ context.Context, page pagination.Page) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this an error? We probably shouldn't just ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is an error and it breaks the test

Copy link
Member Author

Choose a reason for hiding this comment

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

@dtantsur please check the unit test failure now caused by this, I would need help understanding this in case we dont want to ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't get an easy pointer to this issue, as you said in slack chat, possibly this is due to some bad mocking, I will check it separately (will create an issue for that), for now ignoring it.

@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024
@kashifest kashifest force-pushed the update-golangci-linters branch from 38c1139 to c955780 Compare April 3, 2024 16:36
@metal3-io-bot metal3-io-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 3, 2024
@kashifest kashifest force-pushed the update-golangci-linters branch 2 times, most recently from 585db87 to 293addf Compare April 4, 2024 07:19
@lentzi90
Copy link
Member

lentzi90 commented Apr 5, 2024

Testing pipeline change. Please disregard.
/test metal3-bmo-e2e-test-pull

@kashifest
Copy link
Member Author

@zaneb @dtantsur @honza please review, please review since rebasing this is taking much time and new codes are being added with issues as well.

@kashifest
Copy link
Member Author

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

lentzi90
lentzi90 reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

As a way forward, I suggest we do the import sorting in a separate PR first. That will make this way smaller and easier to review

cmd/make-virt-host/main.go Outdated Show resolved Hide resolved
controllers/metal3.io/controller_test.go Outdated Show resolved Hide resolved
@kashifest kashifest force-pushed the update-golangci-linters branch from 21fb198 to e79cd20 Compare April 29, 2024 13:05
@kashifest
Copy link
Member Author

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

kashifest added a commit to Nordix/baremetal-operator that referenced this pull request Apr 29, 2024
@kashifest
Enable gci linted and fix go imports
a388e92 
This PR adds gci linter and fixes according to gci suggestions. This is part of metal3-io#1642

Signed-off-by: Kashif Khan <kashif.khan@est.tech>
@kashifest kashifest mentioned this pull request Apr 29, 2024
Merged
@kashifest
Copy link
Member Author

As a way forward, I suggest we do the import sorting in a separate PR first. That will make this way smaller and easier to review

Good idea. here you go. #1704 . I will rebase this once #1704 goes in

@kashifest
Copy link
Member Author

/hold
until #1704 goes in

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2024
@kashifest kashifest force-pushed the update-golangci-linters branch from e79cd20 to a62124a Compare April 30, 2024 07:44
@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 30, 2024
@kashifest
Copy link
Member Author

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

@kashifest
Copy link
Member Author

/ok-to-test

@metal3-io-bot metal3-io-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 30, 2024
lentzi90
lentzi90 reviewed Apr 30, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2024
@kashifest
Copy link
Member Author

/hold cancel
@zaneb @dtantsur @honza PTAL

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2024
zaneb
zaneb reviewed May 1, 2024
View reviewed changes
Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

/approve

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved
.golangci.yaml
# - nakedret
# - nilerr
# - noctx
- nakedret
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that this is an improvement, but I respect that other people seem to have a different opinion.

controllers/metal3.io/host_state_machine.go Outdated Show resolved Hide resolved
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90, 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:

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

@kashifest
Enabled last batch of linters
e609781 
Signed-off-by: Kashif Khan <kashif.khan@est.tech>
@kashifest kashifest force-pushed the update-golangci-linters branch from a62124a to e609781 Compare May 2, 2024 06:23
@kashifest
Copy link
Member Author

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

lentzi90
lentzi90 reviewed May 2, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

We have two approvals. Changing mine to lgtm
/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2024
@metal3-io-bot metal3-io-bot merged commit 7dfda13 into metal3-io:main May 2, 2024
17 checks passed
@metal3-io-bot metal3-io-bot deleted the update-golangci-linters branch May 2, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lentzi90 lentzi90 lentzi90 left review comments

@tuminoid tuminoid tuminoid left review comments

@zaneb zaneb zaneb left review comments

@dtantsur dtantsur dtantsur left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kashifest @tuminoid @lentzi90 @metal3-io-bot @dtantsur @zaneb