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

resourcegroupstaggingapi_resources - new data source #17804

Merged
merged 17 commits into from
Apr 29, 2021
Merged

resourcegroupstaggingapi_resources - new data source #17804

merged 17 commits into from
Apr 29, 2021

Conversation

DrFaust92
Copy link
Collaborator

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #17705

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccDataSourceAwsResourceGroupsTaggingAPIResources_'
--- PASS: TestAccDataSourceAwsResourceGroupsTaggingAPIResources_basic (26.83s)
--- PASS: TestAccDataSourceAwsResourceGroupsTaggingAPIResources_compliance (27.22s)
--- PASS: TestAccDataSourceAwsResourceGroupsTaggingAPIResources_resource_type_filters (33.55s)
--- PASS: TestAccDataSourceAwsResourceGroupsTaggingAPIResources_tag_key_filter (66.65s)
--- PASS: TestAccDataSourceAwsResourceGroupsTaggingAPIResources_resource_arn_list (98.83s)

@DrFaust92 DrFaust92 requested a review from a team as a code owner February 24, 2021 22:37
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/resourcegroupstaggingapi Issues and PRs that pertain to the resourcegroupstaggingapi service. labels Feb 24, 2021
@DrFaust92 DrFaust92 changed the title D/resource tagging groups resourcegroupstaggingapi_resources - new data source Feb 24, 2021
@ewbankkit ewbankkit mentioned this pull request Feb 25, 2021
Merged
@ewbankkit
Copy link
Contributor

ewbankkit commented Feb 25, 2021 •
edited
Loading

Add the resourcegroupstaggingapi service to infrastructure/repository/labels-service.tf.

@DrFaust92
Copy link
Collaborator Author

Addressed the comments, thanks!

@DrFaust92 DrFaust92 added the new-data-source Introduces a new data source. label Mar 27, 2021
@bflad bflad self-assigned this Apr 29, 2021
bflad
bflad reviewed Apr 29, 2021
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.

Overall looking really good, @DrFaust92 👍 Just going to switch from filter to tag_filter to match the API naming and re-verify everything.

aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated Show resolved Hide resolved
aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated Show resolved Hide resolved
aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated Show resolved Hide resolved
aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated Show resolved Hide resolved
aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated Show resolved Hide resolved
website/docs/d/resourcegroupstaggingapi_resources.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resourcegroupstaggingapi_resources.html.markdown Outdated Show resolved Hide resolved
website/docs/d/resourcegroupstaggingapi_resources.html.markdown Outdated Show resolved Hide resolved
aws/data_source_aws_resourcegroupstaggingapi_resources_test.go Outdated Show resolved Hide resolved
bflad
bflad reviewed Apr 29, 2021
View reviewed changes
.changelog/17804.txt Outdated Show resolved Hide resolved
website/docs/d/resourcegroupstaggingapi_resources.html.markdown Outdated Show resolved Hide resolved
bflad
bflad reviewed Apr 29, 2021
View reviewed changes
aws/data_source_aws_resourcegroupstaggingapi_resources.go Outdated
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
ConflictsWith: []string{"filter"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ConflictsWith: []string{"filter"},
ConflictsWith: []string{"tag_filter"},

bflad
bflad reviewed Apr 29, 2021
View reviewed changes
aws/data_source_aws_resourcegroupstaggingapi_resources_test.go
}

const testAccDataSourceAwsResourceGroupsTaggingAPIResourcesBasicConfig = `
data "aws_resourcegroupstaggingapi_resources" "test" {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty configuration is taking 30+ minutes to paginate in our testing accounts 😢 In this case, I think it is probably okay to omit the testing since the API doesn't provide any sort of "maximum results" parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i was afraid of this. i'm ok with omitting it. maybe hide it behind a flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a similar issue with aws_ami where unfiltered results would result in a huge result set as well. We skip including that testing with the hope that the unfiltered API operation would operate the same as a filtered one. Not aware of any particular issues with that in the last 3 years. We could put this behind an environment variable, but it is probably okay without it.

aws/data_source_aws_resourcegroupstaggingapi_resources_test.go

func testAccDataSourceAwsResourceGroupsTaggingAPIResourcesTagKeyFilterConfig(rName string) string {
return fmt.Sprintf(`
resource "aws_api_gateway_rest_api" "test" {
Copy link
Contributor

Choose a reason for hiding this comment

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

API Gateway REST APIs can be slightly problematic:

  • Throttled to 2 deletions/minute
  • EDGE endpoints (default) are unavailable in GovCloud

Going to switch this to an easier resource such as a VPC to see if that cooperates a little better. 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i tinkered a lot with this. was the simplest one i could think of at the time. i remember trying vpc but dont remember why i dropped it. IIRC it didn't propagate and results were empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be having really good luck today as it seems to be passing with aws_vpc resources and random tags. 😄

@bflad bflad merged commit d22d66d into hashicorp:main Apr 29, 2021
@github-actions github-actions bot added this to the v3.38.0 milestone Apr 29, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

This was referenced May 1, 2021
Closed
Merged
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2021
@DrFaust92 DrFaust92 deleted the d/resource_tagging_groups branch May 31, 2021 05:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ewbankkit ewbankkit ewbankkit left review comments

@bflad bflad bflad left review comments

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. new-data-source Introduces a new data source. provider Pertains to the provider itself, rather than any interaction with AWS. service/resourcegroupstaggingapi Issues and PRs that pertain to the resourcegroupstaggingapi service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v3.38.0
Development

Successfully merging this pull request may close these issues.

Generic Fetching Resources via Tags
3 participants
@DrFaust92 @ewbankkit @bflad