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

New data source: consul_agent_config #42

Merged
merged 3 commits into from
May 3, 2018
Merged

New data source: consul_agent_config #42

merged 3 commits into from
May 3, 2018

Conversation

pearkes
Copy link
Contributor

@pearkes pearkes commented Apr 18, 2018

While attempting to get this providers acceptance tests passing on the latest version of Consul I noticed that the consul_agent_self data source largely did not work due to API changes in Consul.

This new datasource provides information similar to consul_agent_self, but is designed to only expose configuration that Consul will not change without versioning upstream.

In the 1.0 release of Consul, the /v1/agent/self endpoint was modified to only expose specific config information, rather than internal Consul data structures that were subject to change without warning.

This internal structure is still available on that API, but I don't think the Terraform provider should expose it given it is explicitly not part of the Consul compatibility promise. The acceptance tests currently fail with >1.0 of Consul, and had other issues in the 0.9 series.

So, this datasource would become the preferred way of retrieving configuration and the current consul_agent_self datasource would be deprecated and removed in a future release.

The new resource exposes far fewer attributes, but I don't believe that will have major impact (
to users of this provider given the seemingly uncommon use case of querying the /v1/agent/self endpoint from where Terraform is running).

So to summarize:

  • This new datasource relies on "agent/self" APIs that should not change
  • The previous datasource relied on APIs that regularly changed and Consul users are now discouraged from utilizing
$ TESTARGS='-run TestAccDataConsulAgentConfig' CONSUL_HTTP_ADDR=localhost:8500 make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccDataConsulAgentConfig -timeout 120m
?   	github.com/terraform-providers/terraform-provider-consul	[no test files]
=== RUN   TestAccDataConsulAgentConfig_basic
--- PASS: TestAccDataConsulAgentConfig_basic (0.05s)
PASS
ok  	github.com/terraform-providers/terraform-provider-consul/consul	0.069s

I'm not sure the process for deprecating an entire resource, but appreciate pointers there, including how I should version the provider.

pearkes added 2 commits April 17, 2018 17:26
This new datasource provides information similar to consul_agent_self,
but is designed to only expose configuration that Consul will not
change without versioning upstream.

In the 1.0 release of Consul, the [`/v1/agent/self`](https://www.consul.io/api/agent.html#read-configuration)
endpoint was [modified](https://www.consul.io/docs/upgrade-specific.html#config-section-of-agent-self-endpoint-has-changed)
to only expose specific config information, rather than internal Consul
data structures that were subject to change without warning.

This internal structure is still avaiable on that API, but I don't think
the Terraform provider should expose it given it is explicitly not part
of the Consul compatibility promise. The acceptance tests currently
fail with >1.0 of Consul, and had other issues in the 0.9 series.

So, this datasource would become the preferred way of retrieving configuration
and the current [`consul_agent_self`](https://www.terraform.io/docs/providers/consul/d/agent_self.html)
datasource would be deprecated and removed in a future release.

The new resource exposes far fewer attributes, but I don't believe that
will have major impact (though willing to hear feedback on this)
to users of this provider given the seemingly uncommon use case
of querying the `/v1/agent/self` endpoint from where Terraform is running.
@pearkes pearkes requested a review from vancluever April 18, 2018 00:35
@vancluever
Copy link
Contributor

LGTM for the most part (just waiting on rebuilding the website so I can check docs, before approving)

I think in regards to deprecating an entire resource - there's no process in the schema I don't think. I'd recommend dropping a notice on a Required field, but I see that consul_agent_self doesn't have any. Not too sure if it will trigger, but maybe if we make an Optional field with a Default, it will trigger the notice in the same way?

"deprecated": {
	Type:        schema.TypeBool,
	Optional:    true,
	Default:     true,
	Deprecated: "The consul_agent_self data source has been deprecated and will be removed from future releases.",
},

As an alternative to deprecating/obsoleting, we could just move the whole of the schema to UnsafeSetFieldRaw, include the data in DebugConfig in the export, and throw a warning in the docs that the API on the data source is almost always unstable, and link back to the Consul docs for a reference on exported values. This is obviously a breaking change though in itself.

vancluever
vancluever previously approved these changes Apr 19, 2018
Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

Just one doc nit, otherwise LGTM!

Provides the configuration information of the local Consul agent.
---

# consul_agent_config
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want to include a ~> **NOTE:** regarding how this data source differs from consul_agent_self (and add something reflective into consul_agent_config)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, added that!

@pearkes
Copy link
Contributor Author

pearkes commented May 1, 2018

@vancluever Thanks for the tips on the deprecating a resource business. I'll experiment with the optional/default method and see how it looks in a future PR.

@pearkes pearkes requested a review from vancluever May 1, 2018 22:26
@pearkes
Copy link
Contributor Author

pearkes commented May 3, 2018

Going to go ahead and merge this given the only comment was for a doc note which was added -- there are a number of website changes (layout file) in here that will conflict with master quickly.

@pearkes pearkes merged commit af70c0c into master May 3, 2018
@pearkes pearkes deleted the data-source-config branch May 3, 2018 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants