WIP: Remove redundant replace directives #227
Conversation
643b72b to
485f967
Compare
|
This should be ready to review now @wpjunior, thanks! |
The replace directives for the packages github.com/ajg/form and github.com/samalba/dockerclient are both redundant. We can require github.com/cezarsa/form directly and bypass the replace. gofmt -w -r '"github.com/ajg/form" -> "github.com/cezarsa/form"' . The second replaced package does not seem to be in use according to `go list -m all`. This also requires a recent enough commit of tsuru after tsuru/tsuru!2703. There is a hidden bonus that `go install github.com/tsuru/tsuru-client/tsuru@latest` is now a thing.
485f967 to
a6b4018
Compare
|
Updating Tsuru to the latest version introduced some CI failures that are unrelated to the changes here. I managed to get the tests to pass again, but I'm not completely sure if the changes are correct. I will leave some comments in these places in the diff. |
| if p.CPUBurst != nil { | ||
| maxAllowed = p.CPUBurst.MaxAllowed | ||
| } | ||
| maxCPULimit := resource.NewMilliQuantity(int64(float64(p.CPUMilli)*maxAllowed), resource.DecimalSI).String() |
There was a problem hiding this comment.
@wpjunior I'm not sure if these changes are correct. Basically in tsuru latest Plan.CPUBurst and Plan.Override are now pointers so we have to check for a nil dereference. All the fixes here were panicking the tests, I will do a code search for the use of these types and update accordingly.
There was a problem hiding this comment.
@mauri870 just a adjustment: maxAllowed default is 1.0, just this adjustment to allow this MR.
There was a problem hiding this comment.
Thanks, I fixed it. I'm pretty sure there are more cases where we can get panics if we do not check for nil. If you feel like this is fine you can merge this as-is and fix these later. I'd advise to search for the uses of Override and CPUBurst and add the nil checks as needed.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #227 +/- ##
==========================================
+ Coverage 70.89% 72.66% +1.77%
==========================================
Files 55 54 -1
Lines 8809 9670 +861
==========================================
+ Hits 6245 7027 +782
- Misses 1675 1748 +73
- Partials 889 895 +6 ☔ View full report in Codecov by Sentry. |
23ded43 to
6ecaa38
Compare
The replace directives for the packages
github.com/ajg/formandgithub.com/samalba/dockerclientare both redundant.We can require
github.com/cezarsa/formdirectly and bypass the replace.The second replaced package does not seem to be in use according to
go list -m all.This also requires a recent enough commit of tsuru after tsuru/tsuru#2703.
There is a hidden bonus that
go install github.com/tsuru/tsuru-client/tsuru@latestis now a thing.