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

app-mapper: govet, golint, errcheck fixes #83

Merged
merged 2 commits into from
Aug 14, 2015
Merged

app-mapper: govet, golint, errcheck fixes #83

merged 2 commits into from
Aug 14, 2015

Conversation

peterbourgon
Copy link

@tomwilkie tomwilkie mentioned this pull request Aug 13, 2015
Merged
@peterbourgon
Copy link
Author

@2opremio Friendly ping :)

2opremio
2opremio reviewed Aug 14, 2015
View reviewed changes
app-mapper/authenticator_test.go
@@ -142,7 +142,9 @@ func TestBadServerResponse(t *testing.T) {

shFunc := func(w http.ResponseWriter, r *http.Request, orgName string) {
w.WriteHeader(http.StatusOK)
w.Write(responseBody)
if _, err := w.Write(responseBody); err != nil {
t.Error(err)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio
2opremio reviewed Aug 14, 2015
View reviewed changes
app-mapper/app_provisioner.go
@@ -62,7 +63,9 @@ func (p *dockerProvisioner) runApp(appID string) (hostname string, err error) {
}
defer func() {
if err != nil {
p.destroyApp(appID)
if err := p.destroyApp(appID); err != nil {
logrus.Warnf("destroyApp: %v", err)

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio
Copy link

Thanks for improving the error handling @peterbourgon !

@2opremio 2opremio assigned peterbourgon and unassigned 2opremio Aug 14, 2015
@peterbourgon
Copy link
Author

@2opremio all good?

FYI, here's what happens when I press the build shortcut in my editor:

go test -i && echo deps OK && \
 go build -v && echo build OK && \
 go test && echo test OK && \
 go vet && echo vet OK && \
 golint && echo lint OK && \
 errcheck -ignore=Close && echo errcheck OK

edit: I realize that can be condensed to

go test -i && echo deps OK && \
 go build -v && echo build OK && \
 go test && echo test OK && \
 gometalinter --exclude="error return value not checked.*Close" && echo metalint OK

@2opremio
Copy link

Yep, LGTM.

I didn't know about errcheck, my pre-commit script so far has been simply

#!/bin/bash

set -e

fgt golint app-mapper
go tool vet app-mapper

(I always run tests manually). I will incorporate errcheck to it, thanks.

peterbourgon added a commit that referenced this pull request Aug 14, 2015
@peterbourgon
Merge pull request #83 from weaveworks/vet-lint-fixes
ab8dbc8 
app-mapper: govet, golint, errcheck fixes
@peterbourgon peterbourgon merged commit ab8dbc8 into master Aug 14, 2015
@peterbourgon peterbourgon deleted the vet-lint-fixes branch August 14, 2015 13:57
@peterbourgon peterbourgon removed their assignment Jan 2, 2017
bboreham added a commit that referenced this pull request Jan 28, 2018
@bboreham
Update weaveworks/common for buckets and error-logging changes
c5450c0 
This brings in the merged commits for:
(#82 was relaced by #83)
bboreham added a commit that referenced this pull request Jan 28, 2018
@bboreham
Update weaveworks/common for buckets and error-logging changes
608b348 
This brings in the merged commits for:
- #81 "Extend histogram buckets for a server up to 100 seconds"
- #83 "Let servers set any gRPC options they like "
- #84 "Don't log request body and headers for 502 errors"
(#82 was relaced by #83)
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peterbourgon @2opremio