-
Notifications
You must be signed in to change notification settings - Fork 194
Issues922 - DC short names in ordering #956
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
Conversation
"longName": "San Jose 1", | ||
"name": "sjc01" | ||
}] | ||
getDatacenters = [{'id': 1854895, 'name': 'dal13', 'regions': [{'keyname': 'DALLAS13'}]}] |
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.
Might be good to combine the fixtures since they are for the same method.
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 problem is that my method expects only 1 result back, but else where expects multiple results back.... I need to figure out a better way to do these fixtures to deal with different object masks/filters. but that might be for a different project.
# in which the order is made | ||
price_id = [p['id'] for p in matching_item['prices'] | ||
if p['locationGroupId'] == ''][0] | ||
if p['locationGroupId'] is None][0] |
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.
Tests are still relying on this being an empty string.
""" | ||
|
||
mask = "mask[id,name,regions[keyname]]" | ||
if match(r'[a-zA-Z]{3}[0-9]{2}', location) is not None: |
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 might be wrong, but there could still be a case where a user is passing in an id as the location, and previously this was just passed on to the order.
For backwards compatibility, I think this method may want to check for a digit-only string (isdigit
) and short circuit the lookup if that is the case.
SoftLayer/managers/ordering.py
Outdated
else: | ||
search = {'regions': {'keyname': {'operation': location}}} | ||
datacenter = self.client.call('SoftLayer_Location', 'getDatacenters', mask=mask, filter=search) | ||
# [{'id': 1854895, 'name': 'dal13', 'regions': [{'keyname': 'DALLAS13'}]}] |
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'm not a fan of putting example results like this because they can get out of date quickly. The corresponding fixture is a good way to discover the result structure that is actually used instead IMHO.
|
modifies managers.ordering.generate_order so that it uses a location ID instead of a long name. Can be either short name (dal13) or keyname (DALLAS13).
Also, default location price id seems to be None now instead of ''
Testing to make sure everything still works.
Debugging for placing order
Actual API calls