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

Add support for setting node pool metadata key/value pairs. #47

Conversation

czka
Copy link

@czka czka commented Dec 19, 2018

No description provided.

@czka
Copy link
Author

czka commented Dec 19, 2018

Should I have squashed my commits?

@morgante morgante self-requested a review December 19, 2018 23:21
@morgante
Copy link
Contributor

@czka Do you mind adding some testing for this new parameter? Probably on the node_pool fixture: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/tree/master/test/integration/node_pool/controls

Rebasing is preferred to squashing or merging, but not strongly required.

Thanks for the contribution!

@czka
Copy link
Author

czka commented Dec 20, 2018

@morgante I force-pushed a nice rebased branch for the PR.

Will you not accept my PRs without tests? I looked into that. It's new thing to learn for me and I don't have an easy way to run them myself. I could hack in something I think would be valid, but won't be able to make sure it is. At this moment it's an effort I'd rather avoid, and focus on fixing/extending the module to our needs. This PR (and probably few more in the coming months) is a part of the company I work for evaluating your module as a replacement for their home-grown TF code.

FWIW, an example node_config.*.metadata.shutdown-script might be:

#!/bin/bash -e

kubectl --kubeconfig=/var/lib/kubelet/kubeconfig drain --force=true --ignore-daemonsets=true --delete-local-data $HOSTNAME

@morgante
Copy link
Contributor

@czka We're generally trying to make sure new features have good test coverage. I will see if someone else can add the tests for this, but if not I will merge as is.

@Jberlinsky
Copy link
Contributor

Jberlinsky commented Jan 3, 2019

@czka Thank you for your contribution! Like @morgante mentioned, we'd like to add some test coverage to this pull request. Now that we have continuous integration set up, running the tests should be pretty straightforward.

To move this forward, I'm going to merge this pull request into the feature/czka-node-pool-metadata-key-value-pairs branch and add test coverage there. I'll submit a final PR from that branch into master.

@Jberlinsky Jberlinsky changed the base branch from master to feature/czka-node-pool-metadata-key-value-pairs January 3, 2019 19:54
@Jberlinsky Jberlinsky merged commit 1592559 into terraform-google-modules:feature/czka-node-pool-metadata-key-value-pairs Jan 3, 2019
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

3 participants