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.

Sign up for GitHub

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

Jump to bottom

Kubernetes: add support for node pool taints #854

Merged
merged 1 commit into from
Sep 28, 2020
Merged

Kubernetes: add support for node pool taints #854

merged 1 commit into from
Sep 28, 2020

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Sep 21, 2020 •
edited
Loading

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

Refs digitalocean/DOKS#3

@guardrails
Copy link

guardrails bot commented Sep 21, 2020 •
edited
Loading

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 September 21, 2020 14:57
MorrisLaw
MorrisLaw reviewed Sep 21, 2020
View reviewed changes
commands/kubernetes_test.go
@@ -337,6 +338,7 @@ func TestKubernetesList(t *testing.T) {

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

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 •
edited
Loading

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
Contributor

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
Contributor

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!

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

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

Copy link
Contributor

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

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
Contributor

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
Contributor

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

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 September 23, 2020 10:34
@timoreimann
Copy link
Contributor Author

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.

@andrewsomething andrewsomething mentioned this pull request Sep 24, 2020
Merged
@timoreimann timoreimann force-pushed the add-support-for-kubernetes-node-pool-taints branch from e6b58c4 to b93a049 Compare September 25, 2020 16:16
@kamaln7 kamaln7 mentioned this pull request Sep 25, 2020
Merged
Kubernetes: add support for node pool taints
ae9567c 
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 September 26, 2020 05:26
@timoreimann
Copy link
Contributor Author

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

Thanks for the help @kamaln7 👏

andrewsomething
andrewsomething approved these changes Sep 28, 2020
View reviewed changes
Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrewsomething andrewsomething merged commit e7aeb04 into digitalocean:master Sep 28, 2020
@timoreimann timoreimann deleted the add-support-for-kubernetes-node-pool-taints branch July 23, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisLaw MorrisLaw MorrisLaw left review comments

@kamaln7 kamaln7 kamaln7 left review comments

@andrewsomething andrewsomething andrewsomething approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timoreimann @andrewsomething @kamaln7 @MorrisLaw