-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added variable skip_provisioners
to skip 'local-exec'
#278
Added variable skip_provisioners
to skip 'local-exec'
#278
Conversation
4de5836
to
90f64f4
Compare
90f64f4
to
8263a68
Compare
README.md
Outdated
## Upgrade to v3.0.0 | ||
|
||
v3.0.0 is a breaking release. Refer to the | ||
[Upgrading to v3.0 guide][upgrading-to-v3.0] for details. | ||
|
||
## Upgrade to v2.0.0 | ||
|
||
v2.0.0 is a breaking release. Refer to the | ||
[Upgrading to v2.0 guide][upgrading-to-v2.0] for details. | ||
|
||
## Upgrade to v1.0.0 | ||
|
||
Version 1.0.0 of this module introduces a breaking change: adding the `disable-legacy-endpoints` metadata field to all node pools. This metadata is required by GKE and [determines whether the `/0.1/` and `/v1beta1/` paths are available in the nodes' metadata server](https://cloud.google.com/kubernetes-engine/docs/how-to/protecting-cluster-metadata#disable-legacy-apis). If your applications do not require access to the node's metadata server, you can leave the default value of `true` provided by the module. If your applications require access to the metadata server, be sure to read the linked documentation to see if you need to set the value for this field to `false` to allow your applications access to the above metadata server paths. | ||
|
||
In either case, upgrading to module version `v1.0.0` will trigger a recreation of all node pools in the cluster. | ||
|
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 it's showing up as new change?
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.
Thank you for notice.
Removed old upgrading guides all README's. In addition to commit
autogen/dns.tf
Outdated
@@ -20,7 +20,7 @@ | |||
Delete default kube-dns configmap | |||
*****************************************/ | |||
resource "null_resource" "delete_default_kube_dns_configmap" { | |||
count = local.custom_kube_dns_config || local.upstream_nameservers_config ? 1 : 0 | |||
count = (local.custom_kube_dns_config || local.upstream_nameservers_config) || var.skip_provisioners ? 1 : 0 |
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.
need to add explanation of this in skip_provisioners var description
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.
Updated description of the variable.
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 logic is wrong.
If you skip provisioners, no local-execs should ever be initialized.
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.
Fixed
0e0defd
to
8727e84
Compare
.kitchen.yml
Outdated
@@ -131,3 +131,10 @@ suites: | |||
systems: | |||
- name: workload_metadata_config | |||
backend: local | |||
- name: "simple_regional_skip_local_exec" |
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 doesn't need to be a separate suite, just add it to one of the existing ones.
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.
Moved to simple_regional
autogen/README.md
Outdated
@@ -122,22 +122,6 @@ Then perform the following commands on the root folder: | |||
- `terraform apply` to apply the infrastructure build | |||
- `terraform destroy` to destroy the built infrastructure | |||
|
|||
## Upgrade to v3.0.0 |
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 did you remove this? Please rebase.
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.
rebased
autogen/cluster.tf
Outdated
@@ -352,6 +352,7 @@ resource "google_container_node_pool" "pools" { | |||
} | |||
|
|||
resource "null_resource" "wait_for_cluster" { | |||
count = var.skip_provisioners ? 1 : 0 |
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.
If you skip provisioners, shouldn't this be count of 0? Please pay attention to quality of work and correct logic.
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.
FIxed
autogen/dns.tf
Outdated
@@ -20,7 +20,7 @@ | |||
Delete default kube-dns configmap | |||
*****************************************/ | |||
resource "null_resource" "delete_default_kube_dns_configmap" { | |||
count = local.custom_kube_dns_config || local.upstream_nameservers_config ? 1 : 0 | |||
count = (local.custom_kube_dns_config || local.upstream_nameservers_config) || var.skip_provisioners ? 1 : 0 |
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 logic is wrong.
If you skip provisioners, no local-execs should ever be initialized.
dc2448d
to
c13e003
Compare
autogen/README.md
Outdated
@@ -201,6 +201,7 @@ In either case, upgrading to module version `v1.0.0` will trigger a recreation o | |||
| resource\_usage\_export\_dataset\_id | The dataset id for which network egress metering for this cluster will be enabled. If enabled, a daemonset will be created in the cluster to meter network egress traffic. | string | `""` | no | | |||
| sandbox\_enabled | (Beta) Enable GKE Sandbox (Do not forget to set `image_type` = `COS_CONTAINERD` and `node_version` = `1.12.7-gke.17` or later to use it). | bool | `"false"` | no | | |||
| service\_account | The service account to run nodes as if not overridden in `node_pools`. The create_service_account variable default value (true) will cause a cluster-specific service account to be created. | string | `""` | no | | |||
| skip\_provisioners | Flag to skip local-exec provisioners. Does not affect if `stub_domains` or `upstream_nameservers` variable set. | bool | `"false"` | no | |
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.
change this one
0dca148
to
198b191
Compare
* Fix terraform-google-modules#258 * Added test `simple_regional_skip_local_exec` * Remove old upgrading guide from README's
198b191
to
9c66273
Compare
autogen/variables.tf
Outdated
@@ -311,6 +311,11 @@ variable "cluster_resource_labels" { | |||
default = {} | |||
} | |||
|
|||
variable "skip_provisioners" { | |||
type = bool | |||
description = "Flag to skip all local-exec provisioners. It breaks down `stub_domains` and `upstream_nameservers` variables functionality." |
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.
description = "Flag to skip all local-exec provisioners. It breaks down `stub_domains` and `upstream_nameservers` variables functionality." | |
description = "Flag to skip all local-exec provisioners. It breaks `stub_domains` and `upstream_nameservers` variables functionality." |
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.
updated
da4b297
to
53ec7a9
Compare
scripts/wait-for-cluster.sh
Outdated
@@ -15,10 +15,6 @@ | |||
|
|||
set -e | |||
|
|||
if [ -n "${GOOGLE_APPLICATION_CREDENTIALS}" ]; then |
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 are you removing this?
…add_skip_provisioners_variable_to_skip_local-exec Added variable `skip_provisioners` to skip 'local-exec'
simple_regional_skip_local_exec