Add spell check #6716

dixudx · 2018-02-08T09:00:26Z

/assign @BenTheElder

Add spelling check to avoid typos when a PR gets merged.

rmmh · 2018-02-08T19:47:14Z

Do we really need to check in a dictionary for this?

BenTheElder · 2018-02-08T20:34:08Z

needs a hack/update-bazel.sh to fix the presubmit

dixudx · 2018-02-09T03:19:22Z

ping @fejta @ixdy for help.

Now package github.com/client9/misspell is vendored. But hack/prune-libraries.sh --fix will still prune it, since it is not a dependency package yet, only used by our shell script.

Is there any way to fix this?

Thanks so much.

ixdy · 2018-02-09T04:36:01Z

It looks like you didn't use dep to pull this into vendor/? I would expect to see changes to Gopkg.toml and Gopkg.lock if you did.

You'll need to add the binary to the required list in Gopkg.toml:

test-infra/Gopkg.toml

Lines 3 to 6 in 4c30b2d

    
           # Updating this list likely also requires updating hack/prune-libraries.sh. 
        
           required = [ 
        
             "github.com/golang/dep/cmd/dep" 
        
           ]

as otherwise dep ensure will remove it later.

To prevent hack/prune-libraries.sh from removing it too, you'll need to do one of two things:

Add it to the REQUIRED list in hack/prune-libraries.sh. https://github.com/kubernetes/test-infra/blob/master/hack/prune-libraries.sh#L27-L32
Alternately, add a Bazel dependency on it somewhere outside vendor/.
For example, rather than creating a new pull-test-infra-verify-spelling prow job, you could wrap hack/verify-spelling.sh in a sh_test (similar to the pylint test) which has a data dependency on //vendor/github.com/client9/misspell/cmd/misspell. This would then build the mispell binary and run the check automatically as part of our generic bazel test job.

dixudx · 2018-02-09T04:55:04Z

/assign @ixdy @fejta

BenTheElder · 2018-02-09T04:56:49Z

@ixdy we should tweak this comment a bit and add it to doc/dep.md #6716 (comment) :-)

BenTheElder

/lgtm
/hold

k8s-ci-robot · 2018-02-09T04:58:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [BenTheElder]

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

BenTheElder · 2018-02-09T04:59:08Z

Thanks! holding in case @ixdy or @fejta have any more nits, will /hold cancel later (tomorrow morning?) if not :-)

ixdy · 2018-02-09T05:19:25Z

It looks like hack/verify-spelling.sh is not executable?

fejta · 2018-02-09T05:31:23Z

We need a hack/verify-sh-is-executable.sh

LGTM from me

ixdy · 2018-02-09T05:55:34Z

I'm pretty sure we don't want to merge this yet, since there seem to be a number of spelling errors that need to be fixed.

I also wonder if we want to turn this into a proper bazel test, rather than a separate verify job. It's actually a pretty simple change, roughly something like

diff --git a/hack/BUILD b/hack/BUILD
index 5795a230e..8ec4e58e3 100644
--- a/hack/BUILD
+++ b/hack/BUILD
@@ -24,6 +24,16 @@ sh_test(
     ],
 )
 
+sh_test(
+    name = "verify-spelling",
+    srcs = ["verify-spelling.sh"],
+    args = ["$(location //vendor/github.com/client9/misspell/cmd/misspell)"],
+    data = [
+        "//:all-srcs",
+        "//vendor/github.com/client9/misspell/cmd/misspell",
+    ],
+)
+
 test_suite(
     name = "verify-all",
     tests = [
diff --git a/hack/verify-spelling.sh b/hack/verify-spelling.sh
old mode 100644
new mode 100755
index 9957a9a95..cfc3e7923
--- a/hack/verify-spelling.sh
+++ b/hack/verify-spelling.sh
@@ -16,14 +16,12 @@
 set -o errexit
 set -o nounset
 set -o pipefail
-set -o xtrace
 
-go install ./vendor/github.com/client9/misspell/cmd/misspell
-if ! which misspell >/dev/null 2>&1; then
-    echo "Can't find misspell - is your GOPATH 'bin' in your PATH?"
-    echo "  GOPATH: ${GOPATH}"
-    echo "  PATH:   ${PATH}"
-    exit 1
-fi
-
-git ls-files | grep -v -e vendor -e static -e third_party | xargs misspell -error
+misspell=$1
+find -L . -type f -not \( \
+  \( \
+  -path '*/vendor/*' \
+  -o -path '*/static/*' \
+  -o -path '*/third_party/*' \
+  \) -prune \
+\) | xargs "${misspell}" -error

It might be nice to pass the location of the misspell binary as a flag, rather than just a bare argument, but I didn't want to go try to figure out getopts for a POC.

ixdy · 2018-02-09T05:58:02Z

oh, my comment about there currently being spelling errors was wrong; this PR needed rebasing.

$ bazel test hack/verify-spelling 
INFO: Analysed target //hack:verify-spelling (14 packages loaded).
INFO: Found 1 test target...
Target //hack:verify-spelling up-to-date:
  bazel-bin/hack/verify-spelling
INFO: Elapsed time: 7.951s, Critical Path: 5.33s
INFO: Build completed successfully, 4 total actions
//hack:verify-spelling                                                   PASSED in 5.2s

(just sayin')

fejta · 2018-02-09T06:03:41Z

Making this a bazel test certainly sounds nice to me. Way easier to run with confidence.

fejta · 2018-02-09T06:04:17Z

(otoh I see no reason why that can't be done in the next PR?)

fejta · 2018-02-09T06:06:34Z

Poor dixudx has already had to deal with godep in k/k and dep in k/t-i :-) seems like sufficient suffering for one PR, and I'm excited to start complaining about how my poor spelling keeps me from merging anything 😁

ixdy · 2018-02-09T06:06:41Z

that's true. this is passing all tests, so lgtm.

dixudx · 2018-02-09T06:08:43Z

@ixdy @fejta So we made above change in this PR or open another one to address it.

ixdy · 2018-02-09T06:09:02Z

another PR is fine.

dixudx · 2018-02-09T06:10:32Z

So it is ready to go? And remove the hold now ?

dixudx · 2018-02-09T06:12:22Z

#6717 has already fixed the typos. It's okay to merge this PR.

ixdy · 2018-02-09T18:03:57Z

/hold cancel

k8s-ci-robot · 2018-02-09T18:13:56Z

@dixudx: Updated the config configmap

Details

In response to this:

/assign @BenTheElder

Add spelling check to avoid typos when a PR gets merged.

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.

ixdy · 2018-02-09T18:22:50Z

bazelification in #6760.

ixdy · 2018-02-09T18:31:38Z

Hm, it seems like this isn't working entirely correctly; there are misspellings it's still missing, such as those being fixed in #6750, #6748, #6747, #6744, and possibly others.

ixdy · 2018-02-09T18:37:10Z

also, the permissions of hack/verify-spelling.sh didn't get fixed in this PR, so now this check is failing everywhere.

dixudx · 2018-02-10T01:02:29Z

Hm, it seems like this isn't working entirely correctly; there are misspellings it's still missing, such as those being fixed in #6750, #6748, #6747, #6744, and possibly others.

Yeah, I found that as well. I've added some new words in client9/misspell#133.

ixdy · 2018-02-10T01:06:00Z

ah, I didn't realize it was a list of corrections, not of known words.

is there an option to add additional corrections on the cmdline? we probably want to correct kuberentes -> kubernetes, but I doubt they'll want that upstream.

dixudx · 2018-02-10T01:10:43Z

is there an option to add additional corrections on the cmdline?

No. So sad.

dixudx requested review from BenTheElder, krzyzacy and michelle192837 as code owners February 8, 2018 09:00

k8s-ci-robot assigned BenTheElder Feb 8, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 8, 2018

dixudx force-pushed the add_spell_check branch from 7e0e3af to e366dcd Compare February 8, 2018 09:06

dixudx mentioned this pull request Feb 8, 2018

fix all the typos #6717

Merged

dixudx force-pushed the add_spell_check branch from e366dcd to d76f99f Compare February 9, 2018 02:33

dixudx added 2 commits February 9, 2018 12:51

vendor misspell

064f69c

add job verify-spelling

ee4c0d9

dixudx force-pushed the add_spell_check branch from d76f99f to ee4c0d9 Compare February 9, 2018 04:52

k8s-ci-robot assigned fejta and ixdy Feb 9, 2018

BenTheElder approved these changes Feb 9, 2018

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 9, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 9, 2018

k8s-ci-robot merged commit 5badbb1 into kubernetes:master Feb 9, 2018

ixdy mentioned this pull request Feb 9, 2018

Wrap hack/verify-spelling in a bazel test #6760

Merged

dixudx deleted the add_spell_check branch February 10, 2018 00:57

Add spell check #6716

Add spell check #6716

Uh oh!

Conversation

dixudx commented Feb 8, 2018

Uh oh!

rmmh commented Feb 8, 2018

Uh oh!

BenTheElder commented Feb 8, 2018

Uh oh!

dixudx commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

dixudx commented Feb 9, 2018

Uh oh!

BenTheElder commented Feb 9, 2018

Uh oh!

BenTheElder left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 9, 2018

Uh oh!

BenTheElder commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

fejta commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

fejta commented Feb 9, 2018

Uh oh!

fejta commented Feb 9, 2018

Uh oh!

fejta commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

dixudx commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

dixudx commented Feb 9, 2018

Uh oh!

dixudx commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

k8s-ci-robot commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

ixdy commented Feb 9, 2018

Uh oh!

dixudx commented Feb 10, 2018

Uh oh!

ixdy commented Feb 10, 2018

Uh oh!

dixudx commented Feb 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants