New resource: aws_cognito_user_pool_app_client

[tomelliff]

Minimal Cognito User Pool App/Client resource.
Only implements generate_secret, refresh_token_validity and read/write_attributes so far.
All the Oauth stuff has been left for work at a later date.

[tomelliff]

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSCognitoUserPoolAppClient"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserPoolAppClient -timeout 120m
=== RUN   TestAccAWSCognitoUserPoolAppClient_basic
--- PASS: TestAccAWSCognitoUserPoolAppClient_basic (28.67s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_generate_secret
--- PASS: TestAccAWSCognitoUserPoolAppClient_generate_secret (28.65s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_complex
--- PASS: TestAccAWSCognitoUserPoolAppClient_complex (52.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	109.667s

[tomelliff]

Interestingly I managed to accidentally return resourceAwsCognitoUserPoolRead from the update instead of resourceAwsCognitoUserPoolAppClientRead but all my tests still passed. When I checked, I had a Cognito user pool still left in us-west-2 from the test that hadn't been destroyed despite all the tests having a CheckDestroy that looks like it should be fine.

I've since fixed the mistake and squashed the fix but have I done something wrong there?

[tomelliff]

Moving this back to WIP because after a little bit of testing it looks like updating either read or write attributes but not both then wipes the other one. I'll try and debug what's going on there but it's not good to be merged until then.

[tomelliff]

Think that's fixed everything and have tests to cover that last issue too:

$ make testacc TEST=./aws TESTARGS="-run=TestAccAWSCognitoUserPoolAppClient"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoUserPoolAppClient -timeout 120m
=== RUN   TestAccAWSCognitoUserPoolAppClient_basic
--- PASS: TestAccAWSCognitoUserPoolAppClient_basic (28.99s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_update_basic
--- PASS: TestAccAWSCognitoUserPoolAppClient_update_basic (28.35s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_generate_secret
--- PASS: TestAccAWSCognitoUserPoolAppClient_generate_secret (28.47s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_complex
--- PASS: TestAccAWSCognitoUserPoolAppClient_complex (53.17s)
=== RUN   TestAccAWSCognitoUserPoolAppClient_update_non_attributes
--- PASS: TestAccAWSCognitoUserPoolAppClient_update_non_attributes (48.38s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	187.378s

[jen20]

Hi @tomelliff! Thanks for the comprehensive pull request. I have another couple to review ahead of this one but if no-one else picks this up first I will do so shortly.

[tomelliff]

Cool, thanks. The only time I get to write any Go is when I contribute to Terraform so let me know if there's anything I've done wrong or could do better.

[radeksimko]

Hi @tomelliff
this looks like a duplicate of a resource which I just merged: #1803 (admittedly raised earlier) c583147

Can you please take a look and confirm that we can close this PR?

Thanks.

[tomelliff]

Ack, I'm not doing too well with Github search apparently and keep missing things that other people have done before starting.

Looking at the discussion in the PR I can see this comment that seems to suggest that there's no support for custom attributes but I can't see why not because it was straightforward for me to add them (and I have tests for them too).

I also noticed that the read_attributes and write_attributes are only set on an update when they are changed (c583147#diff-691a9416b6661d1ee9e2ed38b6dce6fdR269) but this won't actually work because if you set just the read attributes and not the write attributes the API annoyingly blanks the write attributes and vice versa. This has the awful fallback of making everything readable/writable as well :( I added a test in my PR to cover that use case but I could probably rework fixes on top of that work and close that PR if you want? I'd probably want to test the update a bit more heavily with a test case that loops through a lot of changes just from the weirdness going on with the attributes.

[radeksimko]

I could probably rework fixes on top of that work and close that PR if you want?

That sounds like a good plan 👍 Thanks.

[radeksimko]

I'm going to close this PR then and let you pick any changes & PR them separately.

Thanks for all the efforts!

[ghost]

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!