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

🐛 golangci-lint should use go-build tags appropriately #5540

Merged
merged 1 commit into from
Nov 4, 2021
Merged

🐛 golangci-lint should use go-build tags appropriately #5540

merged 1 commit into from
Nov 4, 2021

Conversation

vincepri
Copy link
Member

Signed-off-by: Vince Prignano vincepri@vmware.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 29, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 29, 2021
@vincepri vincepri force-pushed the hack-tools-gomod branch 3 times, most recently from 3ebd037 to ed508a1 Compare November 3, 2021 20:22
vincepri
vincepri commented Nov 3, 2021
View reviewed changes
hack/boilerplate/boilerplate.py
@@ -62,10 +62,6 @@ def get_refs():
return refs
Copy link
Member Author

Choose a reason for hiding this comment

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

This file was updated from upstream https://github.com/kubernetes/kubernetes/blob/4dfd73940396730caf331e35cbb28235d233f2a0/hack/boilerplate/boilerplate.py

Copy link
Member

Choose a reason for hiding this comment

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

Should we add a comment somewhere (top of the file or a README.md) where we got/sync the file from? (I assume this also applies to the other boilerplate files)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a readme, given that the whole folder was copied

@vincepri
Copy link
Member Author

vincepri commented Nov 3, 2021

/assign @sbueringer @fabriziopandini

sbueringer
sbueringer reviewed Nov 4, 2021
View reviewed changes
hack/boilerplate/boilerplate.py
@@ -62,10 +62,6 @@ def get_refs():
return refs
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a comment somewhere (top of the file or a README.md) where we got/sync the file from? (I assume this also applies to the other boilerplate files)

hack/tools/mdbook/tabulate/tabulate.go Outdated
@@ -28,10 +29,13 @@ import (
"sigs.k8s.io/kubebuilder/docs/book/utils/plugin"
)

// Tabulate.
// Tabulate adds support for switchable tabls in mdbook.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Tabulate adds support for switchable tabls in mdbook.
// Tabulate adds support for switchable tabs in mdbook.

nit

Copy link
Member Author

Choose a reason for hiding this comment

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

tabls 😂

.golangci.yml
- linters:
- typecheck
text: import (".+") is a program, not an importable package
path: ^tools\.go$
Copy link
Member

Choose a reason for hiding this comment

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

nit: should we match the entire path and not only the filename?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't, the path is relative to where golanci-lint runs, which is the tools folder in this case. It's not ideal but that's also why I added a stricter validation here

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah makes sense

hack/tools/mdbook/embed/embed.go
@@ -1,3 +1,4 @@
//go:build tools
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to enforce this going forward?

Copy link
Member Author

@vincepri vincepri Nov 4, 2021 •
edited

Choose a reason for hiding this comment

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

It's enforced by golangci-lint now through gofmt, and you can use make lint-fix

@sbueringer
Copy link
Member

Thx!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2021
@vincepri
🐛 golangci-lint should use build tags appropriately
fb1f862 
Signed-off-by: Vince Prignano <vincepri@vmware.com>
sbueringer
sbueringer reviewed Nov 4, 2021
View reviewed changes
internal/envtest/webhooks.go
@@ -67,7 +67,7 @@ func initWebhookInstallOptions() envtest.WebhookInstallOptions {
if err != nil {
klog.Fatalf("Failed to append bootstrap controller webhook config: %v", err)
}
controlplaneyamlFile, err := os.ReadFile(filepath.Join(root, "controlplane", "kubeadm", "config", "webhook", "manifests.yaml"))
controlplaneyamlFile, err := os.ReadFile(filepath.Join(root, "controlplane", "kubeadm", "config", "webhook", "manifests.yaml")) //nolint:Gosec
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
controlplaneyamlFile, err := os.ReadFile(filepath.Join(root, "controlplane", "kubeadm", "config", "webhook", "manifests.yaml")) //nolint:Gosec
controlplaneyamlFile, err := os.ReadFile(filepath.Join(root, "controlplane", "kubeadm", "config", "webhook", "manifests.yaml")) //nolint:gosec

nit if you want

fabriziopandini
fabriziopandini reviewed Nov 4, 2021
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

More lint!
Thanks!
/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7e42829 into kubernetes-sigs:main Nov 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@enxebre enxebre Awaiting requested review from enxebre

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vincepri @sbueringer @k8s-ci-robot @fabriziopandini