Skip to content
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

renamed get_all_vmtypes method to get_all_instance_types to better match... #2118

Merged
merged 2 commits into from Mar 5, 2014

Conversation

dkavanagh
Copy link
Contributor

... actual DescribeInstanceTypes call euca switched to.

I've been meaning to update this call for a while since eucalyptus changed from DescribeVmTypes to DescribeInstanceTypes right after the initial pull request for this code. Recently, the euca: namespace was removed from the XML response, so I also made that change. Since the old call didn't work, this new code will work properly with the forthcoming Eucalyptus 4.0 release. This call should be pretty stable in Eucalyptus now, so I don't expect further changes unless we decide to offer more detailed information beyond cores/mem/disk

…tch actual DescribeInstanceTypes call euca switched to
@danielgtaylor
Copy link
Member

Will this change not break existing customers who upgrade boto but do not switch to Eucalyptus 4.0 yet? Can we do this in a backward-compatible way and also add a unit test for the returned object's attributes? Outside of those minor nits I'm happy to merge this in once fixed.

@dkavanagh
Copy link
Contributor Author

I guess what I was trying to explain is that this method likely wasn't
working for anybody due to a change that was made in Eucalyptus after this
was initially merged into boto. One part of this change fixes that problem.
The other part removes the namespace that we used to emit. So, I think the
chance for breakage is minimal.
On the testing front, there isn't really a good public endpoint we could
test against. It would require checking in some creds if we were to test
against the eucalyptus community cloud. At the moment, that's not up to the
version that I'm coding to (but it will be later in the spring). I'm open
to suggestions.

On Mon, Feb 24, 2014 at 6:02 PM, Daniel G. Taylor
notifications@github.comwrote:

Will this change not break existing customers who upgrade boto but do not
switch to Eucalyptus 4.0 yet? Can we do this in a backward-compatible way
and also add a unit test for the returned object's attributes? Outside of
those minor nits I'm happy to merge this in once fixed.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2118#issuecomment-35951362
.

@danielgtaylor
Copy link
Member

Ah I see. In that case I'd say that if you can add a quick unit test I'll merge it in. Thanks!

@danielgtaylor danielgtaylor self-assigned this Feb 25, 2014
@dkavanagh
Copy link
Contributor Author

Happy to. I seem to recall some other tests use some kind of mock
connection. I guess I am not sure exactly what'd I'd be doing in the test
(what would I need to test for)? Is it the case that I could throw some XML
at the instancetype class and validate the parsing?

David

On Tue, Feb 25, 2014 at 2:22 PM, Daniel G. Taylor
notifications@github.comwrote:

Ah I see. In that case I'd say that if you can add a quick unit test I'll
merge it in. Thanks!

Reply to this email directly or view it on GitHubhttps://github.com//pull/2118#issuecomment-36046816
.

@danielgtaylor
Copy link
Member

Exactly - you could copy and modify one of the existing tests, e.g:

https://github.com/boto/boto/blob/develop/tests/unit/ec2/test_connection.py#L83

class TestReservedInstanceOfferings(TestEC2ConnectionBase):

    def default_body(self):
        return """
            <DescribeReservedInstancesOfferingsResponse>
                <requestId>d3253568-edcf-4897-9a3d-fb28e0b3fa38</requestId>
                    <reservedInstancesOfferingsSet>
                    <item>
                        <reservedInstancesOfferingId>2964d1bf71d8</reservedInstancesOfferingId>
                        <instanceType>c1.medium</instanceType>
                        <availabilityZone>us-east-1c</availabilityZone>
                        <duration>94608000</duration>
                        <fixedPrice>775.0</fixedPrice>
                        <usagePrice>0.0</usagePrice>
                        <productDescription>product description</productDescription>
                        <instanceTenancy>default</instanceTenancy>
                        <currencyCode>USD</currencyCode>
                        <offeringType>Heavy Utilization</offeringType>
                        <recurringCharges>
                            <item>
                                <frequency>Hourly</frequency>
                                <amount>0.095</amount>
                            </item>
                        </recurringCharges>
                        <marketplace>false</marketplace>
                        <pricingDetailsSet>
                            <item>
                                <price>0.045</price>
                                <count>1</count>
                            </item>
                        </pricingDetailsSet>
                    </item>
                    <item>
                        <reservedInstancesOfferingId>2dce26e46889</reservedInstancesOfferingId>
                        <instanceType>c1.medium</instanceType>
                        <availabilityZone>us-east-1c</availabilityZone>
                        <duration>94608000</duration>
                        <fixedPrice>775.0</fixedPrice>
                        <usagePrice>0.0</usagePrice>
                        <productDescription>Linux/UNIX</productDescription>
                        <instanceTenancy>default</instanceTenancy>
                        <currencyCode>USD</currencyCode>
                        <offeringType>Heavy Utilization</offeringType>
                        <recurringCharges>
                            <item>
                                <frequency>Hourly</frequency>
                                <amount>0.035</amount>
                            </item>
                        </recurringCharges>
                        <marketplace>false</marketplace>
                        <pricingDetailsSet/>
                    </item>
                </reservedInstancesOfferingsSet>
                <nextToken>next_token</nextToken>
            </DescribeReservedInstancesOfferingsResponse>
        """

    def test_get_reserved_instance_offerings(self):
        self.set_http_response(status_code=200)
        response = self.ec2.get_all_reserved_instances_offerings()
        self.assertEqual(len(response), 2)
        instance = response[0]
        self.assertEqual(instance.id, '2964d1bf71d8')
        self.assertEqual(instance.instance_type, 'c1.medium')
        self.assertEqual(instance.availability_zone, 'us-east-1c')
        self.assertEqual(instance.duration, 94608000)
        self.assertEqual(instance.fixed_price, '775.0')
        self.assertEqual(instance.usage_price, '0.0')
        self.assertEqual(instance.description, 'product description')
        self.assertEqual(instance.instance_tenancy, 'default')
        self.assertEqual(instance.currency_code, 'USD')
        self.assertEqual(instance.offering_type, 'Heavy Utilization')
        self.assertEqual(len(instance.recurring_charges), 1)
        self.assertEqual(instance.recurring_charges[0].frequency, 'Hourly')
        self.assertEqual(instance.recurring_charges[0].amount, '0.095')
        self.assertEqual(len(instance.pricing_details), 1)
        self.assertEqual(instance.pricing_details[0].price, '0.045')
        self.assertEqual(instance.pricing_details[0].count, '1')

    def test_get_reserved_instance_offerings_params(self):
        self.set_http_response(status_code=200)
        self.ec2.get_all_reserved_instances_offerings(
            reserved_instances_offering_ids=['id1','id2'],
            instance_type='t1.micro',
            availability_zone='us-east-1',
            product_description='description',
            instance_tenancy='dedicated',
            offering_type='offering_type',
            include_marketplace=False,
            min_duration=100,
            max_duration=1000,
            max_instance_count=1,
            next_token='next_token',
            max_results=10
        )
        self.assert_request_parameters({
            'Action': 'DescribeReservedInstancesOfferings',
            'ReservedInstancesOfferingId.1': 'id1',
            'ReservedInstancesOfferingId.2': 'id2',
            'InstanceType': 't1.micro',
            'AvailabilityZone': 'us-east-1',
            'ProductDescription': 'description',
            'InstanceTenancy': 'dedicated',
            'OfferingType': 'offering_type',
            'IncludeMarketplace': 'false',
            'MinDuration': '100',
            'MaxDuration': '1000',
            'MaxInstanceCount': '1',
            'NextToken': 'next_token',
            'MaxResults': '10',},
             ignore_params_values=['AWSAccessKeyId', 'SignatureMethod',
                                   'SignatureVersion', 'Timestamp', 'Version'])

@dkavanagh
Copy link
Contributor Author

Good to go!

@danielgtaylor
Copy link
Member

Thanks for taking the time to add the test 👍

danielgtaylor added a commit that referenced this pull request Mar 5, 2014
Fix getting instance types for Eucalyptus 4.0. Fixes #2118.
@danielgtaylor danielgtaylor merged commit 18dc07d into boto:develop Mar 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants