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

Eliminate shellcheck failures #72956

Closed
BenTheElder opened this issue Jan 16, 2019 · 129 comments · Fixed by #74193, #75662 or #99816
Closed

Eliminate shellcheck failures #72956

BenTheElder opened this issue Jan 16, 2019 · 129 comments · Fixed by #74193, #75662 or #99816
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Milestone

Comments

@BenTheElder
Copy link
Member

BenTheElder commented Jan 16, 2019

What happened: After #24614 / #68438 we had 283 known failing *.sh files being excluded from shellcheck lints.

What you expected to happen: We should be linting all files. When files are listed in hack/.shellcheck_failures we may miss actual bugs in them. Initially to get the linter in place we had to list many files there to keep the PR reasonably sized, but ideally we can fix these over time and eliminate this file.

How to reproduce it (as minimally and precisely as possible): hack/verify-shellcheck.sh

Anything else we need to know?: I intend to continue working on this, EG #72861, #72955, ...

/sig testing
/assign

Known failing files TODO:

@BenTheElder BenTheElder added the kind/bug Categorizes issue or PR as related to a bug. label Jan 16, 2019
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jan 16, 2019
@BenTheElder
Copy link
Member Author

@spiffxp I am tackling this optimistically for 1.14, but I could see it slipping to 1.15 depending on time-to-review, bandwidth, etc..

@BenTheElder
Copy link
Member Author

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jan 16, 2019
@spiffxp
Copy link
Member

spiffxp commented Jan 18, 2019

/milestone v1.14

@phenixblue
Copy link
Member

@BenTheElder would it be ok if I helped out with this? I was thinking I could go through and cover the scripts in ./cluster/centos/ to start with

@BenTheElder
Copy link
Member Author

That would be great! there's a lot to tackle here 😅

If you could reference this issue (just include a link somewhere, please don't mark it as "fixes #" 🙃) that would be helpful for attempting to avoid overlap :-)
I'll continue to do the same as I get to more of these.

@war-turtle
Copy link
Contributor

@BenTheElder
Can I help with ./build/common.sh

@BenTheElder
Copy link
Member Author

BenTheElder commented Jan 20, 2019 via email

@tanshanshan
Copy link

I will fix
./hack/dev-build-and-push.sh
./hack/dev-build-and-up.sh
./hack/dev-push-conformance.sh
./hack/dev-push-hyperkube.sh

@phenixblue
Copy link
Member

I'll work on the scripts in ./cluster/gce/

@mysunshine92
Copy link
Contributor

I will fix
./cluster/addons/addon-manager/kube-addons.sh
./cluster/addons/fluentd-elasticsearch/es-image/run.sh
./cluster/addons/fluentd-elasticsearch/fluentd-es-image/run.sh

@danielqsj
Copy link
Contributor

I will fix ./staging/

@joakimr-axis
Copy link
Contributor

#90405's sub-PR #94458 has now been approved.

@MonzElmasry
Copy link

Hey 👋 Bug Triage here. Wanted to follow up on the status of this issue as we're approaching code freeze on 12.11.2020. This issue is tagged for 1.20, is it still planned for this release?

@joakimr-axis
Copy link
Contributor

Current status is (still) 3 remaining (although a handful of the sub-PRs for cluster/gce/util.sh have been merged):

./cluster/gce/gci/configure.sh #90442 (all tests passed, ready for jay or nay)
./cluster/gce/gci/master-helper.sh #88582 (closed due to inactivity!)
./cluster/gce/util.sh #90405 (has sub-PRs #94456 #94459 #94463 breaking up all the changes into smaller pieces easier to handle)

@joakimr-axis
Copy link
Contributor

I have now created #95865 which is bringing @gavinfish's closed-due-to-inactivity #88582 back to life with a few updates. With that one, the lineup of active ongoings would look like:

./cluster/gce/gci/configure.sh #90442 (all tests passed, ready for jay or nay)
./cluster/gce/gci/master-helper.sh #95865
./cluster/gce/util.sh #90405 (has sub-PRs #94456 #94459 #94463 breaking up all the changes into smaller pieces easier to handle)

@erismaster
Copy link

👋 Hi, Bug Triage here, we're pretty late into the code freeze for 1.20 - I am going to put this in the 1.21 milestone

/milestone v1.21

@spiffxp
Copy link
Member

spiffxp commented Jan 15, 2021

Since I just did this out of curiousity for hack/.golint_failures...

There have been 258 commits that touched hack/.shellcheck_failures in the past 2 years

shellcheck_failures linecount over time

@joakimr-axis
Copy link
Contributor

So close now! Getting there...

@joakimr-axis
Copy link
Contributor

./cluster/gce/gci/configure.sh #90442 (all tests passed, ready for jay or nay)
./cluster/gce/gci/master-helper.sh #95865
./cluster/gce/util.sh #90405 (has sub-PRs #94456 #94459 #94463 breaking up all the changes into smaller pieces easier to handle)

The sub-PRs #94456 #94459 #94463 are now merged and the size of changes that remain in #90405 significantly decreased.

#90442 and #95865 have both passed all tests in the CI pipeline and are waiting for someone to approve (or further comment anything that should be changed) them.

@spiffxp spiffxp added this to To Triage in sig-testing issues Feb 9, 2021
@spiffxp spiffxp moved this from To Triage to Help Wanted in sig-testing issues Feb 9, 2021
@BenTheElder
Copy link
Member Author

I think we should be landing these soon, well within 1.21.

Then we can follow up with making the verify-shellcheck script much simpler and faster (by not supporting known failures).
~~ https://github.com/kubernetes-sigs/kind/blob/master/hack/make-rules/verify/shellcheck.sh

Updated the checklist again, looking at outstanding PRs.

@BenTheElder
Copy link
Member Author

still may hit some test flakes, but I believe the remaining fixes are all LGTM+approve.

@joakimr-axis
Copy link
Contributor

Joy to the world, all fixes for cluster/gce/util.sh (#90405) are finally in place!

@joakimr-axis
Copy link
Contributor

And BAM now #90442 is also merged, leaving hack/.shellcheck_failures an empty file. 👍

@BenTheElder
Copy link
Member Author

#99816 removes the hack/.shellcheck_failures file and associated logic. No more failures allowed, CI will enforce this now*

* unless someone really needs to ignore something and adds a #shellcheck disable=... instead 🙃

Thank you so much all!

@BenTheElder
Copy link
Member Author

xref: https://kubernetes.slack.com/archives/C92G08FGD/p1614915017006400 - thank you!
(invite to join: https://slack.k8s.io)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Projects