Remove redundant validation from duplicate ordering (covered by API) #894
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
** Also add deepcopy for test fixtures to clean up some unit tests
|
Coverage decreased, but I think this is because there is less code to test now, since coverage for all three related Manager files is still at 100%. I'm not sure how to fix this one without adding unit tests elsewhere to unrelated code. I'll try to figure out what to do to address this. |
allmightyspiff
requested changes
Nov 22, 2017
SoftLayer/managers/storage_utils.py
Outdated
| iops = _validate_dupl_performance_iops( | ||
| origin_volume, iops, duplicate_size) | ||
| if iops is None: | ||
| if isinstance(utils.lookup(origin_volume, 'provisionedIops'), str): |
There was a problem hiding this comment.
I think this check is a little unneeded. What if the API changes provisionedIops to be an int in the future?
I think just doing iops = int(origin_volume.get('provisionedIops', 0)) should be sufficient. Then you can make sure that value is > 0 and then throw your exception.
There was a problem hiding this comment.
I can do that instead, no problem.
allmightyspiff
approved these changes
Nov 29, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the upcoming introduction of volume modification, some of the existing size-validation logic for ordering duplicate volumes is no longer valid in all cases. In this branch, I have removed most of the size and performance validation logic for ordering duplicate volumes, since this validation is done within the API anyway. While increased reliance on the API does limit how specific we can be with the SLCLI error messages, it does seem like reducing redundant validation logic in the SLCLI will help increase the SLCLI's maintainability overall. Please let me know if I have removed too much, and I will be happy to add logic back in if needed.
This branch also cleans up some unit tests by using deepcopy for copying test fixtures. This will prevent an undesirable "cascading test failure" effect that came with my previous implementation of some unit tests.