Added govet to our Golang linter suite #5871

bendory · 2022-12-13T02:13:08Z

Changes

This PR adds govet to our Golang linter suite. There are no expected functional changes
in this PR.

Also:

fixed two minor issues uncovered by go vet:
- cmd/nop/main.go:30:3: misuse of unbuffered os.Signal channel as argument to signal.Notify
- internal/sidecarlogresults/sidecarlogresults_test.go:46:4: call to (*T).Fatal from a non-test goroutine
fixed unkeyed composite literal fields in various _test.go files.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

[N/A] Has Docs included if any changes are user facing
[N/A] Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
[N/A] Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
[N/A] Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

bendory · 2022-12-13T02:13:26Z

/kind cleanup

afrittoli · 2022-12-13T09:14:45Z

Thanks @bendory - running go vet sounds good to me.

I'm unsure if we should have a dedicated CI job for it though, since we already run other go tools in the pull-tekton-pipeline-build-tests job, and we plan to migrate that to Tekton.

That said since GitHub actions do not add cost to our infra, I don't mind having the extra job there for now - @vdemeester @abayer wdyt?

bendory · 2022-12-13T11:55:44Z

Thanks @bendory - running go vet sounds good to me.

I'm unsure if we should have a dedicated CI job for it though, since we already run other go tools in the pull-tekton-pipeline-build-tests job, and we plan to migrate that to Tekton.

That said since GitHub actions do not add cost to our infra, I don't mind having the extra job there for now - @vdemeester @abayer wdyt?

I also wondered if GHA or "tekton-on-tekton" is the right answer -- and concluded that GHA is quick and easy for now to keep us in integrity until we get to tekton-on-tekton later.

bendory · 2022-12-13T15:12:15Z

.github/workflows/govet.yml

+    - name: Run vet
+      run: |
+        # Use in-place vendored dependencies.
+        # For now, ignore unkeyed composite fields in structs.


Note: I've already got a follow-on branch that takes care of the unkeyed composite fields. I'm holding that back as a separate PR to keep this PR small.

dibyom · 2022-12-13T17:53:23Z

@bendory could we add govet to the list of linters we already run as part of the build tests: https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L15

bendory · 2022-12-13T18:08:52Z

@bendory could we add govet to the list of linters we already run as part of the build tests: https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L15

🤔

Why are gofmt and errcheck disabled?
We don't have govet disabled, so I assume it runs, but as this PR indicates, we have existing govet errors. What's happening there?

JeromeJu · 2022-12-14T17:04:30Z

https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L15

Might be mistaken but I wonder if we shall add govet to the list at https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L17-L28 to not disable the test if that's what @dibyom pointed.

bendory · 2022-12-14T17:06:37Z

https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L15

Might be mistaken but I wonder if we shall add govet to the list at https://github.com/tektoncd/pipeline/blob/main/.golangci.yml#L17-L28 to not disable the test if that's what @dibyom pointed.

OIC, I misread the configuration -- it leads by disabling everything.

OK, update to this PR coming.

/hold

bendory · 2022-12-14T17:22:59Z

/unhold

JeromeJu

Thanks @bendory
LGTM, just that we might want to update the PR comment and title once the build test passed.

bendory · 2022-12-14T18:39:26Z

Thanks @bendory LGTM, just that we might want to update the PR comment and title once the build test passed.

Done!

bendory · 2022-12-14T18:40:43Z

I've already got code changes ready for the composite literal issues, I'm going to add those here and also addressed the unused findings.

bendory · 2022-12-14T18:49:06Z

I've already got code changes ready for the composite literal issues, I'm going to add those here and also addressed the unused findings.

I decided to split out the unused and whitespace additions to separate PRs in order to keep this PR small.

bendory · 2022-12-14T19:07:30Z

/retest pull-tekton-pipeline-build-tests

tekton-robot · 2022-12-14T19:07:32Z

@bendory: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/retest pull-tekton-pipeline-build-tests

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.

bendory · 2022-12-14T19:07:55Z

/test pull-tekton-pipeline-build-tests

bendory · 2022-12-14T19:36:37Z

/retest

Also: - fixed two minor issues uncovered by `go vet`: - cmd/nop/main.go:30:3: misuse of unbuffered os.Signal channel as argument to signal.Notify - internal/sidecarlogresults/sidecarlogresults_test.go:46:4: call to (*T).Fatal from a non-test goroutine - fixed unkeyed composite literal fields in various _test.go files

.golangci.yml

bendory · 2022-12-15T16:25:24Z

Thanks @bendory - running go vet sounds good to me.

I'm unsure if we should have a dedicated CI job for it though, since we already run other go tools in the pull-tekton-pipeline-build-tests job, and we plan to migrate that to Tekton.

That said since GitHub actions do not add cost to our infra, I don't mind having the extra job there for now - @vdemeester @abayer wdyt?

Note: moved this into our existing dedicated CI job.

bendory · 2022-12-15T18:40:39Z

/assign vdemeester

afrittoli

Thank you!
/lgtm

abayer · 2022-12-16T14:37:04Z

/approve
fwiw. =)

tekton-robot · 2022-12-16T14:37:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, JeromeJu, jerop

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~~ [abayer,jerop]

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

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label Dec 13, 2022

tekton-robot requested review from jerop and vdemeester December 13, 2022 02:13

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 13, 2022

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 13, 2022

bendory commented Dec 13, 2022

View reviewed changes

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2022

JeromeJu approved these changes Dec 14, 2022

View reviewed changes

bendory changed the title ~~Added govet as a GitHub Action.~~ Added govet, unused, and whitespace linters to our Golang linter suite Dec 14, 2022

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 14, 2022

bendory changed the title ~~Added govet, unused, and whitespace linters to our Golang linter suite~~ Added govet to our Golang linter suite Dec 14, 2022

jerop approved these changes Dec 15, 2022

View reviewed changes

.golangci.yml Show resolved Hide resolved

.golangci.yml Show resolved Hide resolved

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 15, 2022

tekton-robot assigned vdemeester Dec 15, 2022

afrittoli reviewed Dec 16, 2022

View reviewed changes

tekton-robot assigned afrittoli Dec 16, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022

tekton-robot merged commit 30fe803 into tektoncd:main Dec 16, 2022

bendory mentioned this pull request Dec 20, 2022

Cleanup: uplevel our technical hygiene by enabling additional Golang linters #5899

Closed

39 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added govet to our Golang linter suite #5871

Added govet to our Golang linter suite #5871

bendory commented Dec 13, 2022 •

edited

Loading

bendory commented Dec 13, 2022

afrittoli commented Dec 13, 2022

bendory commented Dec 13, 2022

bendory Dec 13, 2022

dibyom commented Dec 13, 2022

bendory commented Dec 13, 2022

JeromeJu commented Dec 14, 2022 •

edited

Loading

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

JeromeJu left a comment •

edited

Loading

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

tekton-robot commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 15, 2022

bendory commented Dec 15, 2022

afrittoli left a comment

abayer commented Dec 16, 2022

tekton-robot commented Dec 16, 2022

Added govet to our Golang linter suite #5871

Added govet to our Golang linter suite #5871

Conversation

bendory commented Dec 13, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

bendory commented Dec 13, 2022

afrittoli commented Dec 13, 2022

bendory commented Dec 13, 2022

bendory Dec 13, 2022

Choose a reason for hiding this comment

dibyom commented Dec 13, 2022

bendory commented Dec 13, 2022

JeromeJu commented Dec 14, 2022 • edited Loading

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

JeromeJu left a comment • edited Loading

Choose a reason for hiding this comment

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

tekton-robot commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 14, 2022

bendory commented Dec 15, 2022

bendory commented Dec 15, 2022

afrittoli left a comment

Choose a reason for hiding this comment

abayer commented Dec 16, 2022

tekton-robot commented Dec 16, 2022

bendory commented Dec 13, 2022 •

edited

Loading

JeromeJu commented Dec 14, 2022 •

edited

Loading

JeromeJu left a comment •

edited

Loading