Skip to content
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

Kubernetes: add support for node pool taints #854

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Sep 21, 2020

This change allows to set node pool taints during node pool create and update operations.

Refs digitalocean/DOKS#3

@guardrails
Copy link

@guardrails guardrails bot commented Sep 21, 2020

All previously detected findings have been fixed. Good job! 👍🎉

We will keep this comment up-to-date as you go along and notify you of any security issues that we identify.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@timoreimann timoreimann force-pushed the add-support-for-kubernetes-node-pool-taints branch 2 times, most recently from 829ac85 to a9da600 Compare Sep 21, 2020
@@ -337,6 +338,7 @@ func TestKubernetesList(t *testing.T) {

func TestKubernetesCreate(t *testing.T) {
testNodePool := testNodePool
Copy link
Member

@MorrisLaw MorrisLaw Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of testNodePool := testNodePool?

Copy link
Contributor Author

@timoreimann timoreimann Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already there when I touched the code :) I think it's meant to make a copy of testNodePool to work with that does not interfere with other tests. Although this only has limited effect since testNodePool includes slices and maps which are always copied by reference.

Probably something that can be addressed in a future PR (presumably by creating a function that returns a fresh, new testNodePool struct on each invocation).

Copy link
Member

@MorrisLaw MorrisLaw Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was just curious. But yeah, I guess that makes sense. Thank you for the clarification!

Copy link
Member

@kamaln7 kamaln7 Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added https://github.com/mitchellh/copystructure as a dependency in a recent PR that's been merged already so you could use that to do a deep copy if you wanted to!

@@ -337,6 +338,7 @@ func TestKubernetesList(t *testing.T) {

func TestKubernetesCreate(t *testing.T) {
testNodePool := testNodePool

Copy link
Member

@MorrisLaw MorrisLaw Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra whitespace?

Copy link
Contributor Author

@timoreimann timoreimann Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it on purpose to provide for a better visual separation between initializations of more "complex" structs (namely labels and taints).

Copy link
Member

@MorrisLaw MorrisLaw Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sgtm!

go.mod Outdated
@@ -52,3 +52,5 @@ require (
replace github.com/stretchr/objx => github.com/stretchr/objx v0.2.0

replace golang.org/x/crypto => golang.org/x/crypto v0.0.0-20200420201142-3c4aac89819a

replace github.com/digitalocean/godo => github.com/timoreimann/godo v1.7.4-0.20200921135118-82e120c0a69c
Copy link
Member

@MorrisLaw MorrisLaw Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is adding github.com/timoreimann/godo just for testing?

Copy link
Contributor Author

@timoreimann timoreimann Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- see the PR description about the temporary nature of the associated commit.

@timoreimann timoreimann force-pushed the add-support-for-kubernetes-node-pool-taints branch 2 times, most recently from 60ad833 to e6b58c4 Compare Sep 23, 2020
@timoreimann
Copy link
Contributor Author

@timoreimann timoreimann commented Sep 23, 2020

CI is failing because of the breaking change in godo at digitalocean/godo#367. My suggestion would be that we submit another PR to update the Apps consumer in doctl to the godo changes once godo is released and bumped in doctl; afterwards, this PR can be rebased and should go green.

@timoreimann timoreimann force-pushed the add-support-for-kubernetes-node-pool-taints branch from e6b58c4 to b93a049 Compare Sep 25, 2020
@kamaln7 kamaln7 mentioned this pull request Sep 25, 2020
This change allows to set node pool taints during node pool create and
update operations.
@timoreimann timoreimann force-pushed the add-support-for-kubernetes-node-pool-taints branch from b93a049 to ae9567c Compare Sep 26, 2020
@timoreimann
Copy link
Contributor Author

@timoreimann timoreimann commented Sep 26, 2020

PR has been rebased onto the latest doctl master which has godo updated to v1.45.0.

Thanks for the help @kamaln7 👏

Copy link
Member

@andrewsomething andrewsomething left a comment

LGTM!

@andrewsomething andrewsomething merged commit e7aeb04 into digitalocean:master Sep 28, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants