Various aws_cognito_identity_provider improvements #10705

tomelliff · 2019-11-01T01:39:50Z

Added plan time checks for some parameters and also ignored the computed diff caused by not specifying the optional attribute_mapping.

Came across this while idiotically missing that the resource already existed and then trying to implement it from scratch in #10675. Noticed that this resource was still missing a few useful things so figured I could at least salvage those parts.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

Bug fix:
resource/aws_cognito_identity_provider: Validate parameters at plan time and avoid diff from unspecified `attribute_mapping`.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCognitoIdentityProvider_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSCognitoIdentityProvider_basic -timeout 120m
go: downloading github.com/hashicorp/terraform-plugin-sdk v1.2.0
go: downloading github.com/aws/aws-sdk-go v1.25.24
go: extracting github.com/hashicorp/terraform-plugin-sdk v1.2.0
go: extracting github.com/aws/aws-sdk-go v1.25.24
go: finding github.com/hashicorp/terraform-plugin-sdk v1.2.0
go: finding github.com/aws/aws-sdk-go v1.25.24
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCognitoIdentityProvider_basic
=== PAUSE TestAccAWSCognitoIdentityProvider_basic
=== CONT  TestAccAWSCognitoIdentityProvider_basic
--- PASS: TestAccAWSCognitoIdentityProvider_basic (87.87s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	87.899s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap	0.001s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags	0.010s [no tests to run]

Added plan time checks for some parameters and also ignored the computed diff caused by not specifying the optional attribute_mapping.

bflad

Hey @tomelliff 👋 Thank you for submitting this and sorry for the delayed review. Can you please rebase this against master and make these small adjustments? Thanks.

bflad · 2020-03-17T05:33:00Z

aws/resource_aws_cognito_identity_provider.go

+				Elem: &schema.Schema{
+					Type:         schema.TypeString,
+					ValidateFunc: validation.StringLenBetween(1, 32),
+				},


In the API reference, this refers to this validation as:

Key Length Constraints: Minimum length of 1. Maximum length of 32.

Using ValidateFunc within Elem I believe would be validating the map values, not the keys.

Suggested change

Elem: &schema.Schema{

Type: schema.TypeString,

ValidateFunc: validation.StringLenBetween(1, 32),

},

Elem: &schema.Schema{

Type: schema.TypeString,

},

bflad · 2020-03-17T05:35:20Z

aws/resource_aws_cognito_identity_provider.go

+					cognitoidentityprovider.IdentityProviderTypeTypeFacebook,
+					cognitoidentityprovider.IdentityProviderTypeTypeGoogle,
+					cognitoidentityprovider.IdentityProviderTypeTypeLoginWithAmazon,
+					cognitoidentityprovider.IdentityProviderTypeTypeOidc,


Since this was introduced, there is an additional value, according to the SDK constants:

Suggested change

cognitoidentityprovider.IdentityProviderTypeTypeOidc,

cognitoidentityprovider.IdentityProviderTypeTypeOidc,

cognitoidentityprovider.IdentityProviderTypeTypeSignInWithApple,

bflad · 2020-03-17T05:35:58Z

aws/resource_aws_cognito_identity_provider.go

+				Elem: &schema.Schema{
+					Type:         schema.TypeString,
+					ValidateFunc: validation.StringLenBetween(1, 32),
+				},
 			},

 			"idp_identifiers": {
 				Type:     schema.TypeList,
 				Optional: true,


While we're updating here, seems like this should include MaxItems: 50 according to the API Reference

Suggested change

Optional: true,

Optional: true,

MaxItems: 50,

bflad

Hi again @tomelliff 👋 Since we didn't hear back from you, we implemented the review feedback in a followup commit. Thank you for starting this.

Output from acceptance testing:

--- PASS: TestAccAWSCognitoIdentityProvider_basic (33.86s)

Reference: #10705 (review) Output from acceptance testing: ``` --- PASS: TestAccAWSCognitoIdentityProvider_basic (33.86s) ```

ghost · 2020-04-17T00:44:29Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

tomelliff · 2020-04-17T11:41:14Z

Apologies, only just saw this when I saw it had been released 😄

Thanks for sorting the review feedback and sorry I missed the notification on the feedback.

ghost · 2020-05-15T17:10:13Z

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!

Various aws_cognito_identity_provider improvements

b5a75a3

Added plan time checks for some parameters and also ignored the computed diff caused by not specifying the optional attribute_mapping.

tomelliff requested a review from a team November 1, 2019 01:39

ghost added size/M Managed by automation to categorize the size of a PR. service/cognito tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 1, 2019

bflad suggested changes Mar 17, 2020

View reviewed changes

bflad self-assigned this Mar 17, 2020

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 17, 2020

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Apr 15, 2020

bflad added this to the v2.58.0 milestone Apr 15, 2020

bflad reviewed Apr 15, 2020

View reviewed changes

bflad added a commit that referenced this pull request Apr 15, 2020

resource/aws_cognito_identity_provider: Address PR #10705 feedback

87996d6

Reference: #10705 (review) Output from acceptance testing: ``` --- PASS: TestAccAWSCognitoIdentityProvider_basic (33.86s) ```

bflad merged commit 19ecf49 into hashicorp:master Apr 15, 2020

bflad added a commit that referenced this pull request Apr 15, 2020

Update CHANGELOG for #10705

e73ee3d

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 17, 2020

ghost locked and limited conversation to collaborators May 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various aws_cognito_identity_provider improvements #10705

Various aws_cognito_identity_provider improvements #10705

tomelliff commented Nov 1, 2019

bflad left a comment

bflad Mar 17, 2020

bflad Mar 17, 2020

bflad Mar 17, 2020

bflad left a comment

ghost commented Apr 17, 2020

tomelliff commented Apr 17, 2020

ghost commented May 15, 2020

	cognitoidentityprovider.IdentityProviderTypeTypeOidc,
	cognitoidentityprovider.IdentityProviderTypeTypeOidc,
	cognitoidentityprovider.IdentityProviderTypeTypeSignInWithApple,

Various aws_cognito_identity_provider improvements #10705

Various aws_cognito_identity_provider improvements #10705

Conversation

tomelliff commented Nov 1, 2019

Community Note

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 17, 2020

Choose a reason for hiding this comment

bflad Mar 17, 2020

Choose a reason for hiding this comment

bflad Mar 17, 2020

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

ghost commented Apr 17, 2020

tomelliff commented Apr 17, 2020

ghost commented May 15, 2020