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

New Resource: aws_emr_instance_fleet #9263

Closed
wants to merge 2 commits into from

Conversation

sanoojm
Copy link

@sanoojm sanoojm commented Jul 8, 2019

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" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #1146 Extend EMR ec2_attributtes with subnet_ids, availability_zone, availability_zones. Added hadoop_version parameter.

Release note for CHANGELOG:

New Resource: aws_emr_instance_fleet

Output from acceptance testing:

Need some help in Acceptance testing. I'm not sure it the acceptance tests are done in the best way.

$ make testacc TESTARGS='-run=TestAccXXX'

...

It's my first PR on this project and any feedback is welcome

@sanoojm sanoojm requested a review from a team July 8, 2019 08:43
@ghost ghost added size/XXL 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. service/emr Issues and PRs that pertain to the emr service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 8, 2019
@aeschright aeschright added the new-resource Introduces a new resource. label Aug 19, 2019
@peay
Copy link

peay commented Sep 12, 2019

@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.

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.

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 bflad self-assigned this Oct 10, 2019
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 10, 2019
@sidbrydon
Copy link

@bflad how is this tracking? Can I throw some Dev resource behind it to assist?

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 28, 2019
@bflad
Copy link
Contributor

bflad commented Nov 15, 2019

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. 👍

@bflad bflad closed this Nov 15, 2019
@ghost
Copy link

ghost commented Dec 15, 2019

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 Dec 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/emr Issues and PRs that pertain to the emr service. size/XXL 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.

EMR create a new resource aws_emr_instances_fleet
5 participants