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

add shellcheck lint #68438

Merged
merged 4 commits into from Jan 12, 2019

Conversation

@BenTheElder
Copy link
Member

commented Sep 8, 2018

What this PR does / why we need it: Adds a shellcheck lint to find errors in shell scripts, initially marks all failing scripts as known failures. We can incrementally fix these / disable unnecessary lint error codes going forward.

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 #24614

Special notes for your reviewer:

  • we'll still need to add an actual presubmit job using this via PR to test-infra we're adding it to the existing job
  • this is still WIP because I want to:
    • sanity check by fixing some failures and removing a file from the known failures
    • discuss if there are any other error codes we should ignore can be done in follow-ups
    • figure out how we want to version / locally install shellcheck we're now using the official docker image

Release note:

NONE

/sig testing

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2018

/assign @cblecker @ixdy
Probably the biggest issue is how we want to have users install shellcheck / how we want to version it in CI (do we want to fetch binaries ourselves?)

The other problem is that it's a little slow. There's possibly some optimizations to be had.

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch from 58fc22c to 93c62fa Sep 8, 2018

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch 3 times, most recently from 8edced6 to c21b6e9 Sep 8, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

@dims

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

Suggestion : this is GPLv3 code in https://github.com/koalaman/shellcheck and looks like it's good news that they have published container images https://hub.docker.com/r/koalaman/shellcheck/tags/ - can we please use those?

@dims

This comment has been minimized.

Copy link
Member

commented Sep 9, 2018

we can always optimize it to nail the shellcheck version (and check if there is a local copy and run that instead of needing docker)

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2018

SGTM, we can probably just run the presubmit in this image too using the podutils. I'll take a look at that soon.

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

/hold
We won't want to merge this prior to the release anyhow. I will re-work this to support the shellcheck docker image when we're a little bit closer to the next cycle.

@nikhita

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

/kind feature

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

Will try to come back to this in the next cycle, I think this is still important but I haven't had the time to sort out how we wrap the upstream docker image instead.

@BenTheElder BenTheElder changed the title [WIP] add shellcheck lint add shellcheck lint Jan 11, 2019

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch from 0f90c01 to 9fd6e40 Jan 11, 2019

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

the last remaining question I have: do we make this part of the default verify suite, or add a new job per my original comment?

BenTheElder added some commits Jan 11, 2019

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch from 9fd6e40 to d150df8 Jan 11, 2019

@dims

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

if it's 2-3 mins, am ok to add it to the existing job @BenTheElder

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

/test pull-kubernetes-verify-podutil

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch 3 times, most recently from d998e20 to dab4794 Jan 11, 2019

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

/test pull-kubernetes-verify-podutil

@BenTheElder BenTheElder force-pushed the BenTheElder:shellcheck branch from dab4794 to d150df8 Jan 11, 2019

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

/test pull-kubernetes-verify

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2019

@spiffxp

This comment has been minimized.

Copy link
Member

commented Jan 12, 2019

/approve
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, spiffxp

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

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 171485f into kubernetes:master Jan 12, 2019

18 checks passed

cla/linuxfoundation BenTheElder authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@BenTheElder BenTheElder deleted the BenTheElder:shellcheck branch Jan 12, 2019

@BenTheElder BenTheElder referenced this pull request Jan 16, 2019

Open

Eliminate shellcheck failures #72956

95 of 284 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.