File and Block Hourly billing feature changes for slci #852
Conversation
|
Yeah sure I will take care of conflicts and shall update pull request. |
dpickle2
changed the title
There was a problem hiding this comment.
From the perspective of the SLCLI, these changes look good to me, so long as the info being sent to the API produces the expected ordering results on the backend.
I have added a few minor comments to this review, but I don't see any major changes that are needed.
| type=click.Choice(['hourly', 'monthly']), | ||
| default='monthly', | ||
| show_default=True, | ||
| help="Optional parameter for Billing rate. Default to monthly") |
There was a problem hiding this comment.
This is a minor detail, but with 'show_default' set to True, there is no need to also say "Default to monthly" in the help message, since setting 'show_default' to True will already add this information to the output.
There was a problem hiding this comment.
show_default=True - removed as per discussion with David. will push latest file in a while
| type=click.Choice(['hourly', 'monthly']), | ||
| default='monthly', | ||
| show_default=True, | ||
| help="Optional parameter for Billing rate. Default to monthly") |
There was a problem hiding this comment.
This is a minor detail, but with 'show_default' set to True, there is no need to also say "Default to monthly" in the help message, since setting 'show_default' to True will already add this information to the output.
There was a problem hiding this comment.
show_default=True - removed as per discussion with David. will push latest file in a while.
tests/managers/block_tests.py
Outdated
| }, | ||
| } | ||
|
|
||
| self.block.cancel_snapshot_space(1234, immediate=True) |
There was a problem hiding this comment.
I apologize for missing this when I was adding these unit tests, but the value of 'immediate' here should be False. Would it be possible to update this line?
There was a problem hiding this comment.
Its correct, user can cancel hourly billing instance on immediate basis. its part of requirement.
There was a problem hiding this comment.
Based on the current logic in the Block (and File) managers, immediate will always be used if the volume has hourly billing. Passing in False in the unit test is meant to ensure that the immediate flag is correctly changed to be True when the API call is actually made.
tests/managers/file_tests.py
Outdated
| }, | ||
| } | ||
|
|
||
| self.file.cancel_snapshot_space(1234, immediate=True) |
There was a problem hiding this comment.
I apologize for missing this when I was adding these unit tests, but the value of 'immediate' here should be False. Would it be possible to update this line?
There was a problem hiding this comment.
For hourly billing we can cancel snapshot immediate, its part of requirement. So no need to change anything here.
…d for volume duplicate
…ication name changed.
…ion for duplicate volume.
|
Unstable build .. hence closing. will create new pull request. |
3 participants
Please merge after Davids pull request to see actual changes ( #838 )