-
Notifications
You must be signed in to change notification settings - Fork 9.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
[WIP] provider/google: Update Container Engine features #5871
Conversation
Would love feedback on builtin/providers/google/resource_container_cluster.go as it's my first introduction to the schema model. I'm sure it can be optimized. Like, it may be a bit nesty. Thoughts, @lwander? Here's a template using the two new properties:
|
if v, ok := addonsConfig["http_load_balancing"]; ok { | ||
addon := v.([]interface{})[0].(map[string]interface{}) | ||
cluster.AddonsConfig.HttpLoadBalancing = &container.HttpLoadBalancing{ | ||
Disabled: addon["disabled"].(bool), |
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.
This will default to false
if omitted, since you didn't specify this subattribute as required - is that intended behavior? Same thing below on line 310.
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.
That's correct. These two addons are enabled by default (i.e., not disabled). A user would create an addons_config to set disabled to true. Weird, huh?
Hey @phinze, do we normally checkin the vendor directory? |
we do, but in a separate commit that usually has a descriptive commit message to make the diff easier to read. See https://github.com/hashicorp/terraform#adding-a-dependency |
a52e56d
to
4167c9f
Compare
1. Vendored google.golang.org/api packages were updated
4167c9f
to
c6763fd
Compare
OK, I broke out the vendoring change into a separate commit. Also, @lwander, I removed the Appreciate any more comments or an LGTM/merge when folks have a moment. Thanks! |
LGTM! Nice work |
Type: schema.TypeList, | ||
Optional: true, | ||
ForceNew: true, | ||
MaxItems: 1, |
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.
Why Maxitems: 1
? When I try:
addons_config: {
http_load_balancing: {
disabled: false
},
horizontal_pod_autoscaling: {
disabled: false
}
},
it errors: addons_config: attribute supports 1 item maximum, config has 2 declared
@devth, I was able to deploy the following manifest using
|
I simplified my config down to: {
"provider": {
"google": {
"credentials": "mycreds",
"project": "myproject",
"region": "us-central1"
}
},
"resource": {
"google_container_cluster": {
"test-addons-config": {
"name": "test-addons-config",
"addons_config": {
"http_load_balancing": {
"disabled": false
},
"horizontal_pod_autoscaling": {
"disabled": false
}
},
"initial_node_count": 2,
"master_auth": {
"password": "foo",
"username": "admin"
},
"node_config": {
"machine_type": "n1-standard-8",
"oauth_scopes": [
"https://www.googleapis.com/auth/compute",
"https://www.googleapis.com/auth/devstorage.read_only",
"https://www.googleapis.com/auth/logging.write",
"https://www.googleapis.com/auth/monitoring"
]
},
"zone": "us-central1-a"
}
}
}
}
If I remove either |
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This change introduces support for subnetworks and addon configurations
in a Google Container Engine cluster: