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

Jump to bottom

add errcheck support #95

Closed
wants to merge 2 commits into from
Closed

add errcheck support #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c65ca87
add errcheck support
maleck13 May 25, 2018
6e34458
fix build
maleck13 May 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .travis.yml
View file
Open in desktop
Expand Up @@ -6,11 +6,13 @@ sudo: required
before_script:
- "go get -u golang.org/x/lint/golint"
- "go get -u github.com/golang/dep/cmd/dep"
- "go get github.com/kisielk/errcheck"
- "sudo mkdir -p /var/run/secrets/kubernetes.io/serviceaccount && sudo touch /var/run/secrets/kubernetes.io/serviceaccount/token"
script:
- "make fmtcheck"
- "make vet"
- "make lint"
- "make ensure"
- "make errcheck"
- "make build"
- "KUBERNETES_SERVICE_HOST=localhost KUBERNETES_SERVICE_PORT=8000 make test"
8 changes: 6 additions & 2 deletions Makefile
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ COVERAGE_SVC := travis-ci
.DEFAULT_GOAL := build

ensure: ## Install or update project dependencies
@go get github.com/kisielk/errcheck
@dep ensure

build: $(SOURCES) ## Build Test
Expand Down Expand Up @@ -39,7 +40,7 @@ test-coverage-html: coverage-all.out ## Check out the test coverage locally
ci-test-coverage: coverage-all.out ## CI test coverage, upload to coveralls
@goveralls -coverprofile=coverage-all.out -service $(COVERAGE_SVC)

check: fmtcheck vet lint build test ## Pre-flight checks before creating PR
check: fmtcheck errcheck vet lint build test ## Pre-flight checks before creating PR

clean: ## Clean up your working environment
@rm -f coverage-all.out coverage.out
Expand All @@ -48,6 +49,9 @@ generate: ## regenerate mocks
go get github.com/vektra/mockery/.../
@go generate ./...

errcheck:
@errcheck -ignoretests $$(go list ./... | grep -v mocks)

help: ## Show this help screen
@echo 'Usage: make <OPTIONS> ... <TARGETS>'
@echo ''
Expand All @@ -56,4 +60,4 @@ help: ## Show this help screen
@grep -E '^[ a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | \
awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-20s\033[0m %s\n", $$1, $$2}'

.PHONY: ensure build lint fmt fmtcheck test vet check help test-coverage-html clean
.PHONY: ensure build lint fmt fmtcheck test vet check help test-coverage-html clean errcheck
6 changes: 5 additions & 1 deletion clients/etcd.go
View file
Open in desktop
Expand Up @@ -58,7 +58,11 @@ func GetEtcdVersion(ec EtcdConfig) (string, string, error) {
return "", "", err
}

defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()
body, _ := ioutil.ReadAll(resp.Body)

switch resp.StatusCode {
Expand Down
6 changes: 5 additions & 1 deletion registries/adapters/adapter.go
View file
Open in desktop
Expand Up @@ -69,7 +69,11 @@ func (rre *registryResponseError) Error() string {
}

func registryResponseHandler(resp *http.Response) ([]byte, error) {
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return nil, err
Expand Down
27 changes: 21 additions & 6 deletions registries/adapters/dockerhub_adapter.go
View file
Open in desktop
Expand Up @@ -149,7 +149,11 @@ func (r DockerHubAdapter) getDockerHubToken() (string, error) {
if err != nil {
return "", err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

jsonToken, err := ioutil.ReadAll(resp.Body)

Expand Down Expand Up @@ -183,7 +187,11 @@ func (r DockerHubAdapter) getNextImages(ctx context.Context,
close(ch)
return nil, err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

imageList, err := ioutil.ReadAll(resp.Body)

Expand All @@ -199,7 +207,11 @@ func (r DockerHubAdapter) getNextImages(ctx context.Context,
if iResp.Next != "" {
log.Debugf("getting next page of results - %v", iResp.Next)
// Fan out calls to get the next images.
go r.getNextImages(ctx, org, token, iResp.Next, ch, cancelFunc)
go func() {
if _, err := r.getNextImages(ctx, org, token, iResp.Next, ch, cancelFunc); err != nil {
log.Warn("error occurred calling getNextImages : %s", err)
}
}()
}
for _, imageName := range iResp.Results {
log.Debugf("Trying to load %v/%v", imageName.Namespace, imageName.Name)
Expand Down Expand Up @@ -265,12 +277,15 @@ func (r DockerHubAdapter) getBearerToken(imageName string) (string, error) {
if err != nil {
return "", err
}
defer response.Body.Close()
defer func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we doing this because we added the error check to the check command in the makefile?

I actually don't like adding these go routines just because of a tool. Wondering we can just add this to the make file, and not add it to CI. The tool is useful for checking the code, but I don't know if we want to add this code when we really do want to ignore the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say adding go routines, I am not sure I follow do you mean the additional function call as we are really are just deferring one function call over another. Originally we were deferring the Close method where as now we are deferring an anonymous function that calls the Close method. There are no additional go routines created?

I know where you are coming from with ignoring the error when closing the response body, but do we actually want to ignore it? What is the downside to not ignoring it?
I do think there is value in having the err check tool run as part of the PR process as it gives us the confidence that any new code has at least handled any possible errors. I think a couple of additional err checks as a cost is not that high

Copy link
Contributor

Choose a reason for hiding this comment

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

the defer func() solely for errcheck is annoying because you really don't need to check the err on a close() it just gets ignored. What are we going to do if the response body doesn't get closed? If this ends up logging a bunch of warnings to the broker log because of errors that we would've ignored before I will ask we remove these. For now, I'll let it stand.

if err := response.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()
t := struct {
Token string `json:"token"`
}{}
err = json.NewDecoder(response.Body).Decode(&t)
if err != nil {
if err := json.NewDecoder(response.Body).Decode(&t); err != nil {
return "", err
}
return t.Token, nil
Expand Down
19 changes: 16 additions & 3 deletions registries/adapters/helm_adapter.go
View file
Open in desktop
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/Masterminds/semver"
"github.com/automationbroker/bundle-lib/bundle"
"github.com/ghodss/yaml"
log "github.com/sirupsen/logrus"
)

const (
Expand Down Expand Up @@ -193,7 +194,11 @@ func (r *HelmAdapter) FetchSpecs(imageNames []string) ([]*bundle.Spec, error) {
if err != nil {
continue
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

values = r.loadArchive(resp.Body)
}
Expand Down Expand Up @@ -277,7 +282,11 @@ func (r *HelmAdapter) getHelmIndex() (*IndexFile, error) {
if err != nil {
return index, err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

body, err := ioutil.ReadAll(resp.Body)
if err != nil {
Expand Down Expand Up @@ -305,7 +314,11 @@ func (r *HelmAdapter) loadArchive(in io.Reader) string {
if err != nil {
return ""
}
defer unzipped.Close()
defer func() {
if err := unzipped.Close(); err != nil {
log.Warn("failed to close the zip reader : %s", err)
}
}()

tr := tar.NewReader(unzipped)
for {
Expand Down
12 changes: 10 additions & 2 deletions registries/adapters/openshift_adapter.go
View file
Open in desktop
Expand Up @@ -94,7 +94,11 @@ func (r OpenShiftAdapter) getOpenShiftAuthToken() (string, error) {
if err != nil {
return "", err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

// Ensure that response holds data we expect
if resp.Header.Get("Www-Authenticate") == "" {
Expand Down Expand Up @@ -125,7 +129,11 @@ func (r OpenShiftAdapter) getOpenShiftAuthToken() (string, error) {
if err != nil {
return "", err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

tokenResp := TokenResponse{}
err = json.NewDecoder(resp.Body).Decode(&tokenResp)
Expand Down
18 changes: 15 additions & 3 deletions registries/adapters/partner_rhcc_adapter.go
View file
Open in desktop
Expand Up @@ -68,7 +68,11 @@ func (r PartnerRhccAdapter) GetImageNames() ([]string, error) {
log.Errorf("Failed to load catalog response at %s - %v", partnerCatalogURL, err)
return nil, err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

if resp.StatusCode != 200 {
log.Errorf("Failed to fetch catalog response. Expected a 200 status and got: %v", resp.Status)
Expand Down Expand Up @@ -121,7 +125,11 @@ func (r PartnerRhccAdapter) getAuthToken() (string, error) {
if err != nil {
return "", err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

// Ensure that response holds data we expect
if resp.Header.Get("Www-Authenticate") == "" {
Expand Down Expand Up @@ -152,7 +160,11 @@ func (r PartnerRhccAdapter) getAuthToken() (string, error) {
if err != nil {
return "", err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

tokenResp := TokenResponse{}
err = json.NewDecoder(resp.Body).Decode(&tokenResp)
Expand Down
6 changes: 5 additions & 1 deletion registries/adapters/rhcc_adapter.go
View file
Open in desktop
Expand Up @@ -101,7 +101,11 @@ func (r RHCCAdapter) loadImages(Query string) (RHCCImageResponse, error) {
if err != nil {
return RHCCImageResponse{}, err
}
defer resp.Body.Close()
defer func() {
if err := resp.Body.Close(); err != nil {
log.Warn("failed to close response body : %s", err)
}
}()

if resp.StatusCode != 200 {
return RHCCImageResponse{}, errors.New(resp.Status)
Expand Down
4 changes: 3 additions & 1 deletion runtime/runtime.go
View file
Open in desktop
Expand Up @@ -415,7 +415,9 @@ func (p provider) DestroySandbox(podName string,
if shouldDeleteNamespace(keepNamespace, keepNamespaceOnError, pod, err) {
if configNamespace != namespace {
log.Debugf("Deleting namespace %s", namespace)
k8scli.Client.CoreV1().Namespaces().Delete(namespace, &metav1.DeleteOptions{})
if err := k8scli.Client.CoreV1().Namespaces().Delete(namespace, &metav1.DeleteOptions{}); err != nil {
log.Errorf("Unable to delete namespace: %s : %v", namespace, err)
}
// This is keeping track of namespaces.
} else {
// We should not be attempting to run pods in the ASB namespace, if we are, something is seriously wrong.
Expand Down