-
Notifications
You must be signed in to change notification settings - Fork 76
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mauri870 just a adjustment: maxAllowed default is 1.0, just this adjustment to allow this MR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/form
andgithub.com/samalba/dockerclient
are both redundant.We can require
github.com/cezarsa/form
directly 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@latest
is now a thing.