provider/consul: Fixes for consul_service resource

[dmportella]

Added service_id in place of id for resource.
modified created, read, update to use service_id
modified tests to include service_id.
modified documentation for consul_service to include new value.

Tests results

CONSUL_HTTP_ADDR=localhost:8500 make testacc TEST=./builtin/providers/consul/

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/14 14:43:05 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/consul/ -v -timeout 120m
=== RUN TestAccDataConsulKeys_basic
--- PASS: TestAccDataConsulKeys_basic (0.05s)
=== RUN TestAccConsulAgentService_basic
--- PASS: TestAccConsulAgentService_basic (0.05s)
=== RUN TestAccConsulCatalogEntry_basic
--- PASS: TestAccConsulCatalogEntry_basic (0.06s)
=== RUN TestAccConsulKeyPrefix_basic
--- PASS: TestAccConsulKeyPrefix_basic (0.19s)
=== RUN TestConsulKeysMigrateState
--- PASS: TestConsulKeysMigrateState (0.00s)
=== RUN TestConsulKeysMigrateState_empty
--- PASS: TestConsulKeysMigrateState_empty (0.00s)
=== RUN TestAccConsulKeys_basic
--- PASS: TestAccConsulKeys_basic (0.13s)
=== RUN TestAccConsulNode_basic
--- PASS: TestAccConsulNode_basic (0.05s)
=== RUN TestAccConsulPreparedQuery_basic
--- PASS: TestAccConsulPreparedQuery_basic (0.12s)
=== RUN TestAccConsulService_basic
--- PASS: TestAccConsulService_basic (0.05s)
=== RUN TestResourceProvider
--- PASS: TestResourceProvider (0.00s)
=== RUN TestResourceProvider_impl
--- PASS: TestResourceProvider_impl (0.00s)
=== RUN TestResourceProvider_Configure
--- PASS: TestResourceProvider_Configure (0.00s)
=== RUN TestResourceProvider_ConfigureTLS
--- PASS: TestResourceProvider_ConfigureTLS (0.00s)
PASS
ok github.com/hashicorp/terraform/builtin/providers/consul 0.708s

Refs: #9352

[stack72]

Hi @dmportella

Thanks for the work here - unfortunately, we can't just rename a parameter. This causes backward incompatibilities for people using the resource right now. What we have to do is:

• introduce a new parameter
• add a conflicts_with attribute to each to make sure both are not set
• add a deprecation to the old parameter to make sure users know it will be removed in a newer version
• have the ability that both parameters can be used to send the value to the cloud provider

Paul

[apparentlymart]

@stack72 I think in this case things are a little more nuanced because Terraform treats the "id" attribute as a special-case alias for the instance id (as in d.SetId).

I remember while working on data sources that I had to make a special effort to support user-settable id attributes for looking up things by id, because there is an implicit schema on all resources that sets id as computed. I expect that's the same trouble that is being addressed here.

All of this ramble is to say: I don't think it is necessary to preserve the explicit id attribute here because it is handled automatically by helper/schema as long as SetId is called.

[dmportella]

@stack72 I know I did consider the backwards compatibility, @apparentlymart is correct I noticed the Id is already used by terraform itself for the framework.

Set ID is definitely called on create and update so someone can still extract it since previously this property was never settable it could only be exported and that should still work since I set both service_id and call r.SetID with the same value. Notice that the tests still check for ID and they still pass.

@stack72 I agree with the the idea but i think this might be a special case because ID is like a protected keyword for terraform.

have the ability that both parameters can be used to send the value to the cloud provider

^ This still be true always

add a deprecation to the old parameter to make sure users know it will be removed in a newer version

^ The parameter cant be removed since it is an integral part of terraform.

introduce a new parameter

^ this is done 👍 ;)

add a conflicts_with attribute to each to make sure both are not set

^ I am happy to add this but I am not sure what to do here if you could provide examples...

Is this what you mean (https://github.com/hashicorp/terraform/pull/8987/files)?

Also quick question conflicts with you said both are not set can that be true about the ID property since it is a reserved attribute used by terraform?

[stack72]

Hi @dmportella

After reading what @apparentlymart said, this PR is now indeed correct :)

% make testacc TEST=./builtin/providers/consul                                                                                            1 ↵
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/26 13:05:52 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/consul -v  -timeout 120m
=== RUN   TestAccDataConsulKeys_basic
--- PASS: TestAccDataConsulKeys_basic (0.04s)
=== RUN   TestAccConsulAgentService_basic
--- PASS: TestAccConsulAgentService_basic (0.04s)
=== RUN   TestAccConsulCatalogEntry_basic
--- PASS: TestAccConsulCatalogEntry_basic (0.05s)
=== RUN   TestAccConsulKeyPrefix_basic
--- PASS: TestAccConsulKeyPrefix_basic (0.09s)
=== RUN   TestConsulKeysMigrateState
--- PASS: TestConsulKeysMigrateState (0.00s)
=== RUN   TestConsulKeysMigrateState_empty
--- PASS: TestConsulKeysMigrateState_empty (0.00s)
=== RUN   TestAccConsulKeys_basic
--- PASS: TestAccConsulKeys_basic (0.11s)
=== RUN   TestAccConsulNode_basic
--- PASS: TestAccConsulNode_basic (0.02s)
=== RUN   TestAccConsulPreparedQuery_basic
--- PASS: TestAccConsulPreparedQuery_basic (0.07s)
=== RUN   TestAccConsulService_basic
--- PASS: TestAccConsulService_basic (0.07s)
=== RUN   TestResourceProvider
--- PASS: TestResourceProvider (0.00s)
=== RUN   TestResourceProvider_impl
--- PASS: TestResourceProvider_impl (0.00s)
=== RUN   TestResourceProvider_Configure
--- PASS: TestResourceProvider_Configure (0.00s)
=== RUN   TestResourceProvider_ConfigureTLS
--- PASS: TestResourceProvider_ConfigureTLS (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/consul 0.507s

Thanks for the work here!

Paul

[dmportella]

No worries glad to help!

[ghost]

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.