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

Loop through paginated response to check for cluster id #29732

Merged
merged 16 commits into from
May 1, 2023

Conversation

nsAstro
Copy link
Contributor

@nsAstro nsAstro commented Feb 23, 2023

Loops through the paginated emr list_clusters endpoint and searches each set for the given cluster_id.

closes: #29712

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Feb 23, 2023
@Taragolis
Copy link
Contributor

I took a stab at this but I am at a loss as to how to test without spinning up 51 EMR clusters myself. If anyone has a suggestion for how to whitebox, mock, or test this, happy to contribute.

You could mock this method and use side_effect

@nsAstro nsAstro marked this pull request as ready for review March 3, 2023 18:23
@nsAstro nsAstro requested review from thirtyseven and removed request for eladkal and o-nikolas March 3, 2023 21:27
@thirtyseven
Copy link

@nsAstro I am not a codeowner, you'll need one of @eladkal @o-nikolas to review.

@nsAstro nsAstro requested a review from o-nikolas March 7, 2023 21:35
@o-nikolas
Copy link
Contributor

Hey @nsAstro! Any status update on this one?

Seems like just a few small outstanding feedbacks.

@o-nikolas
Copy link
Contributor

It doesn't look like @nsAstro is enganged on this one any more. I'll push the remaining fixes if I get some time in the next few days.

o-nikolas and others added 2 commits April 27, 2023 13:49
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
- Separate tests into individual methods
- Ensure pagination test checks for cluster id on second page
@o-nikolas
Copy link
Contributor

I updated the PR to address the remaining feedback

@potiuk
Copy link
Member

potiuk commented Apr 29, 2023

unexpected ident does not signal anything good.

@o-nikolas
Copy link
Contributor

unexpected ident does not signal anything good.

Ooop! I updated @nsAstro's PR through the Github UI text editor (which has no bells and whistles for linting or catching those indents) so I missed that one! Hopefully that stops the build from timing out 😁

@o-nikolas
Copy link
Contributor

Looks like this is all passing and we have a couple of approvals. I'm not sure if it's proper for me to approve and merge this since the PR is mostly my code at this point, so CCing @potiuk and @eladkal to approve and merge (and/or advise that it's okay if I do).

@eladkal eladkal merged commit 9662fd8 into apache:main May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EMRHook.get_cluster_id_by_name() doesn't use pagination
8 participants