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

Avoid nested objects with a single optional field #3928

Closed
danawillow opened this issue Jun 26, 2019 · 6 comments
Closed

Avoid nested objects with a single optional field #3928

danawillow opened this issue Jun 26, 2019 · 6 comments

Comments

@danawillow
Copy link
Contributor

For example, GKE addons_config has a bunch of nested objects with a single boolean field inside them:

			"addons_config": {
				Type:     schema.TypeList,
				Optional: true,
				Computed: true,
				MaxItems: 1,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"http_load_balancing": {
							Type:     schema.TypeList,
							Optional: true,
							Computed: true,
							MaxItems: 1,
							Elem: &schema.Resource{
								Schema: map[string]*schema.Schema{
									"disabled": {
										Type:     schema.TypeBool,
										Optional: true,
									},
								},
							},
						},
						"horizontal_pod_autoscaling": {
							Type:     schema.TypeList,
							Optional: true,
							Computed: true,
							MaxItems: 1,
							Elem: &schema.Resource{
								Schema: map[string]*schema.Schema{
									"disabled": {
										Type:     schema.TypeBool,
										Optional: true,
									},
								},
							},
						},
...etc

This means that hypothetically, a user could put the following in their schema:

addons_config {
  http_load_balancing {
  }
}

From the user's point of view, this behavior would probably just take the API-level default. However, what it actually does is set disabled = false, since we ForceSend the field. If we decide to keep these fields nested in this way, we should make the booleans Required in order to make their behavior more clear.

Marking as 3.0 since this would be a breaking change.

@chrisst
Copy link
Contributor

chrisst commented Sep 12, 2019

As of today, I think this is the list:

google_compute_snapshot.source_disk_encryption_key.raw_key
google_dns_managed_zone.private_visibility_config.networks
google_dns_managed_zone.private_visibility_config.networks.network_url
google_compute_instance.scratch_disk.interface
google_storage_bucket.versioning.enabled
google_cloudiot_registry.credentials.public_key_certificate
google_app_engine_application.feature_settings.split_health_checks
google_project.app_engine.feature_settings.split_health_checks
google_compute_instance_from_template.scratch_disk.interface
google_monitoring_uptime_check_config.content_matchers.content
google_compute_backend_service.cdn_policy.cache_key_policy
google_tpu_node.scheduling_config.preemptible
google_compute_backend_bucket.cdn_policy.signed_url_cache_max_age_sec
google_dataproc_job.pyspark_config.logging_config.driver_log_levels
google_dataproc_job.hadoop_config.logging_config.driver_log_levels
google_dataproc_job.pig_config.logging_config.driver_log_levels
google_dataproc_job.sparksql_config.logging_config.driver_log_levels
google_dataproc_job.reference.job_id
google_dataproc_job.scheduling.max_failures_per_hour
google_dataproc_job.spark_config.logging_config.driver_log_levels
google_container_cluster.addons_config.http_load_balancing.disabled
google_container_cluster.addons_config.horizontal_pod_autoscaling.disabled
google_container_cluster.addons_config.kubernetes_dashboard.disabled
google_container_cluster.addons_config.network_policy_config.disabled
google_container_cluster.master_authorized_networks_config.cidr_blocks

@kibbles-n-bytes
Copy link

Is the GKE API's default behavior different than assuming "disabled": false when the object is empty? I just tried creating clusters through the API explorer with "httpLoadBalancing": {} and "kubernetesDashboard": {}, and got behavior consistent with that interpretation: When not specified, HTTP load balancing is on by default and Kubernetes dashboard is off by default, but passing the empty object to either of them forces enablement. Are there any APIs that break that pattern?

@danawillow
Copy link
Contributor Author

Just kidding, apparently there's a few more.

@megan07
Copy link
Contributor

megan07 commented Nov 18, 2019

This can be closed by the PRs referenced above.

@megan07 megan07 closed this as completed Nov 18, 2019
@ghost
Copy link

ghost commented Dec 19, 2019

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 👉 [email protected]. Thanks!

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

No branches or pull requests

4 participants