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

Make services/{admin, httpd, subscriber, udp} golintable #8058

Merged
merged 5 commits into from
Apr 5, 2017
Merged

Make services/{admin, httpd, subscriber, udp} golintable #8058

merged 5 commits into from
Apr 5, 2017

Conversation

karlding
Copy link
Contributor

@karlding karlding commented Feb 25, 2017

Make changes to:

  • services/admin
  • services/httpd
  • services/subscriber
  • services/udp

services/admin
admin/service.go:84:1: exported method Service.WithLogger should have comment or be unexported

services/httpd
response_writer.go:123:12: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

services/subscriber
subscriber/http.go:57:6: func createTlsConfig should be createTLSConfig

services/udp
service.go:23:2: don't use ALL_CAPS in Go names; use CamelCase

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@karlding karlding changed the title Make services/{admin, httpd, meta, subscriber, udp} golintable Make services/{admin, httpd, subscriber, udp} golintable Feb 25, 2017
Copy link
Contributor

@corylanou corylanou left a comment

Choose a reason for hiding this comment

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

Looks good. Can you update the changelog and give yourself credit as well.

Thanks!

@karlding
Copy link
Contributor Author

Alright. I rebased my branch on top of master, and made the change to the CHANGELOG. I thought it was such a trivial change that it didn't warrant a change to the CHANGELOG.

@jsternberg
Copy link
Contributor

Can you rebase this and update the changelog to resolve the conflict? I'll merge it right after. Thanks.

@karlding
Copy link
Contributor Author

karlding commented Apr 3, 2017

Rebased

@jsternberg jsternberg merged commit 4589586 into influxdata:master Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants