WIP: Remove redundant replace directives #227
Conversation
mauri870
force-pushed
the
feature/remove-replace
branch
from
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.
mauri870
force-pushed
the
feature/remove-replace
branch
from
485f967
to
a6b4018
Compare
mauri870
changed the title
|
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. |
mauri870
force-pushed
the
feature/remove-replace
branch
from
23ded43
to
6ecaa38
Compare
The replace directives for the packages
github.com/ajg/formandgit.luolix.top/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.