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

resource/cognito_user_pool_client: fix import with user pool id #3762

Merged
merged 3 commits into from
Apr 24, 2018

Conversation

loivis
Copy link
Contributor

@loivis loivis commented Mar 13, 2018

fix #3753

When describing user pool client, user_pool_id is required.

  1. Currently id is set as client_id, which makes it unable to import by default. Changing id to user_pool_id/client_id should be the best as I think. But that would be a breaking change?

  2. Instead, I updated import function to parse user_pool_id and client_id during import.

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 13, 2018
@loivis loivis force-pushed the 3753-cognito-user-pool-client-import branch from 18c0ad6 to fcef36f Compare March 13, 2018 16:28
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 13, 2018
@loivis loivis force-pushed the 3753-cognito-user-pool-client-import branch from fcef36f to 4449f32 Compare March 16, 2018 21:16
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 16, 2018
@loivis loivis force-pushed the 3753-cognito-user-pool-client-import branch from 4449f32 to 6c3191b Compare March 16, 2018 21:19
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 16, 2018
@loivis loivis force-pushed the 3753-cognito-user-pool-client-import branch from 3b5dec2 to 5b2def1 Compare March 16, 2018 22:28
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 16, 2018
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.

Sorry for the delay in getting this reviewed 😅 could you take a look at removing the extra lookup in the import test?


resourceName := "aws_cognito_user_pool_client.client"

getStateId := func(*terraform.State) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could shorten this lookup function if we set *client = ... in testAccCheckAWSCognitoUserPoolClientExists() from the return value of DescribeUserPoolClient()

Then we can simply grab the right IDs from an object we've already fetched in there 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for input. Not sure if I get your point exactly.

I've mostly copied the function and changed return values. Instead of parsing return value from DescribeUserPoolClient(), I directly return userPoolId and clientId set in DescribeUserPoolClientInput since there is error checking on DescribeUserPoolClient(). I assume that if there is no error then both IDs should be considered as valid.

@bflad bflad added the bug Addresses a defect in current functionality. label Apr 23, 2018
@loivis loivis force-pushed the 3753-cognito-user-pool-client-import branch from 5b2def1 to b5247ec Compare April 24, 2018 08:16
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 24, 2018
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.

Sorry I wasn't clear about what I meant earlier 😅 I can shorten it later. Let's get the more important fix in 🚀

3 tests passed (all tests)
=== RUN   TestAccAWSCognitoUserPoolClient_basic
--- PASS: TestAccAWSCognitoUserPoolClient_basic (8.12s)
=== RUN   TestAccAWSCognitoUserPoolClient_importBasic
--- PASS: TestAccAWSCognitoUserPoolClient_importBasic (8.48s)
=== RUN   TestAccAWSCognitoUserPoolClient_allFields
--- PASS: TestAccAWSCognitoUserPoolClient_allFields (8.62s)

@bflad bflad added this to the v1.16.0 milestone Apr 24, 2018
@bflad bflad merged commit b9b86c3 into hashicorp:master Apr 24, 2018
bflad added a commit that referenced this pull request Apr 24, 2018
@bflad
Copy link
Contributor

bflad commented Apr 25, 2018

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

@ghost
Copy link

ghost commented Apr 6, 2020

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 Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to import cognito user pool client
3 participants