-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Spotinst: Support for Headroom, Taints and Subnets in Ocean Launch Spec and User Data fixes #8294
Spotinst: Support for Headroom, Taints and Subnets in Ocean Launch Spec and User Data fixes #8294
Conversation
Hi @liranp. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test |
@liranp would you mind splitting the commits up a bit? This is a pretty large change (vendoring makes sense) but usually vendored code a standalone commit is easier for reviewers |
bb016ca
to
056ed17
Compare
056ed17
to
f2aceb2
Compare
@joshbranham Done. Please let me know if you want me to split more. Thanks for reviewing this! |
f2aceb2
to
ea9ba8d
Compare
ea9ba8d
to
f8f284c
Compare
f8f284c
to
b800c38
Compare
63d988e
to
490930d
Compare
84d5e0c
to
c9d78e9
Compare
cdb8bf2
to
84e96bd
Compare
/test pull-kops-e2e-kubernetes-aws |
go.mod
Outdated
@@ -139,6 +139,7 @@ require ( | |||
k8s.io/helm v2.9.0+incompatible | |||
k8s.io/klog v1.0.0 | |||
k8s.io/kubectl v0.0.0 | |||
k8s.io/kubernetes v1.17.3 |
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.
We've been working towards dropping k8s.io/kubernetes as a dependency (example: #8055 and #8056) since it can be a nightmare to update with each minor version and all of the transitive dependencies.
Seeing as you're only using the ParseTaints
function and there doesn't appear to be any effort to move it to one of the other k8s.io repos, maybe we consider copying the function directly into the kops repo? It looks like ParseTaints and two unexported functions might be all that is needed.
@justinsb what do you think? You can also provide more context on shifting away from depending on k/k.
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.
I've removed the k8s.io/kubernetes
dependency and extracted the relevant parts only.
debd1de
to
4244bb8
Compare
4244bb8
to
7b796cd
Compare
a8bb7eb
to
a0ec9ee
Compare
a0ec9ee
to
2a17a50
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liranp, rifelpet 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 |
This PR adds support for Headroom, Taints and Subnets in Ocean Launch Spec and minor User Data fixes.