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.

Sign up for GitHub

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

Jump to bottom

Send all attributes when updating a cognito_identity_pool resource #9396

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Send all attributes when updating a cognito_identity_pool resource #9396

merged 1 commit into from
Jul 17, 2019

Conversation

mbudnek
Copy link
Contributor

@mbudnek mbudnek commented Jul 17, 2019

The UpdateIdentityPool API expects all attributes to be present. It
treats missing attributes as a request to remove those attributes from
the identity pool.

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

Fixes #9394

Release note for CHANGELOG:

NONE

Output from acceptance testing:

Newly added test fails on master:

miles@mbudnek-desktop:~/git/terraform-provider-aws$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider -timeout 120m
=== RUN   TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider
--- FAIL: TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider (24.35s)
    testing.go:568: Step 1 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: aws_cognito_identity_pool.main
          allow_unauthenticated_identities:                     "false" => "false"
          arn:                                                  "arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2" => "arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2"
          cognito_identity_providers.#:                         "0" => "2"
          cognito_identity_providers.0.client_id:               "" => "7lhlkkfbfb4q5kpp90urffao"
          cognito_identity_providers.0.provider_name:           "" => "cognito-idp.us-east-1.amazonaws.com/us-east-1_Ab129faBb"
          cognito_identity_providers.0.server_side_token_check: "" => "false"
          cognito_identity_providers.1.client_id:               "" => "7lhlkkfbfb4q5kpp90urffao"
          cognito_identity_providers.1.provider_name:           "" => "cognito-idp.us-east-1.amazonaws.com/us-east-1_Zr231apJu"
          cognito_identity_providers.1.server_side_token_check: "" => "false"
          developer_provider_name:                              "" => ""
          id:                                                   "us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2" => "us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2"
          identity_pool_name:                                   "identity pool qxit7yoirk" => "identity pool qxit7yoirk"
          openid_connect_provider_arns.#:                       "1" => "1"
          openid_connect_provider_arns.0:                       "arn:aws:iam::123456789012:oidc-provider/server.example.com" => "arn:aws:iam::123456789012:oidc-provider/server.example.com"
          saml_provider_arns.#:                                 "0" => "0"



        STATE:

        aws_cognito_identity_pool.main:
          ID = us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2
          provider = provider.aws
          allow_unauthenticated_identities = false
          arn = arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:5cec08a9-f0a5-4ebd-ab14-74cbca7082b2
          developer_provider_name =
          identity_pool_name = identity pool qxit7yoirk
          openid_connect_provider_arns.# = 1
          openid_connect_provider_arns.0 = arn:aws:iam::123456789012:oidc-provider/server.example.com
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	24.394s
make: *** [GNUmakefile:20: testacc] Error 1

Passes on this branch:

miles@mbudnek-desktop:~/git/terraform-provider-aws$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider -timeout 120m
=== RUN   TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider
--- PASS: TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider (34.57s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	34.609s

One existing Cognito Identity Pool related test fails on both master and this branch:

miles@mbudnek-desktop:~/git/terraform-provider-aws$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoIdentityPool_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSCognitoIdentityPool_ -timeout 120m
=== RUN   TestAccAWSCognitoIdentityPool_importBasic
=== PAUSE TestAccAWSCognitoIdentityPool_importBasic
=== RUN   TestAccAWSCognitoIdentityPool_basic
=== PAUSE TestAccAWSCognitoIdentityPool_basic
=== RUN   TestAccAWSCognitoIdentityPool_supportedLoginProviders
=== PAUSE TestAccAWSCognitoIdentityPool_supportedLoginProviders
=== RUN   TestAccAWSCognitoIdentityPool_openidConnectProviderArns
=== PAUSE TestAccAWSCognitoIdentityPool_openidConnectProviderArns
=== RUN   TestAccAWSCognitoIdentityPool_samlProviderArns
=== PAUSE TestAccAWSCognitoIdentityPool_samlProviderArns
=== RUN   TestAccAWSCognitoIdentityPool_cognitoIdentityProviders
=== PAUSE TestAccAWSCognitoIdentityPool_cognitoIdentityProviders
=== RUN   TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider
--- PASS: TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider (36.44s)
=== CONT  TestAccAWSCognitoIdentityPool_importBasic
=== CONT  TestAccAWSCognitoIdentityPool_cognitoIdentityProviders
=== CONT  TestAccAWSCognitoIdentityPool_supportedLoginProviders
=== CONT  TestAccAWSCognitoIdentityPool_samlProviderArns
=== CONT  TestAccAWSCognitoIdentityPool_openidConnectProviderArns
=== CONT  TestAccAWSCognitoIdentityPool_basic
--- PASS: TestAccAWSCognitoIdentityPool_importBasic (14.84s)
--- FAIL: TestAccAWSCognitoIdentityPool_openidConnectProviderArns (19.88s)
    testing.go:568: Step 1 error: After applying this step, the plan was not empty:

        DIFF:

        UPDATE: aws_cognito_identity_pool.main
          allow_unauthenticated_identities: "false" => "false"
          arn:                              "arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6" => "arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6"
          cognito_identity_providers.#:     "0" => "0"
          developer_provider_name:          "" => ""
          id:                               "us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6" => "us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6"
          identity_pool_name:               "identity pool a9stdtokkw" => "identity pool a9stdtokkw"
          openid_connect_provider_arns.#:   "2" => "2"
          openid_connect_provider_arns.0:   "arn:aws:iam::123456789012:oidc-provider/bar.example.com" => "arn:aws:iam::123456789012:oidc-provider/foo.example.com"
          openid_connect_provider_arns.1:   "arn:aws:iam::123456789012:oidc-provider/foo.example.com" => "arn:aws:iam::123456789012:oidc-provider/bar.example.com"
          saml_provider_arns.#:             "0" => "0"



        STATE:

        aws_cognito_identity_pool.main:
          ID = us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6
          provider = provider.aws
          allow_unauthenticated_identities = false
          arn = arn:aws:cognito-identity:us-west-2:116127484844:identitypool/us-west-2:333f51cd-86bc-46bb-ae3a-30932fe5f5f6
          developer_provider_name =
          identity_pool_name = identity pool a9stdtokkw
          openid_connect_provider_arns.# = 2
          openid_connect_provider_arns.0 = arn:aws:iam::123456789012:oidc-provider/bar.example.com
          openid_connect_provider_arns.1 = arn:aws:iam::123456789012:oidc-provider/foo.example.com
--- PASS: TestAccAWSCognitoIdentityPool_basic (23.63s)
--- PASS: TestAccAWSCognitoIdentityPool_supportedLoginProviders (33.19s)
--- PASS: TestAccAWSCognitoIdentityPool_cognitoIdentityProviders (33.36s)
--- PASS: TestAccAWSCognitoIdentityPool_samlProviderArns (33.40s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	69.892s
make: *** [GNUmakefile:20: testacc] Error 1

It looks like it's just shuffling the order of some attributes between runs. Maybe related to #5704, but I didn't investigate all that much.

@mbudnek
Send all attributes when updating a cognito_identity_pool resource
dc3f3f8 
The UpdateIdentityPool API expects all attributes to be present.  It
treats missing attributes as a request to remove those attributes from
the identity pool.
@mbudnek mbudnek requested a review from a team July 17, 2019 22:41
@ghost 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 Jul 17, 2019
@bflad bflad added the bug Addresses a defect in current functionality. label Jul 17, 2019
bflad
bflad approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @mbudnek 👋 Thank you so much for your contribution here along with the covering testing. I'm unfortunately familiar with the previous fixes for the other Cognito resources and this looks good to me. I can also confirm that TestAccAWSCognitoIdentityPool_openidConnectProviderArns is also failing on master and unrelated to this change.

Output from acceptance testing:

--- PASS: TestAccAWSCognitoIdentityPool_importBasic (11.23s)
--- FAIL: TestAccAWSCognitoIdentityPool_openidConnectProviderArns (14.98s)
--- PASS: TestAccAWSCognitoIdentityPool_basic (15.97s)
--- PASS: TestAccAWSCognitoIdentityPool_addingNewProviderKeepsOldProvider (18.89s)
--- PASS: TestAccAWSCognitoIdentityPool_cognitoIdentityProviders (22.05s)
--- PASS: TestAccAWSCognitoIdentityPool_supportedLoginProviders (22.16s)
--- PASS: TestAccAWSCognitoIdentityPool_samlProviderArns (24.26s)

@bflad bflad added this to the v2.20.0 milestone Jul 17, 2019
@bflad bflad merged commit 8ec4b4f into hashicorp:master Jul 17, 2019
bflad added a commit that referenced this pull request Jul 17, 2019
@bflad
Update CHANGELOG for #9396
0446bf0
@ghost
Copy link

ghost commented Nov 2, 2019

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!

@ghost ghost locked and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.20.0
Development

Successfully merging this pull request may close these issues.

Adding an OIDC provider to a Cognito Identity Pool that has a Cognito provider removes the Cognito provider
2 participants
@mbudnek @bflad