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

Fix Resource#wait_until(max_attempts: nil) #855

Merged
merged 1 commit into from
Jun 26, 2015

Conversation

ktheory
Copy link
Contributor

@ktheory ktheory commented Jun 26, 2015

Happy Friday! 👋

I noticed that passing max_attempts: nil to a resource didn't work like these docs said. It only tried 10 times before raising Aws::Waiters::Errors::TooManyAttemptsError. 🐛 🚨

So I changed the code match the docs. 🎉 😌

Is there even a test???

You betcha. Of course it's weird to test that we actually attempt something indefinitely. So I just test that wait_until(max_attempts: nil) attempts more than 10 times.

Could there be another way???

Instead of changing the code, we could change the docs to recommend passing max_attempts: -1 or something instead. That can be a workaround until this is released. IMHO, using nil instead of -1 to indicate "unlimited" seems more conventional for Rubyists.

It now retries indefinitely, instead of defaulting to 10 attempts.
@ktheory ktheory force-pushed the fix-resource-unlimited-attempts branch from a5c961e to 7fae8fa Compare June 26, 2015 19:04
@awood45
Copy link
Member

awood45 commented Jun 26, 2015

Looks good to me, nice catch! 👍

I would be inclined to agree that this is the best way rather than special casing -1, but I'm open to changing my mind.

awood45 added a commit that referenced this pull request Jun 26, 2015
Fix Resource#wait_until(max_attempts: nil)
@awood45 awood45 merged commit 8a7224f into aws:master Jun 26, 2015
awood45 added a commit that referenced this pull request Jun 26, 2015
Adds release notes for pull request #855.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants