Deprecations, Removals, and changes for 2.0.0 #49

pearkes · 2018-06-06T22:34:46Z

You may want to review #46 for more context on these changes, as they are part of a larger effort to simplify the user experience of the provider.

Motivation

This PR includes major backwards incompatible changes from the current version of the provider. This was required due to:

Upstream Consul API breaking changes (in 1.0 and otherwise)
Overlapping use cases between resources (some duplication in service registration, querying)
Allowing for unsupported / discouraged use cases, eg registering services against a local agent with Terraform

Changes

consul_agent_self has been deprecated. It does not work at all on Consul 1.0+ due to an upstream change. Because it had no required attributes, I've added one named deprecated which can be set to a boolean value, which then emits a warning stating it will be removed. This is the best UX I could come up with for deprecating an entire resource. Note that consul_agent_config was added in New data source: consul_agent_config #42 which provides some similar functionality.
consul_catalog_* renamed. The catalog datasources were renamed, removing the "catalog" part of the name. This is mostly for clarity as all service/node resources + datasources should use the catalog in the Terraform provider. Aliases were created, so the old naming should still work.
Deprecation warnings for consul_agent_service, consul_catalog_entry. Warnings have been added about upcoming deprecations for these resources due to the presence of consul_node and consul_service.
Upgrade guide. Added a comprehensive upgrade guide coming from the 1.x.x series.
consul_service changes. The consul_service resource now uses the catalog APIs and requires a node to be specified. Please see this comment for more detail.
Consul client upgrade. This upgrades the vendored version of the Consul API client to the 1.1.0 version, which is the latest Consul release.

Overall, I believe this is the least amount of change possible in this release in order to clarify several different problems reported by users of the provider to date, and to lay groundwork for future support of Consul features, covered in detail by #46. It was tough to propose these changes given it could possibly cause significant downstream work, but I believe the trade-off is worth it.

As a reminder, any users wishing to stay on the 1.x.x series of the provider can do so by pinning the version in the provider block:

provider "consul" {
  version = "1.0.0"
}

I'd definitely appreciate review and feedback on both implementation and upgrade strategies.

apparentlymart · 2018-06-13T16:34:33Z

consul/data_source_consul_agent_self.go

+				Type:       schema.TypeBool,
+				Required:   true,
+				Deprecated: "The consul_agent_self resource will be deprecated and removed in a future version. More information: https://github.com/terraform-providers/terraform-provider-consul/issues/46",
+			},


We have precedent for deprecating resource types from back when some of them were reworked as data sources, implemented via a transform function.

It could be reasonable to add another similar function that allows attaching a more free-form deprecation message via the same hook. The message added here, if non-empty, is returned as a warning during the validation step.

Your approach here is an interesting workaround, but it'll cause users to need to upgrade their configs to add this additional attribute, making this more of an "opt in to legacy behavior" flag. Certainly better than just removing it outright, of course!

paddycarver · 2018-06-13T16:34:24Z

consul/data_source_consul_agent_self.go

+				Type:       schema.TypeBool,
+				Required:   true,
+				Deprecated: "The consul_agent_self resource will be deprecated and removed in a future version. More information: https://github.com/terraform-providers/terraform-provider-consul/issues/46",
+			},


I'm curious about this resource deprecation strategy. Essentially, it's going to force all users to add a deprecated key to their configs, which is technically a backwards incompatible change itself, isn't it?

But I notice the resource has no other user-supplied fields on it, which makes it hard to mark as deprecated otherwise.

Is this solution the result of a previous discussion, or is a discussion about alternatives worthwhile?

Essentially, it's going to force all users to add a deprecated key to their configs, which is technically a backwards incompatible change itself, isn't it?

Yes, exactly. This is a solution based on a previous discussion about how there is no good solution. I'm super open to fixing it, but it sounds like we'd need an upstream change to support it that isn't near-term.

paddycarver · 2018-06-13T16:35:25Z

consul/data_source_consul_agent_self_test.go

@@ -9,6 +9,8 @@ import (
 )

 func TestAccDataConsulAgentSelf_basic(t *testing.T) {
+	t.Skip("consul_agent_self is incompatible with Consul versions >= 1.0. It has been deprecated and will be removed in an upcoming version.")


Generally we continue to test things that are deprecated until they're actually removed, I believe, because we have a responsibility to make sure even deprecated things continue to work.

I agree -- unfortunately the upstream API no longer supports this so the tests fail outright. Should I just leave them failing?

That's fine by me, just felt better to call Skip() like this.

Perhaps we could make this opt-in with an environment variable, so we can still run tests against older versions of Consul if we want to?

In practice we're unlikely to actually bother to do this in any sort of automatic way, so I'd honestly be fine with leaving this t.Skip hardcoded under the expectation that we'll manually remove it temporarily for testing if we end up needing to do some further dev work for this against older versions of Consul. (which seems unlikely in itself)

I mean, the ideal here would be to run tests against the last version of Consul to support this and the latest version of Consul, and use an environment variable to switch whether this was skipped or not.

But also, I don't think it's pressing enough to have a full test matrix built up before we review this PR, so I'm fine with skipping the tests that don't and can't pass, with the understanding that we'll be removing that functionality ASAP.

paddycarver · 2018-06-13T16:36:22Z

consul/data_source_consul_nodes_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccDataConsulCatalogNodes_basic(t *testing.T) {


Is it worth taking this opportunity to drop the Catalog from the test name, as well?

Yes -- this was me being lazy but I think now's the time. Will address.

paddycarver · 2018-06-13T16:38:06Z

consul/data_source_consul_service_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccDataConsulCatalogService_basic(t *testing.T) {


Is it worth taking this opportunity to drop the Catalog from the test name as well?

paddycarver · 2018-06-13T16:38:36Z

consul/data_source_consul_services_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccDataConsulCatalogServices_basic(t *testing.T) {


Is it worth taking this opportunity to drop the Catalog from the test name, too?

paddycarver · 2018-06-13T16:57:23Z

consul/resource_consul_service.go

 	}

 	// Clear the ID
 	d.SetId("")
 	return nil
 }
+
+func retrieveService(client *consulapi.Client, name string, ident string, node string, dc string) (*consulapi.CatalogService, error) {


I don't see where node is used in this function. Am I missing something?

Awesome catch. That was pretty vital due to the Consul data model, I hadn't actually done the appropriate check for matching a service. Also highlighted a gap in my tests.

paddycarver · 2018-06-13T16:59:00Z

consul/resource_consul_service_test.go

-					testAccCheckConsulServiceValue("consul_service.app", "tags.0", "tag0"),
-					testAccCheckConsulServiceValue("consul_service.app", "tags.1", "tag1"),
+					resource.TestCheckResourceAttr("consul_service.google", "id", "google"),
+					resource.TestCheckResourceAttr("consul_service.google", "address", "www.google.com"),


I don't know how we feel about using google.com in our tests, but just calling out that hashicorptest.com exists if you ever need a test domain that doesn't serve content.

Yeah sounds good. This was just in here but I like switching it out.

paddycarver · 2018-06-13T17:01:00Z

consul/resource_consul_service_test.go

+					resource.TestCheckResourceAttr("consul_service.google", "address", "lb.google.com"),
+				),
+			},
+		},


My only real nitpick with this test is that it checks that the tests modified what they were supposed to, but doesn't check that the things they weren't supposed to modify are still what we can expect. If there's a bug that causes a field to get unset in state, or set to a wrong value in state, on update this won't catch it.

Sure, totally fair. Going to add some more checks.

paddycarver · 2018-06-13T17:05:35Z

website/docs/r/service.html.markdown


 ## Attributes Reference

 The following attributes are exported:

-* `service_id` - The id of the service, defaults to the value of `name`.
+* `id` - The ID of the service.


I'd highly recommend storing IDs that mean something to users in something other than id, because we often have to overload those for import, and it gets your users in the habit of treating id as opaque while still making the information accessible.

You're totally right, this change was not needed actually, we already expose service_id for this purpose. Fixed the docs and updated a test to check my assumption there.

paddycarver · 2018-06-13T17:07:28Z

website/docs/d/nodes.html.markdown

-page_title: "Consul: consul_catalog_nodes"
-sidebar_current: "docs-consul-data-source-catalog-nodes"
+page_title: "Consul: consul_nodes"
+sidebar_current: "docs-consul-data-source-nodes"


You also need to update these in website/consul.erb or the sidebar highlighting won't work anymore.

Fixed, thanks!

paddycarver · 2018-06-19T18:16:56Z

consul/resource_consul_service.go

@@ -81,7 +81,7 @@ func resourceConsulServiceCreate(d *schema.ResourceData, meta interface{}) error
 	// managed by the consul_node resource (or datasource)
 	nodeCheck, _, err := client.Catalog().Node(node, &consulapi.QueryOptions{Datacenter: dc})
 	if err != nil {
-		return fmt.Errorf("Cannot retrieve node: %v", err)
+		return fmt.Errorf("Cannot retrieve node '%s': %v", node, err)


Tip for future: %q will correctly quote and escape strings.

Not worth changing, just thought I'd call it out in case you weren't aware.

paddycarver · 2018-06-19T18:17:25Z

consul/resource_consul_service.go

@@ -135,9 +135,11 @@ func resourceConsulServiceCreate(d *schema.ResourceData, meta interface{}) error
 		return fmt.Errorf("Failed to register service (dc: '%s'): %v", dc, err)
 	}

+	// Retrieve the service again to get the canonical service ID. We can't
+	// get this back from the register call or through


It looks like you dropped part of this comment?

Changes made.

paddycarver · 2018-06-19T18:25:54Z

All the changes made look good to me. I think the code of this looks fine, with the exception of that deprecated resource. I haven't run the tests yet, but I'll do that and make sure they still pass for me, too. I'd say we're good to go on this if we can't get a better solution to that deprecated resource in time.

I'm going to work on a PR today to make it possible to deprecate an entire resource. I'll link it from here. If we can get that reviewed and merged to Terraform core this week, I'd much rather we go down that road. And it seems like that's possible? So if we can do that, and you can just vendor Terraform at that commit, and update the resource in question to use the new Deprecated property, I think that's a better way forward, if we can make the timeline work.

If we get tight on time, I'm reluctantly OK with merging this as-is, because absent a Terraform core change, I don't see a better solution.

paddycarver · 2018-06-19T19:06:38Z

hashicorp/terraform#18286 has been opened to make it so providers can deprecate resources. :)

paddycarver · 2018-06-20T18:40:28Z

As of hashicorp/terraform@ec998a2, we have support for deprecating resources, not just fields. @pearkes, do you want to update your vendored copy and we can fix how the consul_agent_self data source is deprecated?

pearkes · 2018-06-21T19:05:02Z

@paddycarver Awesome. Will work on an update and ping you for a final look!

pearkes · 2018-06-21T23:35:13Z

@paddycarver updated to that version and utilizing it in 497123c! Here's the output in practice, looks great:

$ cat main.tf | grep agent_self
data "consul_agent_self" "read-agent" {}
$ terraform plan

Warning: data.consul_agent_self.read-agent: The consul_agent_self resource will be deprecated and removed in a future version. More information: https://github.com/terraform-providers/terraform-provider-consul/issues/46

ref: hashicorp/consul#2922

This moves the consul_service resource to the catalog APIs for service registration and deregistration. Agent APIs are intended for service management on-node, which Terraform is not designed for. This requires 'node' be set for each service, and for the node to exist. It will not allow an upsert, as is the behavior of the Consul API.

paddycarver

Tests pass in CI, Travis is happy again, and our resource deprecations look great.

Amazing job @pearkes, this looks fantastic. Super excited for 2.0.0!

pearkes requested a review from a team June 6, 2018 22:34

apparentlymart reviewed Jun 13, 2018

View reviewed changes

paddycarver previously requested changes Jun 13, 2018

View reviewed changes

paddycarver reviewed Jun 19, 2018

View reviewed changes

pearkes mentioned this pull request Jun 20, 2018

Add node_meta attr to consul_catalog_entry #51

Closed

paddycarver mentioned this pull request Jun 20, 2018

Export resource deprecation. hashicorp/terraform#18286

Merged

pearkes added 17 commits June 21, 2018 16:45

website: add upgrading page

a5bcb19

resource/agent_service: add deprecation notice

700cc34

datasource/agent_self: try a deprecation strategy and skip tests

31fe340

website: improve upgrading guide, modify service

35ffe6b

vendor: update to consul 1.1.0 api client

b64d00e

config: compatibility with new version client

5e12c34

ref: hashicorp/consul#2922

website: note in the upgrading guide about adding node

714d49e

datasource/catalog*: rename and alias

659e1d2

update deprecation messages for catalog_entry, agent_service, agent_self

3d39cbb

datasource/catalog*: add tests for aliases

92092fa

resource/consul_service: dc support, service_id as id

d028672

website: correct link to catalog_entry

34a4e16

datasource/consul_agent_self: make deprecation required to force

0a28925

website: 1.1.0 -> 2.0.0 in upgrade messages, instructions

0d5f949

website: fix a broken link in the upgrade guide

c826df7

resource/consul_service: improve error messaging on create

3870676

pearkes added 8 commits June 21, 2018 16:45

resource/consul_service: validate node and service ID match

adce94f

resource/consul_service: improve checks for lb mod

ecb5644

resource/consul_service: drop references to google in tests

a10bc01

resource/consul_service: fix docs for service_id

f6c77d1

website: fix sidebar highlighting and links

f5b5afe

datasource/consul_catalog*: remove catalog from main data source funcs

a2abb0e

update to ec998a2 of the TF SDK(s) and their dependencies

e743d50

move to DeprecationMessage for whole resource deprecations

ca3a1aa

pearkes force-pushed the 2-0-0 branch from 497123c to ca3a1aa Compare June 21, 2018 23:46

paddycarver approved these changes Jun 21, 2018

View reviewed changes

pearkes merged commit b72d557 into master Jun 21, 2018

pearkes deleted the 2-0-0 branch June 21, 2018 23:58

bflad mentioned this pull request Jul 13, 2018

vendor: hashicorp/terraform@ec998a21bc95366269fbd196e09d3a458bba8be7 hashicorp/terraform-provider-aws#5188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecations, Removals, and changes for 2.0.0 #49

Deprecations, Removals, and changes for 2.0.0 #49

pearkes commented Jun 6, 2018 •

edited

apparentlymart Jun 13, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

pearkes Jun 14, 2018

apparentlymart Jun 19, 2018

paddycarver Jun 19, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

paddycarver Jun 13, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 13, 2018

pearkes Jun 14, 2018

paddycarver Jun 19, 2018

paddycarver Jun 19, 2018

paddycarver commented Jun 19, 2018

paddycarver commented Jun 19, 2018

paddycarver commented Jun 20, 2018

pearkes commented Jun 21, 2018

pearkes commented Jun 21, 2018

paddycarver left a comment

Deprecations, Removals, and changes for 2.0.0 #49

Deprecations, Removals, and changes for 2.0.0 #49

Conversation

pearkes commented Jun 6, 2018 • edited

Motivation

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver commented Jun 19, 2018

paddycarver commented Jun 19, 2018

paddycarver commented Jun 20, 2018

pearkes commented Jun 21, 2018

pearkes commented Jun 21, 2018

paddycarver left a comment

Choose a reason for hiding this comment

pearkes commented Jun 6, 2018 •

edited