Allow hardware.place_order with size ID (and name)
#861
Conversation
|
The CI check should be fixed in the latest master commit. Also, are there any presets that can't be ordered here with just their size name? |
|
@allmightyspiff I do not see anything here that would cause problems with ordering a preset. I'm not thrilled with allowing an id to be used instead of the size (keyName) since we tend to encourage keyNames over ids. I guess it's not that much of a risk though since a preset id isn't as likely to change as something like a price id. |
|
Hello,@kwienken |
|
@ZuluPro in that case I'm fine with using the ID instead of keyName. |
There was a problem hiding this comment.
@ZuluPro looks good for the most part, if you could do those small changes I requested I'll merge this.
Thanks
SoftLayer/managers/hardware.py
Outdated
| 'domain': domain, | ||
| } | ||
|
|
||
| if not isinstance(size, int): |
There was a problem hiding this comment.
I think I would rather this check be in _get_preset_id
SoftLayer/managers/hardware.py
Outdated
| See get_create_options() for valid arguments. | ||
| :param string size: server size name | ||
| :param string size: server size name or ID |
ZuluPro
force-pushed
the
patch-1
branch
2 times, most recently
from
04fc40f to
eae491d
Compare
| except SoftLayer.SoftLayerError as err: | ||
| if not err.message.startswith('Could not find valid size for'): | ||
| raise | ||
| size = size |
There was a problem hiding this comment.
As you asked I check if a size exists with its name before take ID.
I check with try/except, and raise other exceptions.
There was a problem hiding this comment.
I meant, modify _get_preset_id to return the valid id based on either a keyName or preset id
def _get_preset_id(package, size):
"""Get the preset id given the keyName of the preset."""
for preset in package['activePresets']:
if preset['keyName'] == size:
return preset['id']
elif preset['id'] == size:
return preset['id']
raise SoftLayer.SoftLayerError("Could not find valid size for: '%s'" % size)Also, the build scripts need to pass before I will merge a change.
You might need to pull in my new changes in the master branch as I've included some unit test fixes since this request was made.
|
@allmightyspiff @kwienken |
|
Any news ? |
|
I'm going to address this in #912 |
4 participants
No description provided.