-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Replace BuildTestContainer
with use of builder
#5896
Replace BuildTestContainer
with use of builder
#5896
Conversation
/retest Test failure seems unrelated to the PR:
|
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.
Looks good to me
One idea I have is that we're calling resource.MustParse
each time we call WithMemRequest
and similar functions (in existing code too). What do you think about changing the builder to take a string input instead?
(this is a strict improvement so if you think it's better to merge as-is let's do that)
/retest |
If we always use a string for the quantity it makes sense to simplify, agreed. |
Retest didn't trigger, trying again. /retest |
@kwiesmueller: No presubmit jobs available for kubernetes/autoscaler@master In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
73adf4c
to
db507cd
Compare
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbartosik, kwiesmueller The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
It seems so to me. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Resolves a code TODO by replacing uses of
BuildTestContainer
with use of the builder directly.Which issue(s) this PR fixes:
Fixes code TODO
Special notes for your reviewer:
I was resolving the TODO and generally think it might have helped clean things up, but would like to get some feedback if we all agree this change makes sense.
I can see it both ways that having this helper made sense. If so we should just remove the TODO instead.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @jbartosik