New Resource: aws_cognito_user_group #3010

tomelliff · 2018-01-16T19:52:00Z

Added support for AWS Cognito User Groups.

Should close #2865 and I think might be the last thing left on #232

tomelliff · 2018-01-16T19:56:23Z

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSCognitoUserGroup_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserGroup_ -timeout 120m
=== RUN   TestAccAWSCognitoUserGroup_basic
--- PASS: TestAccAWSCognitoUserGroup_basic (91.23s)
=== RUN   TestAccAWSCognitoUserGroup_complex
--- PASS: TestAccAWSCognitoUserGroup_complex (75.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	166.304s

GreenSmile · 2018-01-19T17:27:48Z

Hi, thanks for adding cognito groups!

I was preparing similar pull request (#3025). Please consider possibility to apply the following changes to your PR:

Please add documentation
Please add fields validation to match AWS requirements
I believe debug output before every AWS call could be rather useful
I think resource should be named "aws_cognito_user_pool_group", similarly to "aws_cognito_user_pool_client" and "aws_cognito_user_pool_domain". However, I'd like to hear comments from core members on this one
It maybe useful to include UserPoolId to resource ID, because group name is not unique. Resource ID like us-east-1_esGpht5Fh/Admins looks self-explanatory.
It would be really nice to have possibility to import existent group like
terraform import aws_cognito_user_pool_group.admin_group us-east-1_esGpht5Fh/Admins. Not sure, however, how difficult it is.

Please check my PR for example of all points, except the very last one.

tomelliff · 2018-01-21T15:24:37Z

@GreenSmile Hey, sorry, I had added the docs and validation during the week but while I was on the train without internet and apparently forgot to push until I just saw your comment.

I'll go through your review comments now, thanks for the comments, particularly on the Id which is a good catch that I didn't think about clearly enough because the API docs states that it has to be unique but in hindsight it clearly means unique for the user pool.

Also added debug output as per pull request feedback.

tomelliff · 2018-01-21T19:05:32Z

@GreenSmile I think I've covered everything you talked about but some in slightly different ways to how you approached things by the looks of it, especially around the import.

I'm unsure about changing from user_group to user_pool_group but happy to change if others feel it's more obvious that way.

Tests seem okay and I've done a bit of manual testing as well:

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSCognitoUserGroup_"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserGroup_ -timeout 120m
=== RUN   TestAccAWSCognitoUserGroup_basic
--- PASS: TestAccAWSCognitoUserGroup_basic (91.41s)
=== RUN   TestAccAWSCognitoUserGroup_complex
--- PASS: TestAccAWSCognitoUserGroup_complex (79.48s)
=== RUN   TestAccAWSCognitoUserGroup_importBasic
--- PASS: TestAccAWSCognitoUserGroup_importBasic (49.76s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	220.668s

I've seen the merge conflict on the validators but will leave it for now until there's been a review and then I'll fix that merge conflict.

radeksimko

Hi @tomelliff
thanks for this PR.

This is looking pretty good! Just a few comments, two of which (about names used in tests) I'd certainly want to be addressed prior to merging.

radeksimko · 2018-02-04T14:32:10Z

aws/resource_aws_cognito_user_group_test.go

+func TestAccAWSCognitoUserGroup_basic(t *testing.T) {
+	poolName := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
+	groupName := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
+	updatedGroupName := acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)


Do you mind prepending a unique fixed prefix to each name? e.g. tf_acc_? This is to make it easy/possible to cleanup any potentially leaked resources from the test and make it easier to identify those leaks in general.

radeksimko · 2018-02-04T14:32:31Z

aws/resource_aws_cognito_user_group_test.go

+func TestAccAWSCognitoUserGroup_complex(t *testing.T) {
+	poolName := fmt.Sprintf("%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
+	groupName := fmt.Sprintf("%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
+	updatedGroupName := fmt.Sprintf("%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))


radeksimko · 2018-02-04T14:34:08Z

aws/resource_aws_cognito_user_group_test.go

+}
+
+resource "aws_cognito_user_group" "main" {
+  name         = "user-group-%s"


Nitpick: It's usually better to keep the whole name (incl. the prefix like user-group- here) in the test body as we may then reference it in checks, instead of rebuilding it again. 😉

radeksimko · 2018-02-04T14:39:31Z

aws/resource_aws_cognito_user_group.go

+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/cognitoidentityprovider"
+	"github.com/hashicorp/errwrap"


Nitpick: I know it's used all over the AWS provider at the moment, but in most places for no particular reason. The library is intended to address the problem of wrapping errors into errors so that we can retain all data about the original error. Here we're just wrapping errors to print them out later. In other words I think we don't need this library unless we use Contains() or ContainsType() per https://github.com/hashicorp/errwrap#usage

Conflicts: aws/validators_test.go

tomelliff · 2018-02-06T09:40:41Z

@radeksimko Thanks for the review. Think I've covered those points now and I've fixed the merge conflict on the validators_test as well.

radeksimko

LGTM, thanks.

bflad · 2018-02-09T22:27:37Z

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

jayanderson · 2018-02-16T00:58:54Z

https://www.terraform.io/docs/providers/aws/r/cognito_user_group.html - the documentation page looks a bit messed up for this feature. It isn't rendering the index sidebar and has some raw text at the top.

radeksimko · 2018-02-16T06:01:36Z

@jayanderson Thanks for noticing. See #3411

ghost · 2020-04-07T17:59:21Z

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!

tomelliff force-pushed the cognito-user-group branch from d25709b to 18c2091 Compare January 16, 2018 19:54

bflad added new-resource Introduces a new resource. service/cognito labels Jan 16, 2018

New resource aws_cognito_user_group

8c14196

tomelliff force-pushed the cognito-user-group branch from 18c2091 to 8c14196 Compare January 16, 2018 20:05

Add docs for aws_cognito_user_group resource

d15c3c1

radeksimko changed the title ~~New resource: Cognito user group~~ New Resource: aws_cognito_user_group Jan 17, 2018

GreenSmile mentioned this pull request Jan 17, 2018

Add Cognito Group resource #3025

Closed

Add validation for aws_cognito_user_group

9d7e620

aws_cognito_user_group id should contain the user pool id

abf8f48

Also added debug output as per pull request feedback.

tomelliff force-pushed the cognito-user-group branch from b2faf82 to abf8f48 Compare January 21, 2018 18:14

Add import for aws_cognito_user_group

5888299

tomelliff force-pushed the cognito-user-group branch from e905bb6 to 5888299 Compare January 22, 2018 08:26

radeksimko reviewed Feb 4, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 4, 2018

Actioned PR feedback

adb863c

ghost added the size/XL Managed by automation to categorize the size of a PR. label Feb 6, 2018

Merge remote-tracking branch 'origin/master' into cognito-user-group

c76d8ab

Conflicts: aws/validators_test.go

radeksimko approved these changes Feb 6, 2018

View reviewed changes

radeksimko merged commit 3b1e883 into hashicorp:master Feb 6, 2018

tomelliff deleted the cognito-user-group branch February 6, 2018 11:26

bflad added this to the v1.9.0 milestone Feb 9, 2018

ghost locked and limited conversation to collaborators Apr 7, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_cognito_user_group #3010

New Resource: aws_cognito_user_group #3010

tomelliff commented Jan 16, 2018 •

edited

Loading

tomelliff commented Jan 16, 2018

GreenSmile commented Jan 19, 2018

tomelliff commented Jan 21, 2018

tomelliff commented Jan 21, 2018 •

edited

Loading

radeksimko left a comment

radeksimko Feb 4, 2018

radeksimko Feb 4, 2018

radeksimko Feb 4, 2018

radeksimko Feb 4, 2018

tomelliff commented Feb 6, 2018

radeksimko left a comment

bflad commented Feb 9, 2018

jayanderson commented Feb 16, 2018

radeksimko commented Feb 16, 2018

ghost commented Apr 7, 2020

New Resource: aws_cognito_user_group #3010

New Resource: aws_cognito_user_group #3010

Conversation

tomelliff commented Jan 16, 2018 • edited Loading

tomelliff commented Jan 16, 2018

GreenSmile commented Jan 19, 2018

tomelliff commented Jan 21, 2018

tomelliff commented Jan 21, 2018 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Feb 4, 2018

Choose a reason for hiding this comment

radeksimko Feb 4, 2018

Choose a reason for hiding this comment

radeksimko Feb 4, 2018

Choose a reason for hiding this comment

radeksimko Feb 4, 2018

Choose a reason for hiding this comment

tomelliff commented Feb 6, 2018

radeksimko left a comment

Choose a reason for hiding this comment

bflad commented Feb 9, 2018

jayanderson commented Feb 16, 2018

radeksimko commented Feb 16, 2018

ghost commented Apr 7, 2020

tomelliff commented Jan 16, 2018 •

edited

Loading

tomelliff commented Jan 21, 2018 •

edited

Loading