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

provider/consul: consul_keys data source #7678

Merged
merged 1 commit into from
Jul 26, 2016
Merged

Conversation

apparentlymart
Copy link
Contributor

Previously the consul_keys resource did double-duty as both a reader and writer of values from the Consul key/value store, but that made its interface rather confusing and complex, as well as having all of the other general problems associated with read-only resources.

Here we split the functionality such that reading is done with the consul_keys data source while writing is still done with the consul_keys resource. (or, indeed, the consul_key_prefix resource)

The old read behavior of the resource is still supported, but it's no longer documented (except as a deprecation note) and will generate deprecation warnings when used.

In future it should be possible to simplify the consul_keys resource by removing all of the read support, but that is deferred for now to give users a chance to gracefully migrate to the new data source.

Previously the consul_keys resource did double-duty as both a reader and
writer of values from the Consul key/value store, but that made its
interface rather confusing and complex, as well as having all of the other
general problems associated with read-only resources.

Here we split the functionality such that reading is done with the
consul_keys data source while writing is done with the consul_keys
resource.

The old read behavior of the resource is still supported, but it's no
longer documented (except as a deprecation note) and will generate
deprecation warnings when used.

In future it should be possible to simplify the consul_keys resource by
removing all of the read support, but that is deferred for now to give
users a chance to gracefully migrate to the new data source.
@apparentlymart
Copy link
Contributor Author

I added the release-0.7 tag here in the hope that this patch is good to merge and can go into 0.7 with relatively little fanfare. However, if it seems problematic then I'm happy to defer to 0.7.1 so that any revisions don't block the release.

@jen20
Copy link
Contributor

jen20 commented Jul 17, 2016

Hi @apparentlymart, I'm happy to merge this for 0.7, I'll review on Monday.

@mgaffney
Copy link
Member

@jen20 Have you had a chance to review this?

@stack72
Copy link
Contributor

stack72 commented Jul 26, 2016

Hey @apparentlymart

Just had a look through this and ran the tests :)

% make testacc TEST=./builtin/providers/consul TESTARGS='-run=TestAccDataConsulKeys_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
TF_ACC=1 go test ./builtin/providers/consul -v -run=TestAccDataConsulKeys_ -timeout 120m
=== RUN   TestAccDataConsulKeys_basic
--- PASS: TestAccDataConsulKeys_basic (0.03s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/consul 0.045s

This LGTM!

@stack72 stack72 merged commit 19d0b42 into master Jul 26, 2016
@stack72 stack72 deleted the f-consul-keys-data branch July 26, 2016 17:23
@ghost
Copy link

ghost commented Apr 24, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

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

Successfully merging this pull request may close these issues.

4 participants