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.

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

Enable lint check for comments #2023

Merged
merged 3 commits into from
Jan 2, 2019
Merged

Enable lint check for comments #2023

merged 3 commits into from
Jan 2, 2019

Conversation

siggy
Copy link
Member

@siggy siggy commented Dec 22, 2018

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

2nd commit

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

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>
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>
@siggy
Copy link
Member Author

siggy commented Dec 30, 2018

Relates to #216.

@siggy siggy mentioned this pull request Dec 30, 2018
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.

⭐️ 😍 Thanks for cleaning all of this up! Just had a few small comments but otherwise looks good to me.

@@ -5,6 +5,8 @@ import (
"google.golang.org/grpc"
)

// NewClient creates a new gRPC client to the Destination service.
// TODO: consider moving this into destination-client, or removing altogether.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Looks like this is only called from the destination-client script, so I'm definitely in favor of removing this file and moving the code directly into the script. But doesn't necessarily have to happen as part of this branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, will address in a subsequent PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followup: #2032

@@ -190,6 +190,7 @@ func newClient(apiURL *url.URL, httpClientToUse *http.Client, controlPlaneNamesp
}, nil
}

// NewInternalClient creates a new Public API client intended to run inside a Kubernetes cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind wrapping this at 80 characters? Same goes for the comment on line 203 in this file.

Res model.Value
QueriesExecuted []string // expose the queries our Mock Prometheus receives, to test query generation
rwLock sync.Mutex
}

// PodCounts is a test helper struct that for representing data in a
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here. Maybe it should be: // PodCounts is a test helper struct that is used for representing data in a

@@ -535,6 +547,8 @@ func routeLabels(event *pb.TapEvent) string {
return out
}

// RenderTapEvent renders a Public API TapEvent to a string.
// TODO: consider moving this into cli/cmd/tap.go.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Ah, right, this was originally moved here when we were rendering tap output as plaintext in the web UI, but we could move it back now that the web has its own rendering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a subsequent PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followup: #2032

func (api *API) Pod() coreinformers.PodInformer {
if api.pod == nil {
panic("Pod informer not configured")
}
return api.pod
}

// RC provides access to a shared informer and lister for ReplicaSets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here: ReplicaSets => ReplicationControllers

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
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 updating!

@siggy siggy merged commit 1c30218 into master Jan 2, 2019
@siggy siggy deleted the siggy/all-the-comments branch January 2, 2019 22:04
siggy added a commit that referenced this pull request Jan 2, 2019
This is a followup branch from #2023:
- delete `proxy/client.go`, move code to `destination-client`
- move `RenderTapEvent` and `GetPercentTLS` from `util` to `cmd`

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this pull request Jan 2, 2019
This is a followup branch from #2023:
- delete `proxy/client.go`, move code to `destination-client`
- move `RenderTapEvent` and stat functions from `util` to `cmd`

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants