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

Use provider datacenter as default when the agent cannot be reached #105

Merged

Conversation

remilapeyre
Copy link
Collaborator

@remilapeyre remilapeyre commented Apr 26, 2019

According to the current documentation, the datacenter attribute should
default to the provider configuration and if it is not set to the
datacenter of the agent terraform is connected to:

* `datacenter` - (Optional) The datacenter to use. This overrides the datacenter in the
  provider setup and the agent's default datacenter.

This is not what the code actually does and the datacenter in the provider
configuration is not used most of the time.

Since changing the plugin behavior to what is actually documented may
break code relying on the current one. To keep backward compatibility,
we can change the documentation to:

* `datacenter` - (Optional) The datacenter to use. This overrides the
  agent's default datacenter and the datacenter in the provider setup.

and fall back to the provider configuration only when the datacenter of
the agent to which terraform is connected to can not be read (when ACL
is enabled and the token used by terraform does not have permission to
read /agent/self for example).

The change is very noisy as it requires to change the ConfigureFunc of
the provider from (*consulapi.Client, error) to (*Config, error) and
update all places where was being used.

The interesting change is at https://github.com/terraform-providers/terraform-provider-consul/pull/105/files#diff-7c162fe0d0220927a53a0f1977a47a71R361

Discussion: #57

According to the current documentation, the datacenter attribute should
default to the provider configuration and if it is not set to the
datacenter of the agent terraform is connected to:

	* `datacenter` - (Optional) The datacenter to use. This overrides the datacenter in the
	  provider setup and the agent's default datacenter.

This is not what the code actually does and the datacenter in the provider
configuration is not used most of the time.

Since changing the plugin behavior to what is actually documented may
break code relying on the current one. To keep backward compatibility,
we can change the documentation to:

	* `datacenter` - (Optional) The datacenter to use. This overrides the
	  agent's default datacenter and the datacenter in the provider setup.

and fall back to the provider configuration only when the datacenter of
the agent to which terraform is connected to can not be read (when ACL
is enabled and the token used by terraform does not have permission to
read /agent/self for example).

The change is very noisy as it requires to change the `ConfigureFunc` of
the provider from `(*consulapi.Client, error)` to `(*Config, error)` and
update all places where was being used.

Discussion: hashicorp#57
@ghost ghost added the size/XL label Apr 26, 2019
Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

This looks good – a lot clearer and more accurate from my perspective. I tried to think of potential negative impact of this in terms of existing users configuring at each level but I assume that anyone going the route of per resource configuration won't notice any issues.

consul/data_source_consul_agent_config.go Outdated Show resolved Hide resolved
@ghost ghost added size/L and removed size/XL labels May 21, 2019
@remilapeyre remilapeyre merged commit ea76209 into hashicorp:master May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants