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 node config for GKE node pool #184

Merged
merged 4 commits into from
Jul 31, 2017

Conversation

aadidenko
Copy link
Contributor

No description provided.

@j0hnsmith
Copy link

Looking forward to this getting merged.

Copy link
Contributor

@danawillow danawillow left a comment

Choose a reason for hiding this comment

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

Thanks for this @aadidenko! It mostly looks good to merge, just a few quick comments.

@@ -85,6 +87,34 @@ func resourceContainerNodePoolCreate(d *schema.ResourceData, meta interface{}) e
InitialNodeCount: int64(nodeCount),
}

if v, ok := d.GetOk("node_config"); ok {
nodeConfigs := v.([]interface{})
if len(nodeConfigs) > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this check, let's set MaxItems: 1 in the schema itself (and then we can get rid of this check in resource_container_cluster.go too)


nodePool.Config.OauthScopes = scopes
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the other fields? (local_ssd_count, service_account, metadata, image_type, labels, tags)

}
}`, acctest.RandString(10), acctest.RandString(10))

func nodepoolCheckMatch(attributes map[string]string, attr string, gcp interface{}) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typically we put helpers like this above the resource test vars

@danawillow
Copy link
Contributor

make testacc TEST=./google TESTARGS='-run=TestAccContainerNodePool'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerNodePool -timeout 120m
=== RUN   TestAccContainerNodePool_basic
--- PASS: TestAccContainerNodePool_basic (482.41s)
=== RUN   TestAccContainerNodePool_withNodeConfig
--- PASS: TestAccContainerNodePool_withNodeConfig (544.71s)
=== RUN   TestAccContainerNodePool_withNodeConfigScopeAlias
--- PASS: TestAccContainerNodePool_withNodeConfigScopeAlias (524.11s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	1551.389s

Looks good, thanks! I can merge this once the conflicts are resolved :)

- Set max items in node config schema
- Fill missing node config fields
- Put test helpers above than test vars
@aadidenko aadidenko force-pushed the gke-nodepool-node-config branch from 7a34f6b to c1236a3 Compare July 28, 2017 20:34
@danawillow danawillow merged commit 1ec19cf into hashicorp:master Jul 31, 2017
z1nkum pushed a commit to z1nkum/terraform-provider-google that referenced this pull request Aug 15, 2017
* Add support node config for GKE node pool

* Review fixes:
- Set max items in node config schema
- Fill missing node config fields
- Put test helpers above than test vars

* Update checks in node pool tests

* Fix node pool check match
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
* Add support node config for GKE node pool

* Review fixes:
- Set max items in node config schema
- Fill missing node config fields
- Put test helpers above than test vars

* Update checks in node pool tests

* Fix node pool check match
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
* Add support node config for GKE node pool

* Review fixes:
- Set max items in node config schema
- Fill missing node config fields
- Put test helpers above than test vars

* Update checks in node pool tests

* Fix node pool check match
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants