-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add support for aws_cloudwatch_metric_stream #18870
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome @gcacace 👋
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 CONTRIBUTING 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! 😃
@gcacace After a quick scan, this PR is looking good. Overall, the neatness and completeness, with implementation, docs, tests, is great! Thank you for submitting it! I may point out a few minor items. Also, I encourage you to check the "allow edits by maintainers" box to facilitate getting this merged more quickly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your time on this!
Co-authored-by: Dirk Avery <31492422+YakDriver@users.noreply.github.com>
Hi @YakDriver, wondering if I've missed anything here apart from your comments that I've addressed in the latest revision. |
Hi Folks, is there any eta on when this will be merged, released, and supported by TF? This is a great add we are ourselves waiting on. |
@YakDriver it has been almost 20 days since I've replied and fixed all the review comments. Is there any chance we can get this merged? |
… honeycomb Things we need: 1. hashicorp/terraform-provider-aws#18870 to be merged (ismith is subscribed to this PR) 2. To test the module - sandbox account should be fine, we don't need anything complex here that'd require putting it in dogfood or terraform cloud - This includes testing the include_filters/exclude_filters variables 3. Documentarion in README.md 4. Documentation strings in variables.tf 5. version.tf needs a version for hashicorp/aws, and we should consider loosening the terraform.required_version constraint 6. Decide/document a release process (tl;dr: semver tags in github) **Asana:** https://app.asana.com/0/1199917178609623/1200319907426032
@gcacace I was unable to push to your branch to fix remaining issues. I'm not sure if you "allow edits by maintainers" or if it fails because your branch name is "main." It is best to name branches following the convention of
|
This will be superceded by #19429 |
…am resource See: hashicorp/terraform-provider-aws#19429 (which superseded hashicorp/terraform-provider-aws#18870). Which means we can remove all the clunky run-your-own-forked-provider junk. Yay!
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. |
Community Note
Closes #18525
Output from acceptance testing: