-
Notifications
You must be signed in to change notification settings - Fork 194
Update file/block ordering to support storage_as_a_service package #838
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
Update file/block ordering to support storage_as_a_service package #838
Conversation
**Includes updates to logic for ordering volumes, replicas, and snapshot space. The storage_as_a_service package is used as a default for new volume orders, but customers can specify older packages for use. The package associated with the existing primary volume is used in replica and snapshot orders. **Also includes a refactoring of a number of the unit tests for File and Block manager functions.
@@ -1,4 +1,4 @@ | |||
DUPLICATABLE_VOLUME = { | |||
SAAS_TEST_VOLUME = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be STAAS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, SAAS was meant to represent "Storage As A Service". This test volume object is intended for use in tests relating to the Storage As A Service package. If it would help clear up possible misunderstanding, I am happy to add a comment to the source to indicate that SAAS corresponds to the Storage As A Service offering/package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks pretty good.
:param snapshot_size: The size of optional snapshot space, | ||
if snapshot space should also be ordered (None if not ordered) | ||
if snapshot space should also be ordered (None if not ordered) | ||
:param service_offering: Requested offering package to use in the order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we enumerate the possible options for service_offering here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update this. Thanks!
'the order [optional, default is \'storage_as_a_service\'', | ||
type=click.Choice([ | ||
'storage_as_a_service', | ||
'enterprise', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be endurance instead of enterprise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose "enterprise" to correspond to the name of the SoftLayer_Container_Product_Order_Network_Storage_Enterprise order container (http://developer.softlayer.com/reference/datatypes/SoftLayer_Container_Product_Order_Network_Storage_Enterprise). Calling it "endurance" instead of "enterprise" might be more clear within the SLCLI, though. Should I go ahead and update this as well @allmightyspiff ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, since that corresponds with how the datatype is named "enterprise" is fine.
SoftLayer/CLI/block/order.py
Outdated
storage_type = storage_type.lower() | ||
|
||
if service_offering is None: | ||
service_offering = 'storage_as_a_service' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you able to use default='storage_as_a_service'
on click.option
here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cleaner. I will change this to use a default value.
SoftLayer/CLI/block/order.py
Outdated
'the size (in GB) of snapshot space to order') | ||
@click.option('--service-offering', | ||
help='The service offering package to use for placing ' | ||
'the order [optional, default is \'storage_as_a_service\'', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bracket isn't closed in this help text, as well as the others below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed that. Thank you for pointing that out!
**Includes updates to logic for ordering volumes, replicas,
and snapshot space. The storage_as_a_service package is used
as a default for new volume orders, but customers can specify
older packages for use. The package associated with the
existing primary volume is used in replica and snapshot orders.
**Also includes a refactoring of a number of the unit tests
for File and Block manager functions.
Issue #832 should be addressed by this pull request.