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 spelling checking script #59463

Merged
merged 3 commits into from Feb 23, 2018

Conversation

@dixudx
Member

dixudx commented Feb 7, 2018

What this PR does / why we need it:
Add spell checking script to avoid involving any typos.

Currently many small PRs are fixing those annoying typos, which is time-consuming and low efficient. We should add such a preflight check before a PR gets merged.

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 #

Special notes for your reviewer:
/sig testing
/area test-infra
/sig release
/cc @ixdy
/assign @liggitt @smarterclayton

Release note:

add spelling checking script
source "${KUBE_ROOT}/hack/lib/init.sh"
# The git commit sha1s here should match the values in $KUBE_ROOT/WORKSPACE.
kube::util::go_install_from_commit \

This comment has been minimized.

@fejta

fejta Feb 7, 2018

Contributor

Can we vendor this code instead?

This comment has been minimized.

@dixudx

dixudx Feb 7, 2018

Member

Currently core source codes do not need this package. We only need it for spell checking.
I can't see a need to vendor this.

This comment has been minimized.

@fejta

fejta Feb 7, 2018

Contributor

If you want a blocking presubmit that runs this code, that code needs to be checked into vendor.

We shouldn't download this code every time we run a PR, nor should we have an outage because client9 decided to move their repos off github.

Note this is the same thing we have done and/or are doing with the rest of these types of tools, see #57600 for example

This comment has been minimized.

@dixudx

dixudx Feb 7, 2018

Member

Done. @fejta PTAL. Thanks.

# skipping vendor/, translations/, third_party/ and all the other generated files
git ls-files | grep -v -e 'vendor/' -e 'translations/' -e 'third_party/' -e 'generated' \
-e 'staging/src/k8s.io/apiserver/pkg/server/routes/data/swagger/datafile.go' | xargs \
misspell -i "Creater,creater" -error

This comment has been minimized.

@fejta

fejta Feb 7, 2018

Contributor

Leave a comment about why creater is acceptable?

This comment has been minimized.

@dixudx

dixudx Feb 7, 2018

Member

Actually this is a legacy.

This comment has been minimized.

@mkumatag

mkumatag Feb 7, 2018

Member

how about avoiding autogenerated files like md, man pages, swagger doc which are getting generated by go source anyways.

This comment has been minimized.

@dixudx

dixudx Feb 8, 2018

Member

Adding swagger is ok and verified.

How about grep -v -e 'vendor/' -e 'translations/' -e 'third_party/' -e 'swagger' -e 'docs/man/'.
@mkumatag @ixdy @liggitt @fejta WDYT?

though I'm not sure I'd block merge on spell check... for example CHANGELOG files are pushed by the release tool

For CHANGELOG files, shall we skip the checking? Actually we could let the release tool automatically fix those typos.

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 7, 2018

@fejta Vendor the package. PTAL. Thanks.

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 7, 2018

This PR can be merged after #59464, which is fixing all the typos across current project.

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 7, 2018

/test pull-kubernetes-unit

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 7, 2018

having a tool like this available is fine, though I'm not sure I'd block merge on spell check... for example CHANGELOG files are pushed by the release tool, and contain release notes scraped from PR descriptions, which would not have been spell checked. This would block merges to master after any release until those typos were fixed, which doesn't seem appropriate.

@fejta

This comment has been minimized.

Contributor

fejta commented Feb 8, 2018

/lgtm
/hold

Let's see if there's a good process to

@fejta

This comment has been minimized.

Contributor

fejta commented Feb 8, 2018

*to follow before merging. Asking #sig-contribex on slack

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Feb 8, 2018

How much time does this add to the verify job? This job is already ridiculously long :/

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 8, 2018

I like this idea, but I share these two concerns:

  • Adds time to verify job, which is already long. If this is suuuper quick, then I could be convinced.
  • I don't necessarily want to block a presubmit on spelling. Unless, perhaps, we have a hack/.spelling_failures file to exempt things from it.
@dixudx

This comment has been minimized.

Member

dixudx commented Feb 8, 2018

Adds time to verify job, which is already long. If this is suuuper quick, then I could be convinced.

@cblecker It runs very fast, taking no more than 1 min.

I don't necessarily want to block a presubmit on spelling. Unless, perhaps, we have a hack/.spelling_failures file to exempt things from it.

I like this suggestion. Waiting for others' inputs. @liggitt @fejta WDYT?

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 8, 2018

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 8, 2018

@liggitt @fejta @cblecker @mkumatag I've added a file hack/.spelling_failures to exempt files from spell checking.

PTAL. Thanks.

@fejta

This comment has been minimized.

Contributor

fejta commented Feb 8, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 8, 2018

@fejta

This comment has been minimized.

Contributor

fejta commented Feb 8, 2018

We need to wait for the other PR to go in anyway, right?

I don't see 1m as being a problem. And this is a great way to not get all these spell check PRs

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 8, 2018

We need to wait for the other PR to go in anyway, right?

@fejta Right. #59464 fixes all the typos across the project, which should be merged first.

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 9, 2018

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 9, 2018

Addressed the comments. Updated. PTAL. Thanks.

@mkumatag

This comment has been minimized.

Member

mkumatag commented Feb 9, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 9, 2018

k8s-merge-robot added a commit that referenced this pull request Feb 11, 2018

Merge pull request #59464 from dixudx/fix_all_typos
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

fix all the typos across the project

**What this PR does / why we need it**:
There are lots of typos across the project. We should avoid small PRs on fixing those annoying typos, which is time-consuming and low efficient.

This PR does fix all the typos across the project currently. And with #59463, typos could be avoided when a new PR gets merged.

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

**Special notes for your reviewer**:
/sig testing
/area test-infra
/sig release
/cc @ixdy 
/assign @fejta 

**Release note**:

```release-note
None
```
@dixudx

This comment has been minimized.

Member

dixudx commented Feb 11, 2018

/retest

# All the skipping files are defined in hack/.spelling_failures
skipping_file="${KUBE_ROOT}/hack/.spelling_failures"
failing_packages=$(echo `cat ${skipping_file}` | sed "s| | -e |g")
git ls-files | grep -v -e ${failing_packages} | xargs misspell -i "Creater,creater" -error

This comment has been minimized.

@cblecker

cblecker Feb 12, 2018

Member

I'd prefer the use of find here, rather than ls-files piping through grep. additionally, I'd also like some logic similar to the golint script around sorting, and maintaining the known failures list.

That said, those can both be done in a follow up (to ensure that this gets merged before more spelling errors get added to the repo). This is still a worthwhile addition to the verify job. Running this locally only adds 3 seconds, but will hopefully catch a number of errors moving forward.

/lgtm

This comment has been minimized.

@dixudx

dixudx Feb 13, 2018

Member

I'd also like some logic similar to the golint script around sorting, and maintaining the known failures list.

@cblecker We have .spelling_failures file here.

This comment has been minimized.

@cblecker

cblecker Feb 22, 2018

Member

Oh, can we add -o stderr to the flags? This will ensure that the errors get picked up in the junit report.

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 12, 2018

@thockin Can you approve the license addition for a new dep? MIT licensed.

k8s-merge-robot added a commit that referenced this pull request Feb 21, 2018

Merge pull request #59600 from aerostitch/misspell
Automatic merge from submit-queue (batch tested with PRs 59292, 59600). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix some minor misspells using misspell

**What this PR does / why we need it**:
Fixes some misspells detected with misspell that might become handy when merging #59463 .
I saw that other files were already taken care of in other PR.
Added those  I didn't see fixed here.

**Special notes for your reviewer**:

I'm not 100% sure if the files in the api dir are supposed to be updated manually. I can revert that part if not.

**Release note**:
```release-note
NONE
```
@dixudx

This comment has been minimized.

Member

dixudx commented Feb 22, 2018

/retest

@thockin

This comment has been minimized.

Member

thockin commented Feb 22, 2018

/approve no-issue

@cblecker

It's currently failing because:

  • hack/update-bazel.sh needs to be run
  • There are three spelling errors right now:
I0222 01:24:28.702] pkg/cloudprovider/providers/azure/azure_routes_test.go:125:24: "addres" is a misspelling of "address"
I0222 01:24:28.981] pkg/kubelet/apis/deviceplugin/v1beta1/constants.go:22:39: "unhealty" is a misspelling of "unhealthy"
I0222 01:24:30.209] pkg/proxy/endpoints.go:57:6: "ect" is a misspelling of "etc"

Let's add these to the failures file with a TODO comment to follow up in another PR so we can get this in.

# All the skipping files are defined in hack/.spelling_failures
skipping_file="${KUBE_ROOT}/hack/.spelling_failures"
failing_packages=$(echo `cat ${skipping_file}` | sed "s| | -e |g")
git ls-files | grep -v -e ${failing_packages} | xargs misspell -i "Creater,creater" -error

This comment has been minimized.

@cblecker

cblecker Feb 22, 2018

Member

Oh, can we add -o stderr to the flags? This will ensure that the errors get picked up in the junit report.

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 23, 2018

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 23, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Feb 23, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dixudx, fejta, mkumatag, thockin

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

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 23, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 23, 2018

@dixudx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kubeadm-gce-canary 1398ac5 link /test pull-kubernetes-e2e-kubeadm-gce-canary

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 23, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit ec77ddf into kubernetes:master Feb 23, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation dixudx authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@dixudx dixudx deleted the dixudx:add_verify_spelling branch Feb 23, 2018

@Bradamant3 Bradamant3 referenced this pull request Mar 8, 2018

Closed

add a misspell check #7651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment