Skip to content

golint fix TLs->TLS in docker/#14764

Merged
calavera merged 2 commits intomoby:masterfrom
sevki:14756-lint
Jul 20, 2015
Merged

golint fix TLs->TLS in docker/#14764
calavera merged 2 commits intomoby:masterfrom
sevki:14756-lint

Conversation

@sevki
Copy link
Contributor

@sevki sevki commented Jul 20, 2015

golint errors mentioned in #14756

docker/flags.go:29:2: var dockerTlsVerify should be dockerTLSVerify
docker/flags.go:94:2: var flTls should be flTLS
docker/flags.go:96:2: var flTlsVerify should be flTLSVerify

@sevki
golint fix TLs->TLS in docker/
16ea3cf 
Signed-off-by: Sevki Hasirci <s@sevki.org>
runcom
runcom reviewed Jul 20, 2015
View reviewed changes
docker/daemon.go Outdated
Copy link
Member

@runcom runcom Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be TLS? :) seems like the L is still lowercase

Copy link
Contributor Author

@sevki sevki Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 just noticed that. need to switch to fonts

@sevki
fix mixed typo TlS -> TLS
ce3156f 
Signed-off-by: Sevki Hasirci <s@sevki.org>
@runcom
Copy link
Member

runcom commented Jul 20, 2015

LGTM if janky's happy, thanks @sevki !!

@sevki
Copy link
Contributor Author

sevki commented Jul 20, 2015

@runcom cheers. 🎉 my first PR to docker.

@sevki
Copy link
Contributor Author

sevki commented Jul 20, 2015

@runcom had a side project that commented inline on golint issues. Should I include a script to run that against commits in this PR too?

@runcom
Copy link
Member

runcom commented Jul 20, 2015

@sevki interesting, we may need to discuss that so either create a PR or just come on IRC to chat (@jfrazelle or others may have more to say about including your script in CI if it's really useful also, not sure tho)

@sevki
Copy link
Contributor Author

sevki commented Jul 20, 2015

@runcom well I say script but it's basically literally this

joker -repo=docker -owner=docker -token={token} -commit=`git describe --always` -scanner=golint

don't know if it's PR worthy, I'll hop on IRC

@calavera
Copy link
Contributor

calavera commented Jul 20, 2015

I think we don't want to run golint just yet because we have too much technical debt that doesn't match the go linter. Either way that should be another PR.

Thanks for fixing this issue. LGTM. Merging!

calavera added a commit that referenced this pull request Jul 20, 2015
@calavera
Merge pull request #14764 from sevki/14756-lint
bd8386c 
golint fix TLs->TLS in docker/
@calavera calavera merged commit bd8386c into moby:master Jul 20, 2015
@sevki
Copy link
Contributor Author

sevki commented Jul 20, 2015

@calavera that app only works on modified or added lines in a commit, doesn't lint existing stuff. so no noise, it's just about preventing new stuff from coming in.

@calavera
Copy link
Contributor

calavera commented Jul 20, 2015

@sevki that sounds good, we could integrate it in https://github.com/jfrazelle/leeroy. We might want to post only one comment with all the warnings instead of line by line though.

@icecrime icecrime mentioned this pull request Jul 20, 2015
Closed
tiborvass
tiborvass reviewed Jul 21, 2015
View reviewed changes
docker/flags.go
Copy link
Contributor

@tiborvass tiborvass Jul 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sevki @runcom @calavera you missed this one :P it should be TLS not TlS ;)

EDIT: I'll fix it in my PR

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

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sevki @runcom @calavera @tiborvass @GordonTheTurtle