-
Notifications
You must be signed in to change notification settings - Fork 262
Adds support for tectonic_etcd_scheme #2273
Conversation
|
Can one of the admins verify this patch? |
squat
left a comment
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 suffice as a temporary fix. We really need to infer the scheme from the certs but we cannot do this until we allow certificates for external etcd clusters. Please also add configuration for openstack clusters.
config.tf
Outdated
| default = "https" | ||
|
|
||
| description = <<EOF | ||
| (optional) Can be either "http" or "https" When set this scheme will be used for all provided etcd endpoints. |
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.
Please add a “.” between the two sentences and add a “,” after “When set” so it reads better.
| master_subnetwork_name = "${module.network.master_subnetwork_name}" | ||
| external_endpoints = ["${compact(var.tectonic_etcd_servers)}"] | ||
|
|
||
| etcd_scheme = "${var.tectonic_etcd_scheme}" |
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.
We need to declare “etcd” in platforms/gcp/etcd otherwise GCP will fail to build.
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
| container_image = "${var.tectonic_container_images["etcd"]}" | ||
| base_domain = "${var.tectonic_base_domain}" | ||
| external_endpoints = ["${compact(var.tectonic_etcd_servers)}"] | ||
| etcd_scheme = "${var.tectonic_etcd_scheme}" |
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.
Same here, let’s declare etcd in platforms/vmware/etcd so as to not break this platform.
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
|
We did some changes (#2082) to the testing process. Please rebase on to current master, so that the |
This adds support for specifying the scheme that will be used for the external etcd cluster. it retains the default `https` I have tested bare metal azure and aws.
5c21492 to
970e463
Compare
|
We can't infer https only from certs. There are a few ways to configure etcd.
|
|
closed in favor of #2288 |
This adds support for specifying the scheme that will be used for the
external etcd cluster. it retains the default
httpsI have tested bare metal azure and aws. @squat @kyoto