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

Add methods ex_add/ex_delete port_forwarding_rule to CloudstackNode (similar to ip_forwarding_rule) #196

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@gigimon
Contributor

gigimon commented Dec 16, 2013

Hi!

I have added port forwarding method to CloudstackNode object because class CloudstackPortForwardingRule has a method delete which doesn't work. And change call signature because node object was not in first place

And want ask about naming of port forwarding methods. We have ex_add_ip_forwarding_rule and ex_create_port_forwarding_rule maybe rename one of this?

@sebgoa

This comment has been minimized.

Show comment
Hide comment
@sebgoa

sebgoa Dec 16, 2013

Member

I am not sure what you mean by 'has a method delete which doesn't work' ? what exactly does not work ? you did not seem to modify that method.

Also what do you mean by "change call signature...'

Ok to rename ex_add_ip_forwarding_rule to ex_create_ip_frowarding_rule, but we will have to keep track of the fact this will break backward compatibility.

Member

sebgoa commented Dec 16, 2013

I am not sure what you mean by 'has a method delete which doesn't work' ? what exactly does not work ? you did not seem to modify that method.

Also what do you mean by "change call signature...'

Ok to rename ex_add_ip_forwarding_rule to ex_create_ip_frowarding_rule, but we will have to keep track of the fact this will break backward compatibility.

@gigimon

This comment has been minimized.

Show comment
Hide comment
@gigimon

gigimon Dec 16, 2013

Contributor

ex_create_port_forwarding_rule was:

def ex_create_port_forwarding_rule(self, address, private_port,
                                       public_port, protocol, node,
                                       public_end_port=None,
                                       private_end_port=None,
                                       openfirewall=True):

Now it:

def ex_create_port_forwarding_rule(self, node, address,
                                       private_port, public_port,
                                       protocol,
                                       public_end_port=None,
                                       private_end_port=None,
                                       openfirewall=True):

Because in all methods argument 'node' in first place.

About method delete:
Class CloudStackPortForwardingRule has method delete, it call self.node.ex_delete_port_forwarding_rule(self), but CloudstackNode doesn't has a method ex_delete_port_forwarding_rule - I fix this.

Ok, I'm rename method and fix test.

Sorry for bad english :(

Contributor

gigimon commented Dec 16, 2013

ex_create_port_forwarding_rule was:

def ex_create_port_forwarding_rule(self, address, private_port,
                                       public_port, protocol, node,
                                       public_end_port=None,
                                       private_end_port=None,
                                       openfirewall=True):

Now it:

def ex_create_port_forwarding_rule(self, node, address,
                                       private_port, public_port,
                                       protocol,
                                       public_end_port=None,
                                       private_end_port=None,
                                       openfirewall=True):

Because in all methods argument 'node' in first place.

About method delete:
Class CloudStackPortForwardingRule has method delete, it call self.node.ex_delete_port_forwarding_rule(self), but CloudstackNode doesn't has a method ex_delete_port_forwarding_rule - I fix this.

Ok, I'm rename method and fix test.

Sorry for bad english :(

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 16, 2013

Member

@gigimon Can you please also add tests which cover those newly added methods?

Member

Kami commented Dec 16, 2013

@gigimon Can you please also add tests which cover those newly added methods?

@gigimon

This comment has been minimized.

Show comment
Hide comment
@gigimon

gigimon Dec 17, 2013

Contributor

@Kami ok, later today I add tests

Contributor

gigimon commented Dec 17, 2013

@Kami ok, later today I add tests

@gigimon

This comment has been minimized.

Show comment
Hide comment
@gigimon

gigimon Dec 17, 2013

Contributor

@Kami I've added tests, please check, if all good I create one commit with all changess

Contributor

gigimon commented Dec 17, 2013

@Kami I've added tests, please check, if all good I create one commit with all changess

@@ -42,16 +42,36 @@ def ex_release_public_ip(self, address):
"Release a public IP that this node holds."
return self.driver.ex_release_public_ip(self, address)
def ex_add_ip_forwarding_rule(self, address, protocol, start_port,
end_port=None):
def ex_create_ip_forwarding_rule(self, address, protocol,

This comment has been minimized.

@Kami

Kami Dec 18, 2013

Member

I just went over the pull request again and I'm kinda confused now.

Why did you rename methods from add to create (ex_add_ip_forwarding_rule -> ex_create_ip_forwarding_rule)? Is it for consistency reasons or is it something else?

@Kami

Kami Dec 18, 2013

Member

I just went over the pull request again and I'm kinda confused now.

Why did you rename methods from add to create (ex_add_ip_forwarding_rule -> ex_create_ip_forwarding_rule)? Is it for consistency reasons or is it something else?

This comment has been minimized.

@gigimon

gigimon Dec 18, 2013

Contributor

Yes, it's for consistency reason. In pull request @runseb say me rename to create

@gigimon

gigimon Dec 18, 2013

Contributor

Yes, it's for consistency reason. In pull request @runseb say me rename to create

This comment has been minimized.

@Kami

Kami Dec 18, 2013

Member

OK, sounds good. Need to note this in the changelog though since it's a backward incompatible change.

@Kami

Kami Dec 18, 2013

Member

OK, sounds good. Need to note this in the changelog though since it's a backward incompatible change.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 18, 2013

Member

(I also plan open a corresponding JIRA ticket so it will be easier to track this issue)

@runseb While reviewing this pull request I noticed a couple of unrelated issues in the driver.

ex_allocate_public_ip and ex_release_public_ip method on the CloudStackNode class seems to be broken.

Both of those methods call methods on the driver class with invalid arguments. They pass node object to the driver method, but the driver method doesn't take a node object.

See:

We have two options:

  1. Fix those methods to actually do what the docstring says - allocate the IP and bind it to the node
  2. Remove those methods from the CloudStackNode class since in the current form, they don't belong there.

We also need better test coverage to detect issues like this automatically in the future.

Edit: I've opened a JIRA issue for it - https://issues.apache.org/jira/browse/LIBCLOUD-462. We can move this off topic debate there.

Member

Kami commented Dec 18, 2013

(I also plan open a corresponding JIRA ticket so it will be easier to track this issue)

@runseb While reviewing this pull request I noticed a couple of unrelated issues in the driver.

ex_allocate_public_ip and ex_release_public_ip method on the CloudStackNode class seems to be broken.

Both of those methods call methods on the driver class with invalid arguments. They pass node object to the driver method, but the driver method doesn't take a node object.

See:

We have two options:

  1. Fix those methods to actually do what the docstring says - allocate the IP and bind it to the node
  2. Remove those methods from the CloudStackNode class since in the current form, they don't belong there.

We also need better test coverage to detect issues like this automatically in the future.

Edit: I've opened a JIRA issue for it - https://issues.apache.org/jira/browse/LIBCLOUD-462. We can move this off topic debate there.

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Dec 18, 2013

Member

@gigimon Rebased the commits and merged those changes into trunk. This PR can be closed now.

Thanks.

Member

Kami commented Dec 18, 2013

@gigimon Rebased the commits and merged those changes into trunk. This PR can be closed now.

Thanks.

* add methods ex_create/ex_delete port_forwarding_rule for CloudStack…
…Node object (similar to ip_forwarding_rule)

* rename ex_add_ip_forwarding_rule to ex_create_ip_forwarding_rule
* add tests for CloudStackNode.ex_create/ex_delete_port_forwarding_rule
@gigimon

This comment has been minimized.

Show comment
Hide comment
@gigimon

gigimon Dec 18, 2013

Contributor

@Kami done

Contributor

gigimon commented Dec 18, 2013

@Kami done

@gigimon gigimon closed this Dec 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment