Skip to content

Conversation

@kwienken
Copy link
Contributor

This addresses the issue I opened, #809.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 82.572% when pulling 6ef39db on kwienken:issue-809 into ee66567 on softlayer:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 82.572% when pulling c7e3d84 on kwienken:issue-809 into ee66567 on softlayer:master.

@allmightyspiff
Copy link
Member

Can you apply this change to

def _get_package_items(self):

so that both hardware and virtual use your get_package_by_key method, then we can remove

get_package_id_by_type
get_package_by_type
get_packages_of_type
from the ordering manager and clean it up a bit.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 82.563% when pulling 4ca40e5 on kwienken:issue-809 into ee66567 on softlayer:master.

@kwienken
Copy link
Contributor Author

@allmightyspiff I've updated the virtual manager to use get_package_by_key but I left the _by_type methods in the ordering manager. After talking with @sergiocarlosmorales I think it's best that we leave them in to prevent breaking other users applications that may include the package and use the ordering library.

Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@allmightyspiff allmightyspiff merged commit 47199db into softlayer:master Mar 31, 2017
@sergiocarlosmorales
Copy link
Member

Agree here, if we were to remove that functionality we might break existing customer implementations and since it would be backwards incompatible, we'd likely need to bump the release version. Too much, let's keep it simple for now :)

@camporter
Copy link
Member

There appears to be reports of issues with packages having the same type and the package not being chosen here before this change (on 5.2.1). @allmightyspiff any way we could get a new minor or bugfix release out with this included?

@allmightyspiff
Copy link
Member

5.2.3 is now released.

@kwienken kwienken deleted the issue-809 branch January 15, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants