Add codespell to validate spelling mistakes in code. #4817

rhatdan · 2020-01-08T14:39:26Z

Fix all errors found by codespell

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci-robot · 2020-01-08T14:39:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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:

~~OWNERS~~ [rhatdan]

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

mheon · 2020-01-08T14:44:18Z

cmd/podman/containers_prune.go

@@ -49,11 +49,11 @@ func pruneContainersCmd(c *cliconfig.PruneContainersValues) error {
 		reader := bufio.NewReader(os.Stdin)
 		fmt.Printf(`WARNING! This will remove all stopped containers.
 Are you sure you want to continue? [y/N] `)
-		ans, err := reader.ReadString('\n')
+		answer, err := reader.ReadString('\n')


I really don't like this. Can we restrict to comments only? I don't want a spell-checker telling me what my variable names can and cannot be.

Sure, and we can tell the codespeller to ignore certain names, in this case I actually liked the change, since I was not crazy about ans.

TomSweeneyRedHat · 2020-01-08T14:45:24Z

Makefile

@@ -530,7 +534,7 @@ validate.completions: completions/bash/podman
 	. completions/bash/podman
 	if [ -x /bin/zsh ]; then /bin/zsh completions/zsh/_podman; fi

-validate: gofmt .gitvalidation validate.completions golangci-lint man-page-check
+validate: gofmt .gitvalidation validate.completions golangci-lint man-page-check codespell


I'm just concerned about codespell being a bit overzealous and keeping the build from happening. Will it just ping a warning or stop the submit from going forward?

I think we should have this on by default, and see if it causes issues, once I get it into a good state. I think this would help us keep this clean.

Off the bat, this will run first from the gate task here. Adding things to the container image is easy, see contrib/gate/Dockerfile. Do that in a separate PR, and once merged, quay will automatically build the image (can be confirmed by logging in and checking it on quay). Then, re-running the jobs here will start using the new image.

I have disabled it by default for now, so we can get this PR merged, and then we can figure out how to get the CI system to gate on it in a future PR.

mheon · 2020-01-08T14:45:29Z

libpod/container_internal_linux.go

@@ -1365,7 +1365,7 @@ func (c *Container) getHosts() string {
 }

 // generatePasswd generates a container specific passwd file,
-// iff g.config.User is a number
+// if g.config.User is a number


This is definitely wrong - iff is accepted terminology

vrothberg

AFAIK, codespell is too strict in some cases. golangci-lint does spellchecks already but less strict. Maybe we can run it periodically but not make it part of the CI?

mheon · 2020-01-08T14:46:26Z

pkg/spec/storage.go

@@ -789,7 +789,7 @@ func (config *CreateConfig) getTmpfsMounts() (map[string]spec.Mount, error) {
 	return m, nil
 }

-// AddContainerInitBinary adds the init binary specified by path iff the
+// AddContainerInitBinary adds the init binary specified by path if the


Again, iff is valid

Yes I will revert and make iff valid.

TomSweeneyRedHat · 2020-01-08T14:46:48Z

cmd/podman/images_prune.go

 		if err != nil {
 			return errors.Wrapf(err, "error reading input")
 		}
-		if strings.ToLower(ans)[0] != 'y' {
+		if strings.ToLower(answer)[0] != 'y' {


This right here is an example of a change that I don't think should keep a build from completing. 'answer' is better for sure, but not a stopper in my book.

I think this is better.

TomSweeneyRedHat · 2020-01-08T14:47:31Z

contrib/spec/podman.spec.in

@@ -19,7 +19,7 @@
 %endif

 # %if ! 0% {?gobuild:1}
-%define gobuild(o:) go build -tags="$BUILDTAGS" -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|od -An -tx1|tr -d ' \\n')" -a -v -x %{?**};
+%define gobuild(o:) go build -tags="$BUILDTAGS" -ldflags "${LDFLAGS:-} -B 0x$(head -c20 /dev/urandom|of -An -tx1|tr -d ' \\n')" -a -v -x %{?**};


is this a valid change?

Nope, I thought I reverted, this one.

Oops, looks like it got merged.

giuseppe · 2020-01-08T14:47:53Z

docs/source/markdown/podman-logs.1.md

@@ -52,7 +52,7 @@ To view a container's logs:
 ```
 podman logs -t b3f2436bdb978c1d33b1387afb5d7ba7e3243ed2ce908db431ac0069da86cb45

-2017/08/07 10:16:21 Seeked /var/log/crio/pods/eb296bd56fab164d4d3cc46e5776b54414af3bf543d138746b25832c816b933b/c49f49788da14f776b7aa93fb97a2a71f9912f4e5a3e30397fca7dfe0ee0367b.log - &{Offset:0 Whence:0}
+2017/08/07 10:16:21 Sought /var/log/crio/pods/eb296bd56fab164d4d3cc46e5776b54414af3bf543d138746b25832c816b933b/c49f49788da14f776b7aa93fb97a2a71f9912f4e5a3e30397fca7dfe0ee0367b.log - &{Offset:0 Whence:0}


the log message is generated by: https://github.com/hpcloud/tail/blob/master/tail.go#L244

Should we replace it?

Nope should be reverted.

TomSweeneyRedHat · 2020-01-08T14:48:33Z

docs/source/markdown/podman-logs.1.md

@@ -52,7 +52,7 @@ To view a container's logs:
 ```
 podman logs -t b3f2436bdb978c1d33b1387afb5d7ba7e3243ed2ce908db431ac0069da86cb45

-2017/08/07 10:16:21 Seeked /var/log/crio/pods/eb296bd56fab164d4d3cc46e5776b54414af3bf543d138746b25832c816b933b/c49f49788da14f776b7aa93fb97a2a71f9912f4e5a3e30397fca7dfe0ee0367b.log - &{Offset:0 Whence:0}
+2017/08/07 10:16:21 Sought /var/log/crio/pods/eb296bd56fab164d4d3cc46e5776b54414af3bf543d138746b25832c816b933b/c49f49788da14f776b7aa93fb97a2a71f9912f4e5a3e30397fca7dfe0ee0367b.log - &{Offset:0 Whence:0}


This probably should not change.

Nope these should be eliminated

rhatdan · 2020-01-08T16:19:55Z

@cevich Do I need to rebuild AMI to get codespell installed?

cevich · 2020-01-08T19:28:03Z

to get codespell installed?

Possibly, depends on how it's being called. Installing at runtime is possible if you can find all the places it's required, but will slow down testing for everyone and reduce automation reliability slightly. Let me take a peek at what you have here, then I can make better suggestions...

cevich · 2020-01-08T19:39:09Z

...https://github.com/containers/libpod/pull/4817/files#r364406947 I don't recall if we run 'make validate' from anywhere else (or it gets called by a dependency). Getting past the gate job should be easy, then we'll know if it breaks elsewhere. If so, I can help deal with that.

Fix all errors found by codespell Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2020-01-13T15:38:32Z

@TomSweeneyRedHat @mheon @vrothberg Since codespell is no longer blocking can we get his merged?

vrothberg

LGTM

TomSweeneyRedHat

LGTM
FWIW, I too like the ans to answer variable name change, I just didn't want changes like that holding up a PR in test.

mheon · 2020-01-13T16:23:30Z

/lgtm

openshift-ci-robot requested review from mrunalp and umohnani8 January 8, 2020 14:39

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M labels Jan 8, 2020

mheon reviewed Jan 8, 2020

View reviewed changes

TomSweeneyRedHat reviewed Jan 8, 2020

View reviewed changes

mheon reviewed Jan 8, 2020

View reviewed changes

vrothberg reviewed Jan 8, 2020

View reviewed changes

mheon reviewed Jan 8, 2020

View reviewed changes

TomSweeneyRedHat reviewed Jan 8, 2020

View reviewed changes

giuseppe reviewed Jan 8, 2020

View reviewed changes

TomSweeneyRedHat reviewed Jan 8, 2020

View reviewed changes

rhatdan force-pushed the codespell branch from 7a8e6ac to 733c85b Compare January 8, 2020 16:15

Add codespell to validate spelling mistakes in code.

4093b2c

Fix all errors found by codespell Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the codespell branch from 733c85b to 4093b2c Compare January 11, 2020 11:32

vrothberg reviewed Jan 13, 2020

View reviewed changes

TomSweeneyRedHat reviewed Jan 13, 2020

View reviewed changes

openshift-ci-robot assigned mheon Jan 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020

openshift-merge-robot merged commit e83a1b8 into containers:master Jan 13, 2020

rh-atomic-bot mentioned this pull request Jan 13, 2020

Enable shell completion for podman commands. #4768

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add codespell to validate spelling mistakes in code. #4817

Add codespell to validate spelling mistakes in code. #4817

rhatdan commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

mheon Jan 8, 2020

rhatdan Jan 13, 2020

TomSweeneyRedHat Jan 8, 2020 •

edited

Loading

rhatdan Jan 8, 2020

cevich Jan 8, 2020

rhatdan Jan 13, 2020

mheon Jan 8, 2020

rhatdan Jan 13, 2020

vrothberg left a comment

mheon Jan 8, 2020

rhatdan Jan 8, 2020

rhatdan Jan 13, 2020

TomSweeneyRedHat Jan 8, 2020

rhatdan Jan 8, 2020

TomSweeneyRedHat Jan 8, 2020

rhatdan Jan 8, 2020

edsantiago Jan 13, 2020

giuseppe Jan 8, 2020

rhatdan Jan 8, 2020

TomSweeneyRedHat Jan 8, 2020

rhatdan Jan 8, 2020

rhatdan commented Jan 8, 2020

cevich commented Jan 8, 2020

cevich commented Jan 8, 2020

rhatdan commented Jan 13, 2020

vrothberg left a comment

TomSweeneyRedHat left a comment

mheon commented Jan 13, 2020

Add codespell to validate spelling mistakes in code. #4817

Add codespell to validate spelling mistakes in code. #4817

Conversation

rhatdan commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jan 8, 2020

cevich commented Jan 8, 2020

cevich commented Jan 8, 2020

rhatdan commented Jan 13, 2020

vrothberg left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

mheon commented Jan 13, 2020

TomSweeneyRedHat Jan 8, 2020 •

edited

Loading