-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
New Resource: aws_cognito_user_pool #1419
Conversation
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 :) |
Thanks @jtopper Will let you know on how it goes! |
56c6930
to
63c636c
Compare
@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 |
There was a problem hiding this 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?
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 |
63c636c
to
7e7867a
Compare
7e7867a
to
f38ec29
Compare
f38ec29
to
3752b89
Compare
Does this feature support import? |
7d7add3
to
7d17d1d
Compare
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! |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
},
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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{}) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 😵 😅
"Name" = "FooBar" | ||
"Project" = "Terraform" | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
8f3936b
to
1318ed5
Compare
Here we go!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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
TODOs