Skip to content

Conversation

@vishy1618
Copy link

  • Added ex_start_node method to the ec2 compute driver
  • Added ex_stop_node method to the ec2 compute driver
  • Added a new _get_state_boolean function to get the state of the node while starting or stopping

Copy link
Member

Choose a reason for hiding this comment

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

Lets not do this and use xpath.

Since we will always operate just on a single instance something like this should work:

state = findall(element=element, xpath='instancesSet/item/currentState/name', namespace=NAMESPACE)[0].text
return state in ('pending', 'stopping', 'starting')

@Kami
Copy link
Member

Kami commented Nov 4, 2011

Lets also add some tests for the new functionality.

For the examples you can see test/compute/test_ec2.py. You will also need two test fixtures (one for start and one for stop instance response).

@vishy1618
Copy link
Author

OK, will do it accordingly. Thanks for the inputs!

2011/11/4 Tomaž Muraus <
reply@reply.github.com>

Lets also add some tests for the new functionality.

For the examples you can see test/compute/test_ec2.py. You will also
need two test fixtures (one for start and one for stop instance response).


Reply to this email directly or view it on GitHub:
#35 (comment)

Suvish V.T.

@vishy1618
Copy link
Author

Hi, will the patches be accepted for the next release?

@Kami
Copy link
Member

Kami commented Nov 9, 2011

I will take a look at the updated patch later on today.

Copy link
Member

Choose a reason for hiding this comment

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

You need to add and load fixtures otherwise this tests will fail.

@vishy1618
Copy link
Author

I hope I have written the tests appropriately now. I ran the tests, and they work. I have also added some more functionality to the driver (elastic address association, describe all the addresses, list only specific nodes).

@Kami
Copy link
Member

Kami commented Nov 16, 2011

I have merged your changes without the ex_list_nodes for now (r120273 - http://svn.apache.org/viewvc?view=revision&revision=1202734).

Currently ex_list_nodes contains too much copy and pasted code from list_nodes and I'm not even sure if this is the best approach. For this function it might even make more sense to take a list of node ids as an argument instead of a list of Node objects.

Some thing to keep in mind for future contributions:

  • work on your changes in a separate branch
  • always make sure your branch with changes is synced with the latest trunk
  • don't include trailing whitespace
  • run pep8 on the changed file (line-length, line breaks)

Thanks.

@vishy1618
Copy link
Author

Thanks a lot, Tomaz. I'll definitely take note of your suggestions.

Regarding the ex_list_nodes, I had previously added a list of node_ids as the parameter, but reverted to using a list of nodes as I saw that was the pattern used elsewhere in the code. How about adding a parameter of node_ids=None to the list_nodes method itself?

@Kami
Copy link
Member

Kami commented Nov 20, 2011

Yeah, I would be ok with adding ex_node_ids argument to the list_nodes method.

It also needs to be clearly documented what it does.

@vishy1618 vishy1618 merged commit 7f98954 into apache:trunk Nov 25, 2011
Kami pushed a commit to Kami/libcloud that referenced this pull request Oct 28, 2021
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.

2 participants