Kitware / cumulus

A REST API for creating and using cloud clusters
Apache License 2.0
27 stars 9 forks source link

fixed return value for ec2 running_instances #304

Closed TristanWright closed 7 years ago

TristanWright commented 7 years ago

looking at HPCCloud issue 17 I was getting an error back. This fixes that and returns the right value for number of running instances for a profile.

I'm not sure why tests weren't catching this?

cjh1 commented 7 years ago

@TristanWright The only sort of test we would have for this would mock out the EC2 part so would probably miss this. LGTM, but I would like @kotfic to take a look.

cjh1 commented 7 years ago

@TristanWright Would you be able to implement the suggested change so we can get this closed off?

TristanWright commented 7 years ago

Looking back at this again, it should be more:

def get_instances(self, cluster_id):
        return [self._get_instance_vars(host) for host in self.ec2.instances
                .filter(
                Filters=[{'Name': 'instance-state-name', 'Values': ['running']},
                         {'Name': 'cluster_id', 'Values': [cluster_id]}]
                )]

    def running_instances(self, cluster_id):
        return len([host for host in self.get_instances(cluster_id).keys()
                    if host != '_meta'])

should 'cluster_id' arg in running_instances be passed through a for cluster in profile(?) from the api endpoint?

cjh1 commented 7 years ago

@TristanWright To be honest looking at this issue again I'm not sure what the problem with the original implementation of running_instances was. My understanding is its supposed to return the number of running instances associated with a give aws profile?

TristanWright commented 7 years ago

looking at HPCCloud issue 17 I was getting an error back. This fixes that and returns the right value for number of running instances for a profile.

I guess I was getting some error, re-testing the API on master doesn't give any error (anymore?). Closing this.