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

New Resource: aws_cognito_user_pool #1419

Merged
merged 30 commits into from
Nov 16, 2017

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Aug 15, 2017

Description

This implements Cognito User pool with original work from @Harrison-Miller.
This work is missing a few attributes & options that will be added ASAP.

API Documentation

Test

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoUserPool_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserPool_ -timeout 120m
=== RUN   TestAccAWSCognitoUserPool_importBasic
--- PASS: TestAccAWSCognitoUserPool_importBasic (17.59s)
=== RUN   TestAccAWSCognitoUserPool_basic
--- PASS: TestAccAWSCognitoUserPool_basic (14.16s)
=== RUN   TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration
--- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (24.01s)
=== RUN   TestAccAWSCognitoUserPool_withDeviceConfiguration
--- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (24.74s)
=== RUN   TestAccAWSCognitoUserPool_withEmailVerificationMessage
--- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (23.63s)
=== RUN   TestAccAWSCognitoUserPool_withSmsVerificationMessage
--- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (22.74s)
=== RUN   TestAccAWSCognitoUserPool_withEmailConfiguration
--- PASS: TestAccAWSCognitoUserPool_withEmailConfiguration (23.38s)
=== RUN   TestAccAWSCognitoUserPool_withSmsConfiguration
--- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (34.88s)
=== RUN   TestAccAWSCognitoUserPool_withSmsConfigurationUpdated
--- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (41.22s)
=== RUN   TestAccAWSCognitoUserPool_withTags
--- PASS: TestAccAWSCognitoUserPool_withTags (23.02s)
=== RUN   TestAccAWSCognitoUserPool_withAliasAttributes
--- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (25.68s)
=== RUN   TestAccAWSCognitoUserPool_withPasswordPolicy
--- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (23.89s)
=== RUN   TestAccAWSCognitoUserPool_withLambdaConfig
--- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (45.36s)
=== RUN   TestAccAWSCognitoUserPool_withSchemaAttributes
--- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (24.43s)
=== RUN   TestAccAWSCognitoUserPool_withVerificationMessageTemplate
--- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (27.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	396.355s

TODOs

  • Handle MFA
  • Handle Admin options
  • Handle Admin acc tests
  • Handle Email Configurations
  • Handle Email Configurations acc tests
  • Handle Device Configuration
  • Handle Device Configuration acc tests
  • Handle SMS Configurations
  • Handle SMS Configurations acc tests
  • Handle Schema attributes
  • Handle Schema attributes acc tests
  • Handle Password policy
  • Handle Password policy acc tests
  • Handle Lambda Config
  • Handle Lambda Config acc tests
  • Handle Verification Message Template
  • Handle Verification Message Template acc tests
  • Handle Username attributes
  • Improve the documentation
  • Handle Import

@Ninir Ninir added the new-resource Introduces a new resource. label Aug 15, 2017
@grubernaut grubernaut added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 15, 2017
@jtopper
Copy link

jtopper commented Aug 15, 2017

In case you need any encouragement @Ninir, we came up against this missing feature this week too, and we're keen to see it finished and merged :)

@Ninir
Copy link
Contributor Author

Ninir commented Aug 16, 2017

Thanks @jtopper

Will let you know on how it goes!
Trying to implement every available option, documentation, validating... lot of work on that :D

@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch 2 times, most recently from 56c6930 to 63c636c Compare August 16, 2017 20:36
@psyvision
Copy link
Contributor

@Ninir Will this work also allow the creation of app clients? http://docs.aws.amazon.com/cli/latest/reference/cognito-idp/create-user-pool-client.html

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

Hey @Ninir how do you feel about this PR?

@psyvision
Copy link
Contributor

I've started work on user pool clients in the following fork. I need to add some unit tests and will then submit a pull request: https://github.com/psyvision/terraform-provider-aws

@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch from 63c636c to 7e7867a Compare October 23, 2017 13:03
@radeksimko radeksimko added this to the v1.2.0 milestone Oct 26, 2017
@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch from 7e7867a to f38ec29 Compare October 30, 2017 22:24
@radeksimko radeksimko modified the milestones: v1.2.0, v1.3.0 Oct 31, 2017
@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch from f38ec29 to 3752b89 Compare November 2, 2017 19:28
@dave-lyric
Copy link

dave-lyric commented Nov 7, 2017

Does this feature support import?

@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch from 7d7add3 to 7d17d1d Compare November 10, 2017 16:08
@Ninir
Copy link
Contributor Author

Ninir commented Nov 13, 2017

Hi @dave-lyric

It does support it now :)

For any folk around here: I still need to manage AdminCreateUserConfig tests & play a bit with all the edge use-cases... but it's almost mergeable :)

Thanks for waiting!

@radeksimko radeksimko added the size/XXL Managed by automation to categorize the size of a PR. label Nov 15, 2017
@radeksimko radeksimko changed the title [WIP] Added Cognito User Pool [WIP] New Resource: aws_cognito_user_pool Nov 15, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I left you a bunch of nitpicks around error handling and a few more important questions in the schema.

Overall this is looking really good, huge respect for getting it done! 👍

Type: schema.TypeList,
Optional: true,
Computed: true,
MinItems: 0,
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but 0 is default here, so it seems unnecessary.

"admin_create_user_config": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity what's the value that comes back from the API if not defined?

Copy link
Contributor Author

@Ninir Ninir Nov 16, 2017

Choose a reason for hiding this comment

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

The structure gets back with children having default values:

"AdminCreateUserConfig": {
            "UnusedAccountValidityDays": 7, 
            "AllowAdminCreateUserOnly": false
        }, 

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough - as long as Computed is justified 🤷‍♂️

"invite_message_template": {
Type: schema.TypeList,
Optional: true,
MinItems: 0,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, ^ likewise

},

"alias_attributes": {
Type: schema.TypeList,
Copy link
Member

Choose a reason for hiding this comment

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

I assume the ordering is important here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you write it down, yeh it seems so!

},

"auto_verified_attributes": {
Type: schema.TypeList,
Copy link
Member

Choose a reason for hiding this comment

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

Is the order important here?

aws/structure.go Outdated
data := v.([]interface{})

if len(data) > 0 {
m, _ := data[0].(map[string]interface{})
Copy link
Member

Choose a reason for hiding this comment

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

Why are we ignoring the cast error here?

aws/structure.go Outdated
}

func expandCognitoUserPoolSchema(inputs []interface{}) []*cognitoidentityprovider.SchemaAttributeType {
configs := make([]*cognitoidentityprovider.SchemaAttributeType, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but can't we already allocate fixed-size array here, since we know how many inputs are coming in? i.e.

configs := make([]*cognitoidentityprovider.SchemaAttributeType, len(inputs), len(inputs))

then you'd just use

configs[i] = config

instead of append.
But it's really a nitpick, just a matter of memory efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! :)

aws/structure.go Outdated
}

func flattenCognitoUserPoolSchema(inputs []*cognitoidentityprovider.SchemaAttributeType) []map[string]interface{} {
values := make([]map[string]interface{}, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - just nitpick about memory efficiency here.

es = append(es, fmt.Errorf("%q cannot be longer than 2000 characters", k))
}

if !regexp.MustCompile(`[\p{L}\p{M}\p{S}\p{N}\p{P}\s*]*\{####\}[\p{L}\p{M}\p{S}\p{N}\p{P}\s*]*`).MatchString(value) {
Copy link
Member

Choose a reason for hiding this comment

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

🙈 😵 😅

"Name" = "FooBar"
"Project" = "Terraform"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more minimalistic example we can put here or are those all required arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are not even needed: just wanted to provide a good & "working" example. Thought we might want a basic one and this "verbosed" one. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

How about keeping the "verbose" example in /examples?

@radeksimko radeksimko changed the title [WIP] New Resource: aws_cognito_user_pool New Resource: aws_cognito_user_pool Nov 15, 2017
@Ninir Ninir force-pushed the f-cognito-identity-provider-user-pool branch from 8f3936b to 1318ed5 Compare November 16, 2017 16:15
@Ninir
Copy link
Contributor Author

Ninir commented Nov 16, 2017

Here we go!

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoUserPool_'                               
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserPool_ -timeout 120m
=== RUN   TestAccAWSCognitoUserPool_importBasic
--- PASS: TestAccAWSCognitoUserPool_importBasic (18.32s)
=== RUN   TestAccAWSCognitoUserPool_basic
--- PASS: TestAccAWSCognitoUserPool_basic (15.86s)
=== RUN   TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration
--- PASS: TestAccAWSCognitoUserPool_withAdminCreateUserConfiguration (27.09s)
=== RUN   TestAccAWSCognitoUserPool_withDeviceConfiguration
--- PASS: TestAccAWSCognitoUserPool_withDeviceConfiguration (27.78s)
=== RUN   TestAccAWSCognitoUserPool_withEmailVerificationMessage
--- PASS: TestAccAWSCognitoUserPool_withEmailVerificationMessage (25.44s)
=== RUN   TestAccAWSCognitoUserPool_withSmsVerificationMessage
--- PASS: TestAccAWSCognitoUserPool_withSmsVerificationMessage (25.97s)
=== RUN   TestAccAWSCognitoUserPool_withEmailConfiguration
--- PASS: TestAccAWSCognitoUserPool_withEmailConfiguration (26.41s)
=== RUN   TestAccAWSCognitoUserPool_withSmsConfiguration
--- PASS: TestAccAWSCognitoUserPool_withSmsConfiguration (35.05s)
=== RUN   TestAccAWSCognitoUserPool_withSmsConfigurationUpdated
--- PASS: TestAccAWSCognitoUserPool_withSmsConfigurationUpdated (44.02s)
=== RUN   TestAccAWSCognitoUserPool_withTags
--- PASS: TestAccAWSCognitoUserPool_withTags (27.75s)
=== RUN   TestAccAWSCognitoUserPool_withAliasAttributes
--- PASS: TestAccAWSCognitoUserPool_withAliasAttributes (27.68s)
=== RUN   TestAccAWSCognitoUserPool_withPasswordPolicy
--- PASS: TestAccAWSCognitoUserPool_withPasswordPolicy (26.64s)
=== RUN   TestAccAWSCognitoUserPool_withLambdaConfig
--- PASS: TestAccAWSCognitoUserPool_withLambdaConfig (48.30s)
=== RUN   TestAccAWSCognitoUserPool_withSchemaAttributes
--- PASS: TestAccAWSCognitoUserPool_withSchemaAttributes (25.72s)
=== RUN   TestAccAWSCognitoUserPool_withVerificationMessageTemplate
--- PASS: TestAccAWSCognitoUserPool_withVerificationMessageTemplate (30.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	432.959s

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

tenor

@Ninir Ninir merged commit d6d2147 into hashicorp:master Nov 16, 2017
@Ninir Ninir deleted the f-cognito-identity-provider-user-pool branch November 16, 2017 18:27
@ghost
Copy link

ghost commented Apr 10, 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 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. size/XXL Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants