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

Deprecate workspace_external_id/workspace_external_ids in tfe_notification_configuration, tfe_policy_set, and tfe_run_trigger resources #182

Merged
merged 7 commits into from
Jun 3, 2020

Conversation

lafentres
Copy link
Contributor

@lafentres lafentres commented May 27, 2020

Description

This PR adds deprecation warnings to the ids attribute for data source tfe_workspace_ids, the workspace_external_id attributes for resources tfe_notification_configuration & tfe_run_trigger, and the workspace_external_ids attribute for resource tfe_policy_set.

The following data sources are impacted:

  • tfe_workspace_ids - ids is deprecated and will show a deprecation warning but will not cause any errors. You should use full_names instead.
    NOTE: If you use the tfe_workspace_ids data source, this deprecation warning won't go away until it is removed. This is due to a limitation in Terraform SDK with deprecating attributes that aren't specified via config. See this issue for more information.

The following resources are impacted:

  • tfe_notification_configuration - workspace_external_id is deprecated and will show a deprecation warning but will not cause any errors. You should use workspace_id instead
  • tfe_policy_set - workspace_external_ids is deprecated and will show a deprecation warning but will not cause any errors. You should use workspace_ids instead
  • tfe_run_trigger - workspace_external_id is deprecated and will show a deprecation warning but will not cause any errors. You should use workspace_id instead

Why are these attributes being deprecated?

After #106 migrated workspace ids to the immutable id format (ws-<random string>), we still had some inconsistent usages of external_id, workspace_external_id, and workspace_external_ids. In order to clean this up and make the provider consistent and easier to understand when working with workspace IDs, we will be replacing all usages of workspace_external_id/workspace_external_ids with workspace_id/workspace_ids.

Deprecation plan:

To minimize disruption and give everyone time to migrate over, we will be doing this in 3 phases.

Phase 1:

You are here. That's what this PR is for :]

tfe_workspace_ids Data Source: We will add a new computed attribute to the tfe_workspace_ids data source called full_names that will mimic the current behavior of the ids attribute. In this phase, both attributes will return the old human-readable workspace ID format of <organization_name>/<workspace_name>. We will add deprecation warnings to the ids attribute explaining that users should begin using full_names instead, as the behavior of ids will be changing soon.

Remainder of TFE Provider: We will add a new attribute to impacted resources called workspace_id or workspace_ids (depending on which is required for the specific resource) which will hold the same value as the current workspace_external_id/workspace_external_ids attributes. Deprecation warnings will be added to all of these, recommending the user move to workspace_id/ workspace_ids instead.

Phase 2:

This phase happens 3 months after phase 1 is released.

tfe_workspace_ids Data Source: We will officially change the behavior of the ids attribute so it returns the immutable workspace ID in format ws-<random_string>, just as the external_ids attribute currently does. We will add a deprecation warning to the external_ids attribute, alerting them that it will be deprecated soon and they should use ids instead.

Remainder of TFE Provider: We will remove the workspace_external_id / workspace_external_ids attributes from all the impacted resources.

Phase 3:

This phase happens 3 months after phase 2 is released.

tfe_workspace_ids Data Source: We will remove external_ids from this data source entirely, leaving behind ids returning the immutable workspace ID in the format ws-<random_string>, and full_names returning the format <organization_name>/<workspace_name>.

Testing plan

To run through this test plan you'll need either:

  1. Your own copies of a terraform-random repo and this test-policy-set repo.
    OR
  2. An existing Terraform config that uses the data source tfe_workspace_ids and the resources tfe_notification_configuration, the_policy_set (with workspace_external_ids set), & tfe_run_trigger.

Provision resources for all impacted resources and data sources

  1. Save this config as main.tf, replacing values with your own information as needed.
    provider "tfe" {
      hostname = "YOUR_TFC_HOSTNAME"
      version  = "0.17.1"
    }
    
    resource "tfe_organization" "test-org" {
      name  = "tst-deprecation-test-org"
      email = "YOUR_EMAIL"
    }
    
    resource "tfe_oauth_client" "test-oauth-client" {
      organization     = tfe_organization.test-org.name
      api_url          = "https://api.github.com"
      http_url         = "https://github.com"
      oauth_token      = "YOUR_GITHUB_TOKEN"
      service_provider = "github"
    }
    
    resource "tfe_workspace" "workspace-1" {
      name         = "workspace-1"
      organization = tfe_organization.test-org.name
      vcs_repo {
        identifier     = "GITHUB_USERNAME/terraform-random"
        oauth_token_id = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
    resource "tfe_workspace" "workspace-2" {
      name         = "workspace-2"
      organization = tfe_organization.test-org.name
      vcs_repo {
        identifier     = "GITHUB_USERNAME/terraform-random"
        oauth_token_id = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
    resource "tfe_workspace" "workspace-3" {
      name         = "workspace-3"
      organization = tfe_organization.test-org.name
      vcs_repo {
        identifier     = "GITHUB_USERNAME/terraform-random"
        oauth_token_id = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
    resource "tfe_run_trigger" "run-trigger-1" {
      workspace_external_id = tfe_workspace.workspace-1.id
      sourceable_id = tfe_workspace.workspace-2.id
    }
    
    resource "tfe_notification_configuration" "notification_config" {
      name                  = "my-test-notification-configuration"
      enabled               = true
      destination_type      = "generic"
      triggers              = ["run:created", "run:planning", "run:errored"]
      url                   = "https://example.com"
      workspace_external_id = tfe_workspace.workspace-1.id
    }
    
    resource "tfe_policy_set" "policy-set" {
      name                   = "my-policy-set"
      description            = "A brand new policy set"
      organization           = tfe_organization.test-org.name
      policies_path          = "policy-sets/foo"
      workspace_external_ids = [tfe_workspace.workspace-1.id]
      vcs_repo {
        identifier         = "GITHUB_USERNAME/test-policy-set"
        branch             = "master"
        ingress_submodules = false
        oauth_token_id     = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
    data "tfe_workspace_ids" "all" {
      names        = [tfe_workspace.workspace-1.name, tfe_workspace.workspace-2.name]
      organization = tfe_organization.test-org.name
    }
    
    output "workspace_ids" {
      value = data.tfe_workspace_ids.all.ids
    }
    
    output "workspace_external_ids" {
      value = data.tfe_workspace_ids.all.external_ids
    }
  2. Run terraform init and terraform version. Your provider version should look report back as v0.17.1
    provider.tfe v0.17.1
    
  3. Run terraform apply. This should succeed.
  4. Run terraform plan. There should be no changes.

Upgrade provider version to local build of this branch

  1. Follow the steps in the README to make a local build of the provider from this branch. Copy that build to your ~/.terraform.d/plugins directory.
  2. In the directory where you saved your main.tf, delete your .terraform folder.
  3. In your main.tf remove or comment out the version in your tfe provider block
    provider "tfe" {
      hostname = "YOUR_TFC_HOSTNAME"
      #version  = "0.17.1"
    }
  4. Run terraform init and terraform version. Your provider version should now be unversioned.
    provider.tfe (unversioned)
    
  5. Run terraform plan. There should be no changes but you should see some deprecation warnings. Your output should look something like this:
    No changes. Infrastructure is up-to-date.
    
    This means that Terraform did not detect any differences between your
    configuration and real physical resources that exist. As a result, no
    actions need to be performed.
    
    Warning: "workspace_external_id": [DEPRECATED] Use workspace_id instead. The 
    workspace_external_id attribute will be removed in the future. See the CHANGELOG to learn 
    more: https://github.com/terraform-providers/terraform-provider- 
    tfe/blob/v0.18.0/CHANGELOG.md
    
      on main.tf line 46, in resource "tfe_run_trigger" "run-trigger-1":
      46: resource "tfe_run_trigger" "run-trigger-1" {
    
       (and one more similar warning elsewhere)
    
    Warning: "workspace_external_ids": [DEPRECATED] Use workspace_ids instead. The 
    workspace_external_ids attribute will be removed in the future. See the CHANGELOG to learn 
    more: https://github.com/terraform-providers/terraform-provider- 
    tfe/blob/v0.18.0/CHANGELOG.md
    
      on main.tf line 60, in resource "tfe_policy_set" "policy-set":
      60: resource "tfe_policy_set" "policy-set" {
    
    Warning: "ids": [DEPRECATED] Use full_names instead. The ids attribute will be removed in 
    the future. See the CHANGELOG to learn more: https://github.com/terraform- 
    providers/terraform-provider-tfe/blob/v0.18.0/CHANGELOG.md
    
      on main.tf line 74, in data "tfe_workspace_ids" "all":
      74: data "tfe_workspace_ids" "all" {
    

Make sure you can still update workspace_external_id/workspace_external_ids

  1. Change workspace_external_id in the tfe_notification_configuration and tfe_run_trigger resources to use workspace-3. Change workspace_external_ids in the tfe_policy_set resource to use workspace-3.
    resource "tfe_run_trigger" "run-trigger-1" {
      workspace_external_id = tfe_workspace.workspace-3.id
      sourceable_id = tfe_workspace.workspace-2.id
    }
    
    resource "tfe_notification_configuration" "notification_config" {
      name                  = "my-test-notification-configuration"
      enabled               = true
      destination_type      = "generic"
      triggers              = ["run:created", "run:planning", "run:errored"]
      url                   = "https://example.com"
      workspace_external_id = tfe_workspace.workspace-3.id
    }
    
    resource "tfe_policy_set" "policy-set" {
      name                   = "my-policy-set"
      description            = "A brand new policy set"
      organization           = tfe_organization.test-org.name
      policies_path          = "policy-sets/foo"
      workspace_external_ids = [tfe_workspace.workspace-3.id]
      vcs_repo {
        identifier         = "GITHUB_USERNAME/test-policy-set"
        branch             = "master"
        ingress_submodules = false
        oauth_token_id     = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
  2. Run terraform apply. The output should show Plan: 2 to add, 1 to change, 2 to destroy.. It should show that:
    • tfe_notification_configuration.notification_config must be replaced
    • tfe_policy_set.policy-set will be updated in-place
    • tfe_run_trigger.run-trigger-1 must be replaced
  3. Type yes to confirm the apply. This should succeed and the output should show Apply complete! Resources: 2 added, 1 changed, 2 destroyed..
  4. Run terraform plan. There should be no changes.
  5. Change workspace_external_id in the tfe_notification_configuration and tfe_run_trigger resources back to workspace-1. Change workspace_external_ids in the tfe_policy_set resource back workspace-1.
  6. Run terraform apply. This should succeed.
  7. Run terraform plan. There should be no changes.

Change workspace_external_id/workspace_external_ids to workspace_id/workspace_ids

  1. Change workspace_external_id in the tfe_notification_configuration and tfe_run_trigger resources to workspace_id. Change workspace_external_ids in the tfe_policy_set resource to workspace_ids
    resource "tfe_run_trigger" "run-trigger-1" {
      workspace_id  = tfe_workspace.workspace-1.id
      sourceable_id = tfe_workspace.workspace-2.id
    }
    
    resource "tfe_notification_configuration" "notification_config" {
      name                  = "my-test-notification-configuration"
      enabled               = true
      destination_type      = "generic"
      triggers              = ["run:created", "run:planning", "run:errored"]
      url                   = "https://example.com"
      workspace_id          = tfe_workspace.workspace-1.id
    }
    
    resource "tfe_policy_set" "policy-set" {
      name                   = "my-policy-set"
      description            = "A brand new policy set"
      organization           = tfe_organization.test-org.name
      policies_path          = "policy-sets/foo"
      workspace_ids          = [tfe_workspace.workspace-1.id]
      vcs_repo {
        identifier         = "GITHUB_USERNAME/test-policy-set"
        branch             = "master"
        ingress_submodules = false
        oauth_token_id     = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
  2. Run terraform apply. This should report no changes. Your output should look something like this:
    Warning: "ids": [DEPRECATED] Use full_names instead. The ids attribute will be removed in 
    the future. See the CHANGELOG to learn more: https://github.com/terraform- 
    providers/terraform-provider-tfe/blob/v0.18.0/CHANGELOG.md
    
      on main.tf line 74, in data "tfe_workspace_ids" "all":
      74: data "tfe_workspace_ids" "all" {
    
    Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
    
    Outputs:
    
    workspace_external_ids = {
      "workspace-1" = "YOUR_WORKSPACE_1_ID"
      "workspace-2" = "YOUR_WORKSPACE_2_ID"
    }
    workspace_ids = {
      "workspace-1" = "tst-deprecation-test-org/workspace-1"
      "workspace-2" = "tst-deprecation-test-org/workspace-2"
    }
    
  3. Run terraform plan. There should be no changes.

Make sure you can still update workspace_id/workspace_ids

  1. Change workspace_id in the tfe_notification_configuration and tfe_run_trigger resources to use workspace-3. Change workspace_ids in the tfe_policy_set resource to use workspace-3.
    resource "tfe_run_trigger" "run-trigger-1" {
      workspace_id  = tfe_workspace.workspace-3.id
      sourceable_id = tfe_workspace.workspace-2.id
    }
    
    resource "tfe_notification_configuration" "notification_config" {
      name                  = "my-test-notification-configuration"
      enabled               = true
      destination_type      = "generic"
      triggers              = ["run:created", "run:planning", "run:errored"]
      url                   = "https://example.com"
      workspace_id          = tfe_workspace.workspace-3.id
    }
    
    resource "tfe_policy_set" "policy-set" {
      name                   = "my-policy-set"
      description            = "A brand new policy set"
      organization           = tfe_organization.test-org.name
      policies_path          = "policy-sets/foo"
      workspace_ids          = [tfe_workspace.workspace-3.id]
      vcs_repo {
        identifier         = "GITHUB_USERNAME/test-policy-set"
        branch             = "master"
        ingress_submodules = false
        oauth_token_id     = tfe_oauth_client.test-oauth-client.oauth_token_id
      }
    }
    
  2. Run terraform apply. The output should show Plan: 2 to add, 1 to change, 2 to destroy.. It should show that:
    • tfe_notification_configuration.notification_config must be replaced
    • tfe_policy_set.policy-set will be updated in-place
    • tfe_run_trigger.run-trigger-1 must be replaced
  3. Type yes to confirm the apply. This should succeed and the output should show Apply complete! Resources: 2 added, 1 changed, 2 destroyed..
  4. Run terraform plan. There should be no changes.

Test out the new full_names attribute:

  1. Add an output for the new full_names attribute
    output "full_names" {
       value = data.tfe_workspace_ids.all.full_names
    }
  2. Run terraform apply. Make sure the outputs full_names and workspace_ids are the same. Your output should look like this:
    Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
    
    Outputs:
    
    full_names = {
      "workspace-1" = "tst-deprecation-test-org/workspace-1"
      "workspace-2" = "tst-deprecation-test-org/workspace-2"
    }
    workspace_external_ids = {
      "workspace-1" = "YOUR_WORKSPACE_1_ID"
      "workspace-2" = "YOUR_WORKSPACE_2_ID"
    }
    workspace_ids = {
      "workspace-1" = "tst-deprecation-test-org/workspace-1"
      "workspace-2" = "tst-deprecation-test-org/workspace-2"
    }
    

Make sure destroy still works

  1. Run terraform destroy. This should succeed.

Make sure a fresh create on this new version works

  1. Run terraform apply. This should succeed.

Check the documentation and CHANGELOG:

  1. Please read over the documentation updates for each resource and make sure they make sense. Please let me know if you have any suggestions or additions.
  2. Please read over the CHANGELOG entry and make sure it makes sense. Please let me know if you have any suggestions or additions.

External links

Output from acceptance tests

data source tfe_workspace_ids:

$ TESTARGS="-run TestAccTFEWorkspaceIDsDataSource" envchain terraform-provider-tfe make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEWorkspaceIDsDataSource -timeout 15m
?       github.com/terraform-providers/terraform-provider-tfe   [no test files]
=== RUN   TestAccTFEWorkspaceIDsDataSource_basic
--- PASS: TestAccTFEWorkspaceIDsDataSource_basic (6.70s)
=== RUN   TestAccTFEWorkspaceIDsDataSource_wildcard
--- PASS: TestAccTFEWorkspaceIDsDataSource_wildcard (7.15s)
PASS
ok      github.com/terraform-providers/terraform-provider-tfe/tfe       14.005s
?       github.com/terraform-providers/terraform-provider-tfe/version   [no test files]

resource tfe_notification_configuration:

$ TESTARGS="-run TestAccTFENotificationConfiguration" envchain terraform-provider-tfe make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFENotificationConfiguration -timeout 15m
?       github.com/terraform-providers/terraform-provider-tfe   [no test files]
=== RUN   TestAccTFENotificationConfiguration_basic
--- PASS: TestAccTFENotificationConfiguration_basic (5.99s)
=== RUN   TestAccTFENotificationConfiguration_basicWorkspaceID
--- PASS: TestAccTFENotificationConfiguration_basicWorkspaceID (5.59s)
=== RUN   TestAccTFENotificationConfiguration_update
--- PASS: TestAccTFENotificationConfiguration_update (8.89s)
=== RUN   TestAccTFENotificationConfiguration_updateWorkspaceID
--- PASS: TestAccTFENotificationConfiguration_updateWorkspaceID (8.47s)
=== RUN   TestAccTFENotificationConfiguration_updateWorkspaceExternalIDToWorkspaceID
--- PASS: TestAccTFENotificationConfiguration_updateWorkspaceExternalIDToWorkspaceID (11.93s)
=== RUN   TestAccTFENotificationConfiguration_slackWithToken
--- PASS: TestAccTFENotificationConfiguration_slackWithToken (3.10s)
=== RUN   TestAccTFENotificationConfiguration_duplicateTriggers
--- PASS: TestAccTFENotificationConfiguration_duplicateTriggers (5.49s)
=== RUN   TestAccTFENotificationConfiguration_noWorkspaceID
--- PASS: TestAccTFENotificationConfiguration_noWorkspaceID (3.25s)
=== RUN   TestAccTFENotificationConfigurationImport
--- PASS: TestAccTFENotificationConfigurationImport (5.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-tfe/tfe       59.287s
?       github.com/terraform-providers/terraform-provider-tfe/version   [no test files]

resource tfe_policy_set:

$ TESTARGS="-run TestAccTFEPolicySet" envchain terraform-provider-tfe make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEPolicySet -timeout 15m
?       github.com/terraform-providers/terraform-provider-tfe   [no test files]
=== RUN   TestAccTFEPolicySetParameter_basic
--- PASS: TestAccTFEPolicySetParameter_basic (6.73s)
=== RUN   TestAccTFEPolicySetParameter_update
--- PASS: TestAccTFEPolicySetParameter_update (10.08s)
=== RUN   TestAccTFEPolicySetParameter_import
--- PASS: TestAccTFEPolicySetParameter_import (6.64s)
=== RUN   TestAccTFEPolicySet_basic
--- PASS: TestAccTFEPolicySet_basic (7.07s)
=== RUN   TestAccTFEPolicySet_update
--- PASS: TestAccTFEPolicySet_update (12.42s)
=== RUN   TestAccTFEPolicySet_updateWorkspaceIDs
--- PASS: TestAccTFEPolicySet_updateWorkspaceIDs (16.22s)
=== RUN   TestAccTFEPolicySet_updateEmpty
--- PASS: TestAccTFEPolicySet_updateEmpty (10.05s)
=== RUN   TestAccTFEPolicySet_updatePopulated
--- PASS: TestAccTFEPolicySet_updatePopulated (14.60s)
=== RUN   TestAccTFEPolicySet_updatePopulatedWorkspaceIDs
--- PASS: TestAccTFEPolicySet_updatePopulatedWorkspaceIDs (14.68s)
=== RUN   TestAccTFEPolicySet_updateToGlobal
--- PASS: TestAccTFEPolicySet_updateToGlobal (12.45s)
=== RUN   TestAccTFEPolicySet_updateWorkspaceIDsToGlobal
--- PASS: TestAccTFEPolicySet_updateWorkspaceIDsToGlobal (12.09s)
=== RUN   TestAccTFEPolicySet_updateToWorkspace
--- PASS: TestAccTFEPolicySet_updateToWorkspace (11.96s)
=== RUN   TestAccTFEPolicySet_updateGlobalToWorkspaceIDs
--- PASS: TestAccTFEPolicySet_updateGlobalToWorkspaceIDs (12.38s)
=== RUN   TestAccTFEPolicySet_vcs
--- PASS: TestAccTFEPolicySet_vcs (8.96s)
=== RUN   TestAccTFEPolicySetImport
--- PASS: TestAccTFEPolicySetImport (7.94s)
PASS
ok      github.com/terraform-providers/terraform-provider-tfe/tfe       169.977s
?       github.com/terraform-providers/terraform-provider-tfe/version   [no test files]

resource tfe_run_trigger:

$ TESTARGS="-run TestAccTFERunTrigger" envchain terraform-provider-tfe make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFERunTrigger -timeout 15m
?       github.com/terraform-providers/terraform-provider-tfe   [no test files]
=== RUN   TestAccTFERunTrigger_basic
--- PASS: TestAccTFERunTrigger_basic (6.37s)
=== RUN   TestAccTFERunTrigger_basicWorkspaceID
--- PASS: TestAccTFERunTrigger_basicWorkspaceID (6.01s)
=== RUN   TestAccTFERunTrigger_updateWorkspaceExternalIDToWorkspaceID
--- PASS: TestAccTFERunTrigger_updateWorkspaceExternalIDToWorkspaceID (9.65s)
=== RUN   TestAccTFERunTriggerImport
--- PASS: TestAccTFERunTriggerImport (6.13s)
PASS
ok      github.com/terraform-providers/terraform-provider-tfe/tfe       33.900s
?       github.com/terraform-providers/terraform-provider-tfe/version   [no test files]

@lafentres lafentres changed the title [DRAFT] Deprecate workspace_external_id/workspace_external_ids in tfe_notification_configuration, tfe_policy_set, and tfe_run_trigger resources Deprecate workspace_external_id/workspace_external_ids in tfe_notification_configuration, tfe_policy_set, and tfe_run_trigger resources May 29, 2020
@lafentres lafentres force-pushed the lafentres/deprecate-external-ids-phase2 branch from e5822b0 to 6a0d8c1 Compare May 29, 2020 22:39
@lafentres lafentres removed the wip label May 29, 2020
@lafentres lafentres marked this pull request as ready for review May 29, 2020 22:43
@lafentres lafentres requested review from a team and mbfrahry May 29, 2020 22:43
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM! I just had a minor doc cleanup request but it looks good otherwise!

If you have made sure to change all references to this data source's `ids` attribute to the new `full_names` attribute, you can ignore the warning.

* `full_names` - A map of workspace names and their full names, which look like `<ORGANIZATION>/<WORKSPACE>`.
* `ids` - **Deprecated** Use `full_names` instead. A map of workspace names and their full names, which look like `<ORGANIZATION>/<WORKSPACE>`.
Copy link
Member

Choose a reason for hiding this comment

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

Any deprecated attribute can just be removed from the docs to help remove some of the clutter.

Copy link

@stasquatch stasquatch left a comment

Choose a reason for hiding this comment

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

Tested locally following your ✨ amazing ✨ test plan and everything looks good to me!

Copy link
Contributor

@sudomateo sudomateo left a comment

Choose a reason for hiding this comment

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

Great work on this! Minor wording/grammar changes requested for the changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -75,9 +96,18 @@ func resourceTFENotificationConfiguration() *schema.Resource {
},

"workspace_external_id": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Member

Choose a reason for hiding this comment

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

This is all looking awesome! There's only one thing that I can't quite map out in my head, and we'll use Notification Configurations here as an example.

Notification Configurations require a workspace (as the Required: true here enforces), but in this migration you're (having to) mark both of these as both optional and Computed, to allow the fields to be saved to state.

  • What does that look like then, when you try and create a fresh new Notification Configuration without the workspace? In the new case, not one that's existing and migrating.
  • What happens later after all this migration stuff is completed? Will workspace_id revert to Required: true, ForceNew: true, Computed: false? Is that problematic in any way...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does that look like then, when you try and create a fresh new Notification Configuration without the workspace? In the new case, not one that's existing and migrating.

if you do this, you'll get an error on apply. normally you'd get the error on plan but since i need to allow both attributes temporarily, i have to validate them myself later.
although i've set it to optional and computed, i still wrote checks on create that require workspace_id or workspace_external_id to be set.
the error looks like this:

Error: One of workspace_id or workspace_external_id must be set

  on main.tf line 51, in resource "tfe_notification_configuration" "notification_config":
  51: resource "tfe_notification_configuration" "notification_config" {

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens later after all this migration stuff is completed? Will workspace_id revert to Required: true, ForceNew: true, Computed: false? Is that problematic in any way...?

yep! workspace_id will revert to Required: true, ForceNew: true, and Computed: false. i don't think this should cause problems because:

  1. Required: although the attribute isn't required at the schema level, it is still required to be set in the schema for creation.
  2. ForceNew: the custom diff checks ensure that ForceNew is true right now in all cases except when the user is migrating from workspace_external_id to workspace_id (or back the other direction)
  3. Computed: this one is harder for me to articulate clearly, but right now, these are only computed so that both attributes get populated on read, that way anyone migrating won't get a plan/apply that tells them they have changes when they really don't.

@lafentres lafentres force-pushed the lafentres/deprecate-external-ids-phase2 branch from 3b5d7a7 to 91a5a98 Compare June 2, 2020 16:47
Co-authored-by: Matthew Sanabria <24284972+sudomateo@users.noreply.github.com>
@lafentres lafentres force-pushed the lafentres/deprecate-external-ids-phase2 branch from c3d2f28 to 1f7c629 Compare June 2, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants