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

Expose waiter configuration #1267

Merged
merged 2 commits into from Aug 28, 2017
Merged

Expose waiter configuration #1267

merged 2 commits into from Aug 28, 2017

Conversation

joguSD
Copy link
Contributor

@joguSD joguSD commented Aug 17, 2017

Adding so we can properly address aws/aws-cli#2761

This will allow WaiterConfig to be passed to waiter.wait() calls allowing customers to adjust waiter's maxAttempts and delay settings. This is based on how PaginationConfig works. The current keys are formatted to match the waiter models.

waiter.wait(WaiterConfig={
    'delay': 5,
    'maxAttempts': 5,
})

@codecov-io
Copy link

codecov-io commented Aug 17, 2017

Codecov Report

Merging #1267 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1267      +/-   ##
===========================================
+ Coverage    98.04%   98.05%   +<.01%     
===========================================
  Files           45       45              
  Lines         7382     7389       +7     
===========================================
+ Hits          7238     7245       +7     
  Misses         144      144
Impacted Files Coverage Δ
botocore/docs/waiter.py 100% <100%> (ø) ⬆️
botocore/waiter.py 98.67% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2409af...ed8f8c4. Read the comment docs.

Copy link
Contributor

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me for the most part. We'll need docs though.

# pop the invocation specific config
config = kwargs.pop('WaiterConfig', {})
sleep_amount = config.get('delay', self.config.delay)
max_attempts = config.get('maxAttempts', self.config.max_attempts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/maxAttempts/max_attempts/

Copy link
Contributor Author

@joguSD joguSD Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to be consistent with the PaginationConfig formatting. The keys are capitalized words there. The waiter config uses mixed case keys. No matter what the key is called it will break consistency in some way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little picky, but the casing should be pascal casing where the first letter should be capitalized. That is how pagination does it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with MaxAttempts and Delay.

@kyleknap
Copy link
Member

Approach looks fine. But I agree with Jordon. We need docs for this.

@JordonPhillips
Copy link
Contributor

It's also worth considering exposing this in the CLI

@joguSD
Copy link
Contributor Author

joguSD commented Aug 23, 2017

Updated to pascal casing and added the docs in a similar manner to pagination.

@JordonPhillips JordonPhillips added the pr/needs-review This PR needs a review from a member of the team. label Aug 23, 2017
Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just had a small comment on the docs. I also think we should add tests for the docs.

waiter_config_members['Delay'] = DocumentedShape(
name='Delay', type_name='integer',
documentation=(
'<p>The amount of time in seconds to wait between attempts.</p>'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is in the top level description, but it might be good to say what the defaults are for Delay and MaxAttempts for these operations as well.

@kyleknap
Copy link
Member

Also make sure you add a changelog entity for this.

@joguSD
Copy link
Contributor Author

joguSD commented Aug 25, 2017

Added defaults inline.
screen shot 2017-08-25 at 1 57 04 pm

Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Looks good to me. 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a member of the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants