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.

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

improve repo tooling #805

Merged
merged 6 commits into from Aug 23, 2019

Conversation

@BenTheElder
Copy link
Member

commented Aug 22, 2019

  • perform set on one line in all of the scripts, it's shorter and just as clear
  • use BASH_SOURCE[0] + pwd -P trick instead of git ... to make scripts work from tarballs etc. instead of just from git clones
  • move boilerplate file out of top level hack/
  • upgrade to go 1.13rc1 to prepare for the pending go 1.13 release
  • simplify go_container.sh and move it up to hack/ since we want people to use this
  • replace all linters with golangci-lint
  • fix lint errors
@k8s-ci-robot k8s-ci-robot requested review from amwat and krzyzacy Aug 22, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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

@BenTheElder BenTheElder force-pushed the BenTheElder:shell-nits branch 2 times, most recently from 050c09f to 7e5fb90 Aug 23, 2019
@BenTheElder BenTheElder force-pushed the BenTheElder:shell-nits branch from 7e5fb90 to 6360dd0 Aug 23, 2019
@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/retest
prow flake

BenTheElder added 4 commits Aug 23, 2019
@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Aug 23, 2019
@BenTheElder BenTheElder changed the title nit shell scripts improve repo tooling Aug 23, 2019
GO111MODULE=on bin/golangci-lint \
--enable=golint --enable=vet --enable=gofmt \
--enable=misspell \
run ./pkg/... ./cmd/... .

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

this includes staticcheck and other lints by default, we're explicitly enabling a few that are not on by default:

  • standard go lints (golint, vet, gofmt)
  • misspell

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

iterative (hot) run:

[bentheelder@bentheelder-macbookpro:~/go/src/sigs.k8s.io/kind·2019-08-22T21:38:14-0700·shell-nits@245ca10]
$ time hack/verify/lint.sh 

real    0m7.842s
user    0m23.441s
sys     0m4.258s

much faster than running these individually.

I'm exploring the additional lints we can enable, but may leave that for a follow up.

This comment has been minimized.

Copy link
@aojea

aojea Aug 23, 2019

Contributor

that's progress

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

it's more like a minute on a cold run, but that's not bad considering how much static analysis we're getting. this already caught some bugs we hadn't before, just with the default checks.

k8s.io/code-generator v0.0.0-20190311093542-50b561225d70
k8s.io/gengo v0.0.0-20190327210449-e17681d19d3a // indirect
k8s.io/klog v0.3.0 // indirect
)

// deal with golangci-lint being broken

This comment has been minimized.

Copy link
@BenTheElder

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

basically: go 1.13 is stricter about the timestamps matching the source when using a pseudo-version, the module proxy is equally strict now even in 1.12 / 1.11. golangci-lint needs to update their go.mod upstream, there are PRs open for this.

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/cc @aojea

@k8s-ci-robot k8s-ci-robot requested a review from aojea Aug 23, 2019
hack/go_container.sh Outdated Show resolved Hide resolved
cd "${REPO_ROOT}"
fi

if [[ "${VERIFY_SPELLING:-true}" == "true" ]]; then

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

these are all in lint.sh now, by way of golangci-lint

SOURCE_DIR="${SOURCE_DIR:-$(pwd -P)}"
# default to disabling CGO for easier reproducible builds and cross compilation
export CGO_ENABLED="${CGO_ENABLED:-0}"
# the container image, by default a recent official golang image

This comment has been minimized.

Copy link
@aojea

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

we could use golang:latest but that might get a bit surprising ... probably best to periodically manually update it.

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

(also golang:latest actually works currently, but it's not reproducible..., this is)


# cd to the repo root

This comment has been minimized.

Copy link
@aojea

aojea Aug 23, 2019

Contributor

why do you move this before checking?
if there are no arguments and the next if fails we don't need to cd to the repo root, right?

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Aug 23, 2019

Author Member

for consistency with the other scripts. it doesn't matter but it's easier to spot check these between the scripts (EG if we tweak the REPO_ROOT logic)

@amwat

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 23, 2019
@k8s-ci-robot k8s-ci-robot merged commit 742844e into kubernetes-sigs:master Aug 23, 2019
11 of 14 checks passed
11 of 14 checks passed
tide Not mergeable.
Details
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 1 redirect rule processed
Details
cla/linuxfoundation BenTheElder authorized
Details
deploy/netlify Deploy preview ready!
Details
pull-kind-build Job succeeded.
Details
pull-kind-conformance-parallel Job succeeded.
Details
pull-kind-conformance-parallel-1-12 Job succeeded.
Details
pull-kind-conformance-parallel-1-13 Job succeeded.
Details
pull-kind-conformance-parallel-1-14 Job succeeded.
Details
pull-kind-conformance-parallel-1-15 Job succeeded.
Details
pull-kind-verify Job succeeded.
Details
@BenTheElder BenTheElder deleted the BenTheElder:shell-nits branch Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.