-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
c15979e
to
3579e71
Compare
3579e71
to
e5822b0
Compare
…xternal_id on resource tfe_notification_configuration
…xternal_id on resource tfe_run_trigger
…external_ids on resource tfe_policy_set
e5822b0
to
6a0d8c1
Compare
There was a problem hiding this 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>`. |
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
There was a problem hiding this 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.
@@ -75,9 +96,18 @@ func resourceTFENotificationConfiguration() *schema.Resource { | |||
}, | |||
|
|||
"workspace_external_id": { | |||
Type: schema.TypeString, | |||
Required: true, | |||
ForceNew: true, |
There was a problem hiding this comment.
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 toRequired: true, ForceNew: true, Computed: false
? Is that problematic in any way...?
There was a problem hiding this comment.
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" {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
There was a problem hiding this comment.
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:
- Required: although the attribute isn't required at the schema level, it is still required to be set in the schema for creation.
- 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)
- 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.
3b5d7a7
to
91a5a98
Compare
Co-authored-by: Matthew Sanabria <24284972+sudomateo@users.noreply.github.com>
c3d2f28
to
1f7c629
Compare
see - hashicorp/terraform-provider-tfe#182 for more information.
Description
This PR adds deprecation warnings to the
ids
attribute for data sourcetfe_workspace_ids
, theworkspace_external_id
attributes for resourcestfe_notification_configuration
&tfe_run_trigger
, and theworkspace_external_ids
attribute for resourcetfe_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 usefull_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 useworkspace_id
insteadtfe_policy_set
-workspace_external_ids
is deprecated and will show a deprecation warning but will not cause any errors. You should useworkspace_ids
insteadtfe_run_trigger
-workspace_external_id
is deprecated and will show a deprecation warning but will not cause any errors. You should useworkspace_id
insteadWhy are these attributes being deprecated?
After #106 migrated workspace ids to the immutable id format (
ws-<random string>
), we still had some inconsistent usages ofexternal_id
,workspace_external_id
, andworkspace_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 ofworkspace_external_id
/workspace_external_ids
withworkspace_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 theids
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 theids
attribute explaining that users should begin usingfull_names
instead, as the behavior ofids
will be changing soon.Remainder of TFE Provider: We will add a new attribute to impacted resources called
workspace_id
orworkspace_ids
(depending on which is required for the specific resource) which will hold the same value as the currentworkspace_external_id
/workspace_external_ids
attributes. Deprecation warnings will be added to all of these, recommending the user move toworkspace_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 formatws-<random_string>
, just as theexternal_ids
attribute currently does. We will add a deprecation warning to theexternal_ids
attribute, alerting them that it will be deprecated soon and they should useids
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 behindids
returning the immutable workspace ID in the formatws-<random_string>
, andfull_names
returning the format<organization_name>/<workspace_name>
.Testing plan
To run through this test plan you'll need either:
OR
tfe_workspace_ids
and the resourcestfe_notification_configuration
,the_policy_set
(with workspace_external_ids set), &tfe_run_trigger
.Provision resources for all impacted resources and data sources
main.tf
, replacing values with your own information as needed.terraform init
andterraform version
. Your provider version should look report back as v0.17.1terraform apply
. This should succeed.terraform plan
. There should be no changes.Upgrade provider version to local build of this branch
~/.terraform.d/plugins
directory.main.tf
, delete your.terraform
folder.main.tf
remove or comment out theversion
in your tfe provider blockterraform init
andterraform version
. Your provider version should now be unversioned.terraform plan
. There should be no changes but you should see some deprecation warnings. Your output should look something like this:Make sure you can still update workspace_external_id/workspace_external_ids
workspace_external_id
in thetfe_notification_configuration
andtfe_run_trigger
resources to useworkspace-3
. Changeworkspace_external_ids
in thetfe_policy_set
resource to useworkspace-3
.terraform apply
. The output should showPlan: 2 to add, 1 to change, 2 to destroy.
. It should show that:tfe_notification_configuration.notification_config
must be replacedtfe_policy_set.policy-set
will be updated in-placetfe_run_trigger.run-trigger-1
must be replacedyes
to confirm the apply. This should succeed and the output should showApply complete! Resources: 2 added, 1 changed, 2 destroyed.
.terraform plan
. There should be no changes.workspace_external_id
in thetfe_notification_configuration
andtfe_run_trigger
resources back toworkspace-1
. Changeworkspace_external_ids
in thetfe_policy_set
resource backworkspace-1
.terraform apply
. This should succeed.terraform plan
. There should be no changes.Change workspace_external_id/workspace_external_ids to workspace_id/workspace_ids
workspace_external_id
in thetfe_notification_configuration
andtfe_run_trigger
resources toworkspace_id
. Changeworkspace_external_ids
in thetfe_policy_set
resource toworkspace_ids
terraform apply
. This should report no changes. Your output should look something like this:terraform plan
. There should be no changes.Make sure you can still update workspace_id/workspace_ids
workspace_id
in thetfe_notification_configuration
andtfe_run_trigger
resources to useworkspace-3
. Changeworkspace_ids
in thetfe_policy_set
resource to useworkspace-3
.terraform apply
. The output should showPlan: 2 to add, 1 to change, 2 to destroy.
. It should show that:tfe_notification_configuration.notification_config
must be replacedtfe_policy_set.policy-set
will be updated in-placetfe_run_trigger.run-trigger-1
must be replacedyes
to confirm the apply. This should succeed and the output should showApply complete! Resources: 2 added, 1 changed, 2 destroyed.
.terraform plan
. There should be no changes.Test out the new
full_names
attribute:full_names
attributeterraform apply
. Make sure the outputsfull_names
andworkspace_ids
are the same. Your output should look like this:Make sure destroy still works
terraform destroy
. This should succeed.Make sure a fresh create on this new version works
terraform apply
. This should succeed.Check the documentation and CHANGELOG:
External links
Output from acceptance tests
data source tfe_workspace_ids:
resource tfe_notification_configuration:
resource tfe_policy_set:
resource tfe_run_trigger: