New Data Source: aws_kms_secrets and add DeprecationMessage to aws_kms_secret data source #5195

bflad · 2018-07-14T16:38:07Z

Changes proposed in this pull request:

New Data Source: aws_kms_secrets -- the code acts very similar to the existing data source but changes from dynamic attributes to a static TypeMap attribute
Add DeprecationMessage to existing aws_kms_secret data source and provide migration documentation in the new data source

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSKmsSecretsDataSource_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSKmsSecretsDataSource_basic -timeout 120m
=== RUN   TestAccAWSKmsSecretsDataSource_basic
--- PASS: TestAccAWSKmsSecretsDataSource_basic (91.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	91.476s

apparentlymart

This looks good to me! I left some minor feedback inline about docs and warning output.

Since I'm pretty rusty on AWS provider development these days I suggest also having someone else look at the details here, since my review focused more on the user-facing schema, docs, etc than on the details of the AWS calls.

apparentlymart · 2018-07-16T15:53:29Z

aws/data_source_aws_kms_secret.go

@@ -13,7 +13,8 @@ import (

 func dataSourceAwsKmsSecret() *schema.Resource {
 	return &schema.Resource{
-		Read: dataSourceAwsKmsSecretRead,
+		DeprecationMessage: "This data source will change or be removed in Terraform AWS provider version 2.0. Please see migration information available in: https://www.terraform.io/docs/providers/aws/d/kms_secrets.html",


I don't know if we have precedent for provider-specific "upgrade guides" on the website, but I feel like it'd be better to gather that information on a version-specific upgrade page like we do for Core, since that way that information is obviously attached to a particular version of the provider and we won't get into a confusing situation where in future this page might no longer contain the migration information this message refers to but older builds of the provider are still out there in the wild.

If there isn't already a precedent for this then I guess we'll need to come up with a convention. My first idea would be a doc URL like /docs/providers/aws/upgrade-guides/1-28.html, which mimics our core-oriented upgrade guide URL structure /upgrade-guides/0-12.html, but I'm not wedded to that.

apparentlymart · 2018-07-16T15:58:24Z

website/docs/d/kms_secret.html.markdown

 description: |-
    Provides secret data encrypted with the KMS service
 ---

 # Data Source: aws_kms_secret

+!> **WARNING:** The `aws_kms_secret` data source contains behavior that cannot be supported on Terraform 0.12+ and it will either introduce breaking changes or be removed completely in the next major version (2.0.0) of the AWS provider. You can migrate existing Terraform configurations to the `aws_kms_secrets` data source, which closely resembles this data source, following instructions available in the [`aws_kms_secrets` data source documentation](/docs/providers/aws/d/kms_secrets.html).


This is subjective, but I feel like we should be more direct in what we are telling users to do here. Something like:

WARNING: This data source is deprecated and will be removed in a future version of the AWS provider. Use the replacement data source aws_kms_secrets instead for new configurations.

While indeed it's not necessarily true that it'll be removed entirely, I don't think the "may introduce changes or be removed" really adds anything to the main takeaway for this paragraph, which is "stop using this data source".

I agree with @apparentlymart here that he need a more direct way to say what is specifically happening when 0.12 drops

RulerOf · 2018-07-18T12:31:14Z

@bflad just want to confirm that, based on the documentation I read in your commits, this would definitely fix #5040

apparentlymart · 2018-07-21T01:29:07Z

@RulerOf assuming I understood your use-case correctly, I believe this would get you what you were looking for in #5040, albeit with the attribute called plaintext rather than result:

aws_kms_secrets.example.plaintext["my-secret-name"]

mbfrahry

LGTM. There is the note about moving some of the migration documentation out into it's own pages but that'll take more thought to get right

mbfrahry · 2018-07-24T02:55:53Z

website/docs/d/kms_secrets.html.markdown

+
+* `plaintext` - Map containing each `secret` `name` as the key with its decrypted plaintext value
+
+## Migrating From aws_kms_secret Data Source Prior to Terraform AWS Provider Version 2.0


I think this is going to far for this doc and needs to be moved to it's own Provider Version 2.0 migration page as @apparentlymart was suggesting. I think for right now this is fine but definitely a thing to think about as more resources require changes as this one does with major version bumps.

Add DeprecationMessage to existing aws_kms_secret data source

bflad · 2018-07-24T19:32:46Z

@apparentlymart / @mbfrahry I created an initial version 2 upgrade guide (bare bones with just this information for now but will eventually need the other 29+ existing deprecations) -- can you please take another look?

bflad · 2018-07-24T22:30:47Z

I've added most of the current attribute deprecations into the version 2 upgrade guide. 👍

apparentlymart

Some minor things inline. Feel free to ignore them or not; no need for another round of review from me, regardless.

Given the scope of the upgrade guide is similar to the changelog, it might work out logistically better to commit it separately like we do with changelog updates, but if you're able to turn around merging this quickly so that it doesn't end up becoming stale due to other changes then I guess it's a moot point.

apparentlymart · 2018-07-25T00:03:49Z

website/docs/d/kms_secret.html.markdown

 description: |-
    Provides secret data encrypted with the KMS service
 ---

 # Data Source: aws_kms_secret

+!> **WARNING:** This data source is deprecated and will be removed in the next major version (2.0.0) of the AWS provider. You can migrate existing Terraform configurations to the [`aws_kms_secrets` data source](/docs/providers/aws/d/kms_secrets.html) following instructions available in the [Terraform AWS Provider Version 2 Upgrade Guide](/docs/providers/aws/guides/version-2-upgrade.html).


Just a minor thing again, but since we're already in the context of the AWS provider I think it'd be fine to just say "the upgrade guide" here, to keep things concise. (Users are more likely to read this if it has fewer words in it.)

Given the length of that upgrade guide page, probably also best to link to the #data-sourceaws_kms_secret fragment so users don't get lost.

Great suggestions! Shorted the verbiage of the warning header more and deep linked to the proper section here and in the resource DeprecationMessage 👍

apparentlymart · 2018-07-25T00:07:25Z

website/docs/guides/version-2-upgrade.html.md

+
+<!-- /TOC -->
+
+## data-source/aws_iam_role


This "data-source/name" style is not something I've seen us use anywhere else. I'd probably instead just write out ## aws_iam_role data source to match how we usually talk about these things.

My brain was definitely in CHANGELOG mode here. 😅 Switched these to Data Source: X and Resource: X -- added bonus is that this made the anchor links prettier, e.g. #data-source-aws_kms_secret

apparentlymart · 2018-07-25T00:12:23Z

website/docs/guides/version-2-upgrade.html.md

+
+### assume_role_policy Attribute Removal
+
+Switch your attribute references to the `assume_role_policy` attribute instead.


This seems to be suggesting to replace the assume_role_policy name with itself. I assume one of these is supposed to say something else! 😀

Phew! Thanks for catching that copypasta mistake. 🍝 Fixed!

* docs/guides/version-2-upgrade: Use friendlier headers * data-source/aws_kms_secret: Link directly to migration section in deprecation notice * docs/data-source/aws_kms_secret: Link directly to migration section in deprecation notice * docs/guides/version-2-upgrade: Add guide introduction * docs/guides/version-2-upgrade: Add Provider Version Configuration section * docs/guides/version-2-upgrade: Fix assume_role_policy_document copy-paste typo

…ection for availability_zones Other minor changes: * Clarify Terraform run output to Terraform plan/apply output * Additional clarification that aws_redshift_cluster enable_logging became just enable under the logging argument

bflad · 2018-07-26T07:07:37Z

This has been released in version 1.29.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

… message References: * #5144 * #5195 The `aws_kms_secret` data source uses dynamic attribute functionality which is not supported in Terraform 0.12 and later. Available since Terraform AWS Provider version 1.29.0, operators should migrate to the `aws_kms_secrets` data source, which uses a `plaintext` map attribute. Additional information can be found in the Version 2 Upgrade Guide: https://www.terraform.io/docs/providers/aws/guides/version-2-upgrade.html#data-source-aws_kms_secret Output from Terraform 0.11 attempted usage: ``` terraform apply data.aws_kms_secret.testing: Refreshing state... Error: Error refreshing state: 1 error(s) occurred: * data.aws_kms_secret.testing: 1 error(s) occurred: * data.aws_kms_secret.testing: data.aws_kms_secret.testing: This data source has been replaced with the `aws_kms_secrets` data source. Upgrade information is available at: https://www.terraform.io/docs/providers/aws/guides/version-2-upgrade.html#data-source-aws_kms_secret ``` Output from Terraform 0.12 attempted usage (we cannot return the proper error message if there are references, however capturing the output here in case some one is searching for this in the future): ``` $ terraform apply Error: Unsupported block type on main.tf line 10, in data "aws_kms_secret" "testing": 10: context { Blocks of type "context" are not expected here. Did you mean to define argument "context"? If so, use the equals sign to assign it a value. $ terraform 0.12upgrade -yes $ terraform apply Error: Unsupported attribute on main.tf line 17, in output "testing": 17: value = data.aws_kms_secret.testing.secret_name This object has no argument, nested block, or exported attribute named "secret_name". ``` Output from Terraform 0.11 acceptance testing: ``` --- PASS: TestAccAWSKmsSecretDataSource_removed (2.05s) ``` Output from Terraform 0.12 acceptance testing: ``` --- PASS: TestAccAWSKmsSecretDataSource_removed (2.18s) ```

ghost · 2020-04-04T17:32:34Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

bflad added new-data-source Introduces a new data source. service/kms Issues and PRs that pertain to the kms service. labels Jul 14, 2018

bflad requested review from apparentlymart and a team July 14, 2018 16:38

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 14, 2018

bflad mentioned this pull request Jul 14, 2018

aws_kms_secret is using private SDK feature that will no longer work after Core 0.12 #5144

Closed

apparentlymart reviewed Jul 16, 2018

View reviewed changes

mbfrahry approved these changes Jul 24, 2018

View reviewed changes

bflad added 2 commits July 24, 2018 15:04

New Data Source: aws_kms_secrets

27b8f95

Add DeprecationMessage to existing aws_kms_secret data source

New Guide: AWS Provider Version 2 Upgrade

a56d48d

bflad force-pushed the td-aws_kms_secrets branch from e006236 to a56d48d Compare July 24, 2018 19:31

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 24, 2018

data-soruce/aws_kms_secret: Update migration URL

6dd1480

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 24, 2018

docs/guides/version-2-upgrade: Add currently Deprecated attributes

b4a05b9

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jul 24, 2018

apparentlymart approved these changes Jul 25, 2018

View reviewed changes

ghost added the size/XL Managed by automation to categorize the size of a PR. label Jul 25, 2018

bflad added this to the v1.29.0 milestone Jul 25, 2018

bflad merged commit 4b43b33 into master Jul 25, 2018

bflad deleted the td-aws_kms_secrets branch July 25, 2018 01:31

bflad added a commit that referenced this pull request Jul 25, 2018

Update CHANGELOG for #5195

3220a1a

bflad mentioned this pull request Feb 23, 2019

data-source/aws_kms_secret: Soft remove data source type with removal message #7657

Merged

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Data Source: aws_kms_secrets and add DeprecationMessage to aws_kms_secret data source #5195

New Data Source: aws_kms_secrets and add DeprecationMessage to aws_kms_secret data source #5195

bflad commented Jul 14, 2018

apparentlymart left a comment

apparentlymart Jul 16, 2018

apparentlymart Jul 16, 2018

mbfrahry Jul 24, 2018

RulerOf commented Jul 18, 2018

apparentlymart commented Jul 21, 2018

mbfrahry left a comment

mbfrahry Jul 24, 2018

bflad commented Jul 24, 2018

bflad commented Jul 24, 2018

apparentlymart left a comment

apparentlymart Jul 25, 2018

bflad Jul 25, 2018

apparentlymart Jul 25, 2018

bflad Jul 25, 2018

apparentlymart Jul 25, 2018

bflad Jul 25, 2018

bflad commented Jul 26, 2018

ghost commented Apr 4, 2020


		* `plaintext` - Map containing each `secret` `name` as the key with its decrypted plaintext value

		## Migrating From aws_kms_secret Data Source Prior to Terraform AWS Provider Version 2.0


		### assume_role_policy Attribute Removal

		Switch your attribute references to the `assume_role_policy` attribute instead.

New Data Source: aws_kms_secrets and add DeprecationMessage to aws_kms_secret data source #5195

New Data Source: aws_kms_secrets and add DeprecationMessage to aws_kms_secret data source #5195

Conversation

bflad commented Jul 14, 2018

apparentlymart left a 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

RulerOf commented Jul 18, 2018

apparentlymart commented Jul 21, 2018

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jul 24, 2018

bflad commented Jul 24, 2018

apparentlymart left a 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

bflad commented Jul 26, 2018

ghost commented Apr 4, 2020