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.

Sign up for GitHub

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

Jump to bottom

Address linter errors #1982

Merged
merged 1 commit into from
Dec 20, 2018
Merged

Address linter errors #1982

merged 1 commit into from
Dec 20, 2018

Conversation

radu-matei
Copy link
Contributor

First of all, thanks a lot for this awesome project!

This PR begins to address #217, and it addresses all issues that are not comment related, and some obvious comment issues. However, it does not add canonical comments to all exported types - for two reasons - a lot of them are missing, and I don't have the necessary expertise to add any value to the comments.

Thus, this PR addresses all other linter issues, which have been checked with golangci-lint, a nifty tool similar to gometalinter, but significantly faster.

There is still one linter error (see below) where the fix really depends on how we want to use the package - so I didn't want to make any assumptions here - as this PR doesn't make any fundamental changes.

linkerd2: $ golangci-lint run --disable-all -E golint
pkg/filesonly/filesonly.go:15:29: exported func FileSystem returns unexported type filesonly.fileSystem, which can be annoying to use (golint)
func FileSystem(dir string) fileSystem {
                            ^

TODO:

  • there was a comment on adding a precommit.sh script that would also be used by Travis - I'd recommend a Makefile - if it's the route we want to take, I'd happy to follow-up with another PR

  • decide how to approach the remaining linter error

  • decide how to approach the comment related linter errors.

@radu-matei radu-matei force-pushed the go-reportcard branch 3 times, most recently from 38da9d6 to 6c886f2 Compare December 13, 2018 15:48
@klingerf
Copy link
Contributor

Thanks for submitting this, @radu-matei! Looks like a huge improvement, and we'll review it more thoroughly soon.

@wmorgan
Copy link
Member

wmorgan commented Dec 14, 2018

@radu-matei very cool, thanks for kicking this off!

@siggy siggy self-requested a review December 18, 2018 22:02
klingerf
klingerf reviewed Dec 18, 2018
View reviewed changes
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

@radu-matei This is awesome! Thanks for putting it together. If you don't mind addressing my comments below and fixing the merge conflicts with the latest master, this branch should be good to go.

cli/cmd/stat.go Outdated

canonicalType := k8s.ShortNameFromCanonicalResourceName(resourceType)
return canonicalType + "/"

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the extra newline at the end of this func.

cli/install/template.go Outdated
@@ -688,7 +688,8 @@ data:
homeDashboardId: linkerd-top-line
`

const TlsTemplate = `
// TLSTemplate provides the base template for the `linkerd install` command, with TLS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'd phrase this as:

// TLSTemplate provides additional configs when linkerd is installed with `--tls optional`

cli/install/template.go Outdated
@@ -802,6 +803,7 @@ spec:
runAsUser: {{.ControllerUID}}
`

// ProxyInjectorTemplate provides the template for the `linkerd inject` command.
Copy link
Contributor

Choose a reason for hiding this comment

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

And here:

// ProxyInjectorTemplate provides additional configs when linkerd is installed with `--proxy-auto-inject`

siggy
siggy reviewed Dec 18, 2018
View reviewed changes
Copy link
Member

@siggy siggy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is awesome @radu-matei, thanks for doing this!

This all looks great modulo @klingerf's review comments.

Also, regarding FileSystem, since we only call filesonly.FileSystem() in one place in web/srv/server.go, and that call site only requires an http.FileSystem interface, you should safely be able to change your code like this:

diff --git a/pkg/filesonly/filesonly.go b/pkg/filesonly/filesonly.go
index 2460b457..6378dfee 100644
--- a/pkg/filesonly/filesonly.go
+++ b/pkg/filesonly/filesonly.go
@@ -12,7 +12,7 @@ import (
        "os"
 )

-func FileSystem(dir string) fileSystem {
+func FileSystem(dir string) http.FileSystem {
        return fileSystem{http.Dir(dir)}
 }

@radu-matei
Copy link
Contributor Author

radu-matei commented Dec 19, 2018 •
edited
Loading

Hey, thanks for the review!
Will update the branch later today and ping back when it's ready for a re-review.

Thanks!

Edit: updated the branch with the requested changes and fixed the merge conflicts.

@radu-matei
Fix most golint issues that are not comment related
8fc2fa0 
Signed-off-by: Radu Matei <radu@radu-matei.com>
siggy added a commit that referenced this pull request Dec 20, 2018
@siggy
integration test branch for #1982
7fdef0a 
Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy
siggy approved these changes Dec 20, 2018
View reviewed changes
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

Awesome work @radu-matei ! 👍 🚢 🎉

klingerf
klingerf approved these changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@klingerf klingerf left a comment

Choose a reason for hiding this comment

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

⭐️ Great, thanks for addressing my feedback! This is an awesome change. I'll also give some thought to adding linting to CI so that we don't regress over time.

@siggy siggy merged commit 07cbfe2 into linkerd:master Dec 20, 2018
@klingerf klingerf mentioned this pull request Dec 20, 2018
Merged
siggy added a commit that referenced this pull request Dec 22, 2018
@siggy
Enable lint check for comments
2339700 
Part of #217. Follow up from #1982 and #2018.

A subsequent commit will fix the ci failure.

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@siggy siggy mentioned this pull request Dec 22, 2018
Merged
siggy added a commit that referenced this pull request Jan 2, 2019
@siggy
Enable lint check for comments (#2023)
1c30218 
Commit 1: Enable lint check for comments

Part of #217. Follow up from #1982 and #2018.

A subsequent commit will fix the ci failure.

Commit 2: Address all comment-related linter errors.

This change addresses all comment-related linter errors by doing the
following:
- Add comments to exported symbols
- Make some exported symbols private
- Recommend via TODOs that some exported symbols should should move or
  be removed

This PR does not:
- Modify, move, or remove any code
- Modify existing comments

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klingerf klingerf klingerf approved these changes

@siggy siggy siggy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@radu-matei @klingerf @wmorgan @siggy