Skip to content

Conversation

@FernandoOjeda
Copy link
Contributor

Add functionality to order a hw for capacityRestrictionType PROCCESOR #1289.

@FernandoOjeda
Copy link
Contributor Author

FernandoOjeda commented Jun 30, 2020

I did not find any relationship between the os (capacityRestrictionType PROCCESOR) and the other items such as cpu.

e.g.
https://sldn.softlayer.com/reference/services/SoftLayer_Product_Package/getItemPrices/

[
{
        "currentPriceFlag": null,
        "id": 49515,
        "itemId": 4578,
        "laborFee": "0",
        "locationGroupId": null,
        "recurringFee": "75",
        "tierMinimumThreshold": null,
        "item": {
            "capacity": "3.5",
            "description": "Single Intel Xeon E3-1270 v3 (4 Cores, 3.50 GHz)",
            "id": 4578,
            "keyName": "INTEL_SINGLE_XEON_1270_3_50",
        }
    },
{
        "currentPriceFlag": null,
        "id": 48999,
        "itemId": 6125,
        "laborFee": "0",
        "locationGroupId": null,
        "recurringFee": "45",
        "setupFee": "0",
        "sort": 1,
        "capacityRestrictionMaximum": "1",
        "capacityRestrictionMinimum": "1",
        "capacityRestrictionType": "PROCESSOR",
        "item": {
            "description": "Red Hat Enterprise Linux 7.x (64 bit)  (per-processor licensing)",
            "id": 6125,
            "keyName": "OS_RHEL_7_1_64_BIT",
        }
    },
]

@FernandoOjeda FernandoOjeda self-assigned this Jun 30, 2020
@allmightyspiff allmightyspiff added Bug Ordering Anything related to ordering labels Jun 30, 2020
@allmightyspiff allmightyspiff linked an issue Jun 30, 2020 that may be closed by this pull request
@allmightyspiff
Copy link
Member

So, I don't think users have the option to change the number of Processors in a package, so just skipping PROCESSOR restricted items should be technically fine, I think refactoring this method a bit would help make it more readable.

Here is what I'm thinking.

    @staticmethod
    def get_item_price_id(core, prices):
        """get item price id"""
        price_id = None
        for price in prices:
            # This is the default location group then
            if not price['locationGroupId']:
                restriction = price.get('capacityRestrictionType', False)
                # There is a price restriction. Make sure the price is within the restriction
                if restriction:
                    capacity_min = int(price.get('capacityRestrictionMinimum', -1))
                    capacity_max = int(price.get('capacityRestrictionMaximum', -1))
                    if restriction == "STORAGE":
                        if capacity_min <= int(core) <= capacity_max:
                            price_id = price['id'] 
                    if restriction == "CORE":
                        if capacity_min <= int(core) <= capacity_max:
                            price_id = price['id'] 
                    if restriction == "PROCESSOR":
                        price_id = price['id']
                # No price restrictions
                else:
                    price_id = price['id']

        return price_id

This way we can handle each restriction a little different if needed. Still not great looking, but what can you do.

I thought about just looking up the processor capacity but that seems like a bit more work than just assuming the package will only have 1 type of processor, which so far seems to be the case.

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.

While the change itself is fine, I'd like to take this time to refactor this method a bit to be more readable.
Having 3 different if branches on one line isn't great in my opinion. This way if there are ever any other restrictions added, should be easier to work around.

@FernandoOjeda
Copy link
Contributor Author

FernandoOjeda commented Jun 30, 2020

yep, I am agree refactoring this method using different if for each restriction. It looks good.

I changing the restriction == "STORAGE" to "STORAGE" in restriction because there are other "capacityRestrictionType" for storage such as STORAGE_SPACE, STORAGE_TIER_LEVEL.

It is ok the implementation below or I change the rest of if statements the same that "STORAGE" in restriction.

                if "STORAGE" in restriction:
                    if capacity_min <= int(core) <= capacity_max:
                        price_id = price['id']
                if restriction == "CORE":
                    if capacity_min <= int(core) <= capacity_max:
                        price_id = price['id']
                if restriction == "PROCESSOR":
                    price_id = price['id']

@allmightyspiff
Copy link
Member

It is ok the implementation below or I change the rest of if statements the same that "STORAGE" in restriction.

Yeah that should be fine.

@FernandoOjeda
Copy link
Contributor Author

Done.

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.

LGTM

@allmightyspiff allmightyspiff merged commit 1435ab7 into softlayer:master Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Ordering Anything related to ordering

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ordering not able to lookup RHEL prices properly

2 participants