-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
New Resource: aws_emr_instance_fleet #9263
Conversation
@bflad any chance this can be reviewed and accepted? This changeset has been around in forks for more than two years now since @jmsantorum's initial PR. |
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.
Hi folks 👋 Sorry for the long delay in the review process for this functionality. While this enhancement is not a current focus for the maintainers at the moment, I did want to provide some initial feedback so we can move forward quicker when we are ready to review this functionality.
The maintainers would really prefer if this pull request was split into two separate enhancements for review:
- Adding instance fleet support to the existing
aws_emr_cluster
resource - Adding the new
aws_emr_instance_fleet
resource (based on and submitted after the above)
The reasoning mainly focusing around that changes in the first implementation may influence the second implementation. This should result in quicker feedback cycles, rather than the author(s) needing to worry about updating multiple locations with repetitive changes and similarly reviewer(s) needing to worry about checking multiple locations with repetitive changes.
As for the current aws_emr_cluster
resource implementation, we would really like to see instance_fleet
split up similar to the newer master_instance_group
and core_instance_group
handling over the original (and now deprecated) instance_group
configuration blocks. There were a large number of bugs associated with the older handling as well as certain feature requests that were not easily possible with the old monolithic style. We would prefer to not run into those issues again.
Apologies again for this lengthy review process and thanks for contributing this. Please reach out with any questions.
@bflad how is this tracking? Can I throw some Dev resource behind it to assist? |
Hi again folks 👋 Since there does not seem to be any forward movement or replies on the requested changes above, we are going to opt to close this pull request for now. If anyone is motivated to continue this, following the recommendations outlined in that comment, please open a new pull request and the maintainers will take another look. 👍 |
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! |
Community Note
Fixes #1146 Extend EMR ec2_attributtes with subnet_ids, availability_zone, availability_zones. Added hadoop_version parameter.
Release note for CHANGELOG:
Output from acceptance testing:
Need some help in Acceptance testing. I'm not sure it the acceptance tests are done in the best way.
It's my first PR on this project and any feedback is welcome