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.

Sign up for GitHub

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

Jump to bottom

New Data Source: cognito_user_pools #4212

Merged
merged 1 commit into from
Apr 17, 2018
Merged

New Data Source: cognito_user_pools #4212

merged 1 commit into from
Apr 17, 2018

Conversation

loivis
Copy link
Contributor

@loivis loivis commented Apr 15, 2018 •
edited
Loading

Complete missing part between cognito user pools and api gateway authorizer.

There is no option to filter cognito user pools, so a simple loop is used to match given name.

⎇  make fmt; and echo > aws/debug.log ; and make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsCognitoUserPoolIds_basic'
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsCognitoUserPoolIds_basic -timeout 120m
=== RUN   TestAccDataSourceAwsCognitoUserPoolIds_basic
--- PASS: TestAccDataSourceAwsCognitoUserPoolIds_basic (18.81s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	18.858s

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 15, 2018
@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 2ad5499 to 00ea2a5 Compare April 15, 2018 18:54
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 15, 2018
@bflad bflad added new-data-source Introduces a new data source. service/cognito labels Apr 17, 2018
bflad
bflad reviewed Apr 17, 2018
View reviewed changes
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.

Thanks for this contribution @loivis! Left some initial comments below -- please let me know if you have any questions.

aws/provider.go Outdated
@@ -173,6 +173,7 @@ func Provider() terraform.ResourceProvider {
"aws_cloudformation_stack": dataSourceAwsCloudFormationStack(),
"aws_cloudtrail_service_account": dataSourceAwsCloudTrailServiceAccount(),
"aws_cloudwatch_log_group": dataSourceAwsCloudwatchLogGroup(),
"aws_cognito_user_pool_ids": dataSourceAwsCognitoUserPoolIds(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's opt to call it aws_cognito_user_pools (we may add attributes other than just the ID) - will require refactoring the function names and documentation

website/aws.erb Outdated
@@ -72,6 +72,8 @@
</li>
<li<%= sidebar_current("docs-aws-datasource-cloudwatch-log-group") %>>
<a href="/docs/providers/aws/d/cloudwatch_log_group.html">aws_cloudwatch_log_group</a>
<li<%= sidebar_current("docs-aws-datasource-cognito-user-pool-ids") %>>
<a href="/docs/providers/aws/d/cognito_user_pool_ids.html">aws_cognito_user_pool_ids</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing </li>

aws/data_source_aws_cognito_user_pool_ids.go Outdated
Type: schema.TypeSet,
Computed: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
Copy link
Contributor

Choose a reason for hiding this comment

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

The Set: schema.HashString is unnecessary here

@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 00ea2a5 to 02383f1 Compare April 17, 2018 11:49
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis loivis changed the title New Data Source: cognito_user_pool_ids New Data Source: cognito_user_pools Apr 17, 2018
@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 02383f1 to db6efbb Compare April 17, 2018 11:57
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from db6efbb to 45bdb47 Compare April 17, 2018 12:00
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 45bdb47 to 606a7f2 Compare April 17, 2018 12:01
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 606a7f2 to 3d4e4fb Compare April 17, 2018 12:03
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis
Copy link
Contributor Author

loivis commented Apr 17, 2018

Opt to aws_cognito_user_pools.

Just noticed there are dangling resources after acceptance test. Tried to add sweeper but it didn't seem to work. Did I miss something?

@bflad
Copy link
Contributor

bflad commented Apr 17, 2018

The test sweeper should be part of the resource and not the data source. Let's address it separately.

@loivis loivis force-pushed the data-source-cognito-user-pool-ids branch from 3d4e4fb to 4448516 Compare April 17, 2018 14:48
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 17, 2018
@loivis
Copy link
Contributor Author

loivis commented Apr 17, 2018

All right. I've removed related code now.

bflad
bflad approved these changes Apr 17, 2018
View reviewed changes
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.

LGTM! 🚀

1 test passed (all tests)
=== RUN   TestAccDataSourceAwsCognitoUserPools_basic
--- PASS: TestAccDataSourceAwsCognitoUserPools_basic (8.17s)

@bflad bflad merged commit ef43a69 into hashicorp:master Apr 17, 2018
bflad added a commit that referenced this pull request Apr 17, 2018
@bflad
Update CHANGELOG for #4212
29a75f2
@bflad bflad added this to the v1.15.0 milestone Apr 18, 2018
@bflad
Copy link
Contributor

bflad commented Apr 18, 2018

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

@loivis loivis mentioned this pull request Apr 18, 2018
Merged
@loivis loivis deleted the data-source-cognito-user-pool-ids branch April 18, 2018 19:51
@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.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
new-data-source Introduces a new data source. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.15.0
Development

Successfully merging this pull request may close these issues.
2 participants
@loivis @bflad