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.

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

F aws acm data source tags filter #31453

Merged
merged 41 commits into from
Sep 6, 2024

Conversation

rromic
Copy link
Contributor

@rromic rromic commented May 17, 2023

Description

This PR adds option to filter acm certificates by tags.
Now domain attribute is not required any more but either domain or tags or both needs to be provided to get certificates.

Added acceptance tests to cover new cases and adjusted previous ones to ensure previous logic remains the same.

Relations

Closes #1918

References

Output from Acceptance Testing

export AWS_DEFAULT_REGION=eu-central-1
export ACM_CERTIFICATE_SINGLE_ISSUED_DOMAIN=test.single.domain
export ACM_CERTIFICATE_MULTIPLE_ISSUED_DOMAIN=test.multi.domain
export ACM_CERTIFICATE_ROOT_DOMAIN=domain
export ACM_CERTIFICATE_SINGLE_ISSUED_TAG_NAME=Name
export ACM_CERTIFICATE_SINGLE_ISSUED_TAG_VALUE=test.single.domain
export ACM_CERTIFICATE_MULTIPLE_ISSUED_TAG_NAME=Name
export ACM_CERTIFICATE_MULTIPLE_ISSUED_TAG_VALUE=test.multi.domain
export ACM_CERTIFICATE_TYPE=IMPORTED
make testacc TESTS=TestAccACMCertificateDataSource PKG=acm  
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/acm/... -v -count 1 -parallel 20 -run='TestAccACMCertificateDataSource'  -timeout 180m
=== RUN   TestAccACMCertificateDataSource_singleIssued
=== PAUSE TestAccACMCertificateDataSource_singleIssued
=== RUN   TestAccACMCertificateDataSource_multipleIssued
=== PAUSE TestAccACMCertificateDataSource_multipleIssued
=== RUN   TestAccACMCertificateDataSource_noMatchReturnsError
=== PAUSE TestAccACMCertificateDataSource_noMatchReturnsError
=== RUN   TestAccACMCertificateDataSource_keyTypes
=== PAUSE TestAccACMCertificateDataSource_keyTypes
=== CONT  TestAccACMCertificateDataSource_singleIssued
=== CONT  TestAccACMCertificateDataSource_noMatchReturnsError
=== CONT  TestAccACMCertificateDataSource_multipleIssued
=== CONT  TestAccACMCertificateDataSource_keyTypes
--- PASS: TestAccACMCertificateDataSource_noMatchReturnsError (27.84s)
--- PASS: TestAccACMCertificateDataSource_keyTypes (30.41s)
--- PASS: TestAccACMCertificateDataSource_multipleIssued (119.36s)
--- PASS: TestAccACMCertificateDataSource_singleIssued (199.69s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/acm        203.077s

...

@github-actions
Copy link

Community Note

Voting for Prioritization

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

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. service/acm Issues and PRs that pertain to the acm service. size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels May 17, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @rromic 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTOR guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@rromic rromic force-pushed the f-aws_acm_data_source-tags-filter branch from decbf39 to 6148bae Compare May 17, 2023 19:34
@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels May 17, 2023
@rromic
Copy link
Contributor Author

rromic commented Aug 1, 2023

@ewbankkit any plans for this PR? :)

@ewbankkit
Copy link
Contributor

@rromic Yes, I'll take a look and migrate to transparent tagging. Thanks.

@vishwa-trulioo
Copy link

Can we please get this merged?

@rromic rromic force-pushed the f-aws_acm_data_source-tags-filter branch from a0794c4 to 1e8c2fc Compare October 1, 2023 11:41
@rromic
Copy link
Contributor Author

rromic commented Oct 1, 2023

@rromic Yes, I'll take a look and migrate to transparent tagging. Thanks.

@ewbankkit can I help with transparent tagging? Can you please provide some example so I can try to put it in to get this PR to be merged faster?

@vishwa-trulioo
Copy link

Any update? Thanks.

@rromic
Copy link
Contributor Author

rromic commented Oct 7, 2023

Any update? Thanks.

@vishwa-trulioo nothing from my side. I have resolved all conflicts and made linter fixes that got in with main branch.

@vishwa-trulioo
Copy link

@ewbankkit if the work is good, are you able to get this into the next release? Thank you in advance.

…cACMCertificateDataSource_byTagsAndKeyTypes'.
@ewbankkit ewbankkit requested a review from a team as a code owner September 6, 2024 16:06
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccACMCertificateDataSource_' PKG=acm ACCTEST_PARALLELISM=3
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.0 test ./internal/service/acm/... -v -count 1 -parallel 3  -run=TestAccACMCertificateDataSource_ -timeout 360m
=== RUN   TestAccACMCertificateDataSource_tags
=== PAUSE TestAccACMCertificateDataSource_tags
=== RUN   TestAccACMCertificateDataSource_byDomain
=== PAUSE TestAccACMCertificateDataSource_byDomain
=== RUN   TestAccACMCertificateDataSource_byDomainNoMatch
=== PAUSE TestAccACMCertificateDataSource_byDomainNoMatch
=== RUN   TestAccACMCertificateDataSource_byDomainMultiple
=== PAUSE TestAccACMCertificateDataSource_byDomainMultiple
=== RUN   TestAccACMCertificateDataSource_byDomainAndTags
=== PAUSE TestAccACMCertificateDataSource_byDomainAndTags
=== RUN   TestAccACMCertificateDataSource_byDomainAndStatuses
=== PAUSE TestAccACMCertificateDataSource_byDomainAndStatuses
=== RUN   TestAccACMCertificateDataSource_byDomainAndKeyTypes
=== PAUSE TestAccACMCertificateDataSource_byDomainAndKeyTypes
=== RUN   TestAccACMCertificateDataSource_byDomainAndTypes
=== PAUSE TestAccACMCertificateDataSource_byDomainAndTypes
=== RUN   TestAccACMCertificateDataSource_byDomainAndTypesNoMatch
=== PAUSE TestAccACMCertificateDataSource_byDomainAndTypesNoMatch
=== RUN   TestAccACMCertificateDataSource_byDomainAndKeyTypesMostRecent
=== PAUSE TestAccACMCertificateDataSource_byDomainAndKeyTypesMostRecent
=== RUN   TestAccACMCertificateDataSource_byTags
=== PAUSE TestAccACMCertificateDataSource_byTags
=== RUN   TestAccACMCertificateDataSource_byTagsNoMatch
=== PAUSE TestAccACMCertificateDataSource_byTagsNoMatch
=== RUN   TestAccACMCertificateDataSource_byTagsAndKeyTypes
=== PAUSE TestAccACMCertificateDataSource_byTagsAndKeyTypes
=== CONT  TestAccACMCertificateDataSource_tags
=== CONT  TestAccACMCertificateDataSource_byDomainAndTypes
=== CONT  TestAccACMCertificateDataSource_byDomainAndTags
--- PASS: TestAccACMCertificateDataSource_byDomainAndTags (14.45s)
=== CONT  TestAccACMCertificateDataSource_byDomainNoMatch
--- PASS: TestAccACMCertificateDataSource_tags (14.98s)
=== CONT  TestAccACMCertificateDataSource_byDomainMultiple
--- PASS: TestAccACMCertificateDataSource_byDomainAndTypes (19.90s)
=== CONT  TestAccACMCertificateDataSource_byDomain
--- PASS: TestAccACMCertificateDataSource_byDomainMultiple (9.12s)
=== CONT  TestAccACMCertificateDataSource_byDomainAndKeyTypes
--- PASS: TestAccACMCertificateDataSource_byDomain (13.26s)
=== CONT  TestAccACMCertificateDataSource_byTags
--- PASS: TestAccACMCertificateDataSource_byDomainAndKeyTypes (13.81s)
=== CONT  TestAccACMCertificateDataSource_byTagsAndKeyTypes
--- PASS: TestAccACMCertificateDataSource_byTags (14.64s)
=== CONT  TestAccACMCertificateDataSource_byTagsNoMatch
--- PASS: TestAccACMCertificateDataSource_byTagsNoMatch (7.56s)
=== CONT  TestAccACMCertificateDataSource_byDomainAndKeyTypesMostRecent
--- PASS: TestAccACMCertificateDataSource_byTagsAndKeyTypes (16.01s)
=== CONT  TestAccACMCertificateDataSource_byDomainAndTypesNoMatch
--- PASS: TestAccACMCertificateDataSource_byDomainNoMatch (66.26s)
=== CONT  TestAccACMCertificateDataSource_byDomainAndStatuses
--- PASS: TestAccACMCertificateDataSource_byDomainAndStatuses (14.64s)
--- PASS: TestAccACMCertificateDataSource_byDomainAndTypesNoMatch (66.33s)
--- PASS: TestAccACMCertificateDataSource_byDomainAndKeyTypesMostRecent (200.70s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/acm	263.625s

@ewbankkit
Copy link
Contributor

@rromic Thanks for the contribution 🎉 👏.

@ewbankkit ewbankkit merged commit 433cf45 into hashicorp:main Sep 6, 2024
43 checks passed
@rromic
Copy link
Contributor Author

rromic commented Sep 6, 2024

@rromic Thanks for the contribution 🎉 👏.

awesome! Thank you for all the support!

@github-actions github-actions bot added this to the v5.67.0 milestone Sep 6, 2024
@github-actions github-actions bot removed the prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. label Sep 13, 2024
Copy link

This functionality has been released in v5.67.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. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/acm Issues and PRs that pertain to the acm 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
Development

Successfully merging this pull request may close these issues.

Add tags as search criteria for aws_acm_certificate data source
4 participants