search

Azure / acr-builder

Azure Container Registry Build Runner
MIT License
38 stars 35 forks source link

fix build issue with linter upgrade #558

Closed akashsinghal closed 4 years ago

akashsinghal commented 4 years ago

Purpose of the PR

Fixes #533

msftclas commented 4 years ago

CLA assistant check
All CLA requirements met.

shahzzzam commented 4 years ago

LGTM for 1.20, can you post the errors from version v1.27.0?

akashsinghal commented 4 years ago

Here's the error message from staticcheck when upgrading to 1.27.0:

cmd/acb/commands/exec/exec.go:153:54: SA1029: should not use built-in type string as key for value; define your own type to avoid collisions (staticcheck) ctx := gocontext.WithValue(gocontext.Background(), "debug", debug)

Here's a few solutions I think will work for a fix:

  1. We could ignore the linter warning by adding a tag for this rule for this particular line. Basically the same way the other exec error was handled. I've tried this and it works and resolves the issue.
  2. Rewrite a few parts of the code so that we define a struct and use that instead. This should resolve the error but it will require changing the code in a few spots. This seems much more invasive.
shahzzzam commented 4 years ago

Thanks, I see.

Do you think we can remove putting the "debug" flag in the ctx altogether? it seems unnecessary and the bool debug can do the same job.

https://github.com/Azure/acr-builder/blob/cdd23eeaae850ac05df17588857589bf4b799bd7/cmd/acb/commands/exec/exec.go#L131