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

lint: Introduce Makefile target, introduce pre-commit config and fix issues #495

Merged
merged 10 commits into from Mar 14, 2023
Merged

lint: Introduce Makefile target, introduce pre-commit config and fix issues #495

merged 10 commits into from Mar 14, 2023

Conversation

lyarwood
Copy link
Member

@lyarwood lyarwood commented Feb 10, 2023 •
edited

What this PR does / why we need it:

Adds a lint Makefile target using golangci-lint initially with the following plugins enabled:

  • whitespace
  • misspell
  • ginkgolinter
  • errcheck
  • ineffassign
  • staticcheck
  • unused
  • errname

Additionally provides a pre-commit config users can opt into using to help catch issues before committing changes locally.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 10, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 10, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 10, 2023
@ksimon1
Copy link
Member

ksimon1 commented Feb 11, 2023

Do you think you can include this linter into our tests?

@lyarwood
Copy link
Member Author

Do you think you can include this linter into our tests?

Yup indeed that's why this is just a draft for now, I'll try to work something out next week.

@lyarwood lyarwood changed the title ginkgo-linter: Fix all reported errors ginkgo-linter: Fix all reported errors and add a lint Makefile target Feb 13, 2023
@lyarwood lyarwood marked this pull request as ready for review February 13, 2023 09:18
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023
@lyarwood
Copy link
Member Author

/retest-required

@lyarwood lyarwood changed the title ginkgo-linter: Fix all reported errors and add a lint Makefile target golangci-lint: Introduce Makefile lint target and fix issues for the ginkgolint, whitespace and misspell plugins Feb 15, 2023
@lyarwood lyarwood changed the title golangci-lint: Introduce Makefile lint target and fix issues for the ginkgolint, whitespace and misspell plugins golangci-lint: Introduce lint Makefile target and fix issues for the ginkgolint, whitespace and misspell plugins Feb 15, 2023
0xFelix
0xFelix reviewed Feb 15, 2023
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

Nice initiative!

.golangci.yaml Outdated Show resolved Hide resolved
.golangci.yaml Show resolved Hide resolved
@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 15, 2023
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Feb 15, 2023
@lyarwood
Copy link
Member Author

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023
@lyarwood lyarwood marked this pull request as draft February 16, 2023 19:28
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2023
@lyarwood
Copy link
Member Author

/test ci/prow/images

@kubevirt-bot
Copy link
Contributor

@lyarwood: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test ci/prow/images

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.

@lyarwood
Copy link
Member Author

lyarwood commented Mar 8, 2023

/hold

Will continue looking into this next week and will likely end up adding the ability for must-gather to collect the SSP CR.

Slightly confused by the version of must-gather we are using in CI, I can't seem to find a copy of the SSP CR that is present when I use the kubevirt/must-gather image locally. I'll hack around some more today.

@lyarwood
Copy link
Member Author

lyarwood commented Mar 8, 2023

/retest-required

@codingben
Copy link
Member

@lyarwood Is it possible to also include #509 in this PR?

@lyarwood
Copy link
Member Author

lyarwood commented Mar 9, 2023

internal/operands/vm-console-proxy/reconcile_test.go

Yeah I'll rebase on that shortly once I've worked out the failures

lyarwood
lyarwood commented Mar 9, 2023
View reviewed changes
internal/template-validator/service/service.go Outdated
@@ -60,7 +64,9 @@ func Setup(service Service) {
f2 := klogFlags.Lookup(f1.Name)
if f2 != nil {
value := f1.Value.String()
f2.Value.Set(value)
if err := f2.Value.Set(value); err != nil {
panic(fmt.Sprintf("%v", err))
Copy link
Member Author

Choose a reason for hiding this comment

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

Wonderful, this is causing a panic in the tests. Looking in more detail now.

panic: syntax error: expect file.go:234

│ goroutine 1 [running]:                                                                                                                                                                                                                                │
│ kubevirt.io/ssp-operator/internal/template-validator/service.Setup.func1(0xc000284a00)                                                                                                                                                                │
│     /workspace/internal/template-validator/service/service.go:68 +0xd9                                                                                                                                                                                │
│ github.com/spf13/pflag.(*FlagSet).VisitAll(0xc00014e000?, 0xc00009ff10)                                                                                                                                                                               │
│     /workspace/vendor/github.com/spf13/pflag/flag.go:290 +0xec                                                                                                                                                                                        │
│ kubevirt.io/ssp-operator/internal/template-validator/service.Setup({0x18c4760?, 0xc000458000?})                                                                                                                                                       │
│     /workspace/internal/template-validator/service/service.go:63 +0x185                                                                                                                                                                               │
│ main.Main()                                                                                                                                                                                                                                           │
│     /workspace/internal/template-validator/main.go:13 +0x39                                                                                                                                                                                           │
│ main.main()                                                                                                                                                                                                                                           │
│     /workspace/internal/template-validator/main.go:20 +0x19

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed by 1650fa6

@lyarwood
lint: Add Makefile target using golangci-lint
b338d15 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add misspell
b1c7997 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add ginkgolinter
ef51dc2 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
Copy link
Member Author

lyarwood commented Mar 9, 2023

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023
@lyarwood
Copy link
Member Author

/retest-required

@0xFelix
Copy link
Member

0xFelix commented Mar 10, 2023

Looks good, just one ping about a previous comment.

@lyarwood
lint: Add errcheck
9b3bb20 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add ineffassign
5ce8bfa 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add staticcheck
c143559 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add unused
37505b1 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
lint: Add errname
e4c94ed 
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
pre-commit: Add config and use lint
dbe0f21 
This change introduces pre-commit, a useful framework for ensuring
changes meet certain requirements *before* they are fully committed.

This requires the user install pre-commit itself but this is as easy as:

https://pre-commit.com/#usage

$ pip install --user pre-commit
$ cd path/to/ssp-operator && pre-commit install

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@lyarwood
template-validator: Add glog flags to flagset correctly
d5c0c85 
This change removes some older glue code with a native pflag call to
handle the setup of any goflag flags defined by indirect dependencies
like glog. This should be removed when this dependency has gone.

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Mar 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@0xFelix
Copy link
Member

0xFelix commented Mar 13, 2023

Thanks!
/lgtm
/retest

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2023
@lyarwood
Copy link
Member Author

/retest-required

@lyarwood
Copy link
Member Author

https://prow.ci.openshift.org/job-history/gs/origin-ci-test/pr-logs/directory/pull-ci-kubevirt-ssp-operator-master-e2e-single-node-functests looks like the job is broken? I'll take a look in the morning.

@lyarwood
Copy link
Member Author

/retest-required

1 similar comment
@lyarwood
Copy link
Member Author

/retest-required

@kubevirt-bot kubevirt-bot merged commit 6b778b1 into kubevirt:master Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix left review comments

@ksimon1 ksimon1 Awaiting requested review from ksimon1

@akrejcir akrejcir Awaiting requested review from akrejcir

Assignees

@0xFelix 0xFelix

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lyarwood @ksimon1 @kubevirt-bot @0xFelix @codingben @akrejcir