-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Add ability to set DockerClusterSpec load balancer #4951
Conversation
Looks like I'll need some pointers on the object conversion handling. |
7f51dda
to
c765c9a
Compare
c765c9a
to
e720e57
Compare
/retest |
e720e57
to
f44a080
Compare
@fabriziopandini it would be great to get your feedback on how I tied in using the image setting for the loadbalancer.Create() call. Just let me know if there are alternatives that would be preferable to how that was done. |
@vincepri I didn't follow your suggestion because I wasn't quite sure how that would fit here. But now that I have a working skeleton in place, maybe you could point out where you think it could be improved? |
@stmcginnis PR looks great to me @sbueringer PTAL |
@stmcginnis just a bunch of nits |
We need the ability to set the repository and tag of the load balancer to use. This adds a field to the DockerClusterSpec in the v1alpha4 API to enable this. Signed-off-by: Sean McGinnis <[email protected]>
f44a080
to
7264f91
Compare
Thx!! |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
What this PR does / why we need it:
We need the ability to set the repository and tag of the load balancer
to use. This adds a field to the DockerClusterSpec in the v1alpha4 API
to enable this.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #4950