Added OVH Ip loadbalancing module for managing backends #1324

pascalheraud · 2015-12-03T23:03:22Z

Hello,

Here is a module I've written for managing OVH IP Loadbalancer.
The OVH api is a very comprehensive API and could need several Ansible modules.
I think it's a good start for supporting this API through Ansible.

Pascal.

mscherer · 2015-12-04T10:49:13Z

cloud/ovh/ovh_ip_loadbalancing_backend.py

+'''
+
+import sys
+import ovh


you should wrap that import in a try/catch ( like https://github.com/ansible/ansible-modules-extras/blob/devel/system/firewalld.py#L94 ) and show a error message if the ovh module is not present.

mscherer · 2015-12-04T11:03:28Z

There is also not much error handling in the module, so if something fail, we would have a backtrace rather than a clean error, could it be added ?

gregdek · 2015-12-04T16:09:56Z

@pascalheraud After some review, it looks as though this PR needs some revisions.

Please make the suggested revisions, and when you're done, please add a comment with the text "ready_for_review" and we will continue with the review process. Thanks!

- Caught the exception from import ovh to provide a proper message to the user - Removed unuseful brackets - Added a else to check the state instead of two if - Changed the module to be added to 2.0 - Added exceptions handling for all APIs calls with a clear message including the return from the API. And : - Fixed dependency of OVH api to 0.3.5

pascalheraud · 2015-12-05T17:00:22Z

I've made some improvements based on @mscherer review.
Thank you for your input.

Pascal

ready_for_review

pascalheraud · 2015-12-05T20:34:21Z

ready_for_review

gregdek · 2015-12-06T15:16:47Z

Thanks @pascalheraud for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

mscherer · 2015-12-06T17:09:52Z

cloud/ovh/ovh_ip_loadbalancing_backend.py

+
+def waitForNoTask(client, name):
+    while len(client.get('/ip/loadBalancing/{}/task'.format(name)))>0:
+        time.sleep(1)  # Delay for 1 sec


There is no timeout, nor any kind of mutex, so this could loop forever in theory, no ? Shouldn't something be done for that ?

mscherer · 2015-12-06T17:21:47Z

Also, as a general note, I think it would be nicer to make sure that the code is clean when it come to the pep8 (at least, for new code, since the discussion I see on the list are mostly about fixing the existing code base, but new one have no reason to not be clean). But I am not sure on what is the consensus of the community around it.

Can you verify with make pep8 that at least, the module is clean ? (the rest of the code base is not, so you will see a ton of errors)

completion of a task.

pascalheraud · 2015-12-06T17:48:53Z

Added a timeout param to prevent infinite loop

Added missing exceptions handling

pascalheraud · 2015-12-06T18:05:56Z

Added exceptions handling
Removed unnecessary moduleChanged=False

pascalheraud · 2015-12-07T23:22:56Z

I checked the module with pep8 and it has now 0 warning in pep8.

pascalheraud · 2015-12-07T23:23:15Z

ready_for_review

gregdek · 2015-12-08T15:03:45Z

Thanks @pascalheraud for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

abadger · 2016-04-20T18:34:05Z

I think the code here looks good now. However, there's a lot of string formatting issues. Some of them might break the docs building and some of them will lead to oddly formatted messages. I'll add some contextual notes to the first occurrence of the problems and you can fix the rest. Once you fix those I think we can merge this.

abadger · 2016-04-20T18:39:14Z

cloud/ovh/ovh_ip_loadbalancing_backend.py

+notes:
+    - Uses the python OVH Api U(https://github.com/ovh/python-ovh). \
+    You have to create an application (a key and secret) with a consummer \
+    key as described into U(https://eu.api.ovh.com/g934.first_step_with_api)


Python multiline strings (started and stopped using triple quotes like `'''and"""``` do not need the backslash as a line continuation character. So you can remove all trailing backslashes at the end of lines within triple quoted strings.

I can't be certain but you may have added them in here because you weren't sure how yaml was parsing the documentation. If so, the problem is not that the line needs to be continued. Instead, it's that the indentation level is wrong. In order for YAML to understand that all of this is one entry, you probably need to make sure the indent lines up with the other text, not with the character introducing the element. IE: The "Y" in You needs to be directly underneath the "U" in Uses. Similarly, the "k" in key needs to be directly underneath those.

gregdek · 2016-05-04T18:40:00Z

@pascalheraud please respond to @abadger's concerns, and when you've done so, please respond with "ready_for_review" and we'll get it back on track. Thanks!

pascalheraud · 2016-05-08T20:40:11Z

ready_for_review

gregdek · 2016-05-08T22:27:09Z

Thanks @pascalheraud for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

hedii · 2016-05-09T06:55:23Z

shipit

hedii · 2016-05-11T13:01:44Z

@mscherer can you take a look?

Flaburgan · 2016-05-19T08:03:18Z

It would be awesome to see this merged. Looks like the code is OK, what are we waiting for?

hedii · 2016-05-19T08:07:38Z

@Flaburgan test it, type 'shipit' in a comment if it is ok, and the plugin will be marked for inclusion.
(the plugin needs 2 'shipit' comments, i already gave one. Only another one is missing)

Flaburgan · 2016-05-19T08:16:21Z

@mscherer already gave another one and he is deeply involved in ansible ;)

hedii · 2016-05-19T08:24:11Z

@Flaburgan He gave one at the previous review stage, not this final one, no ?

mscherer · 2016-05-19T10:56:39Z

Yeah, I think the code is ok (still need to review one last time), but testing it would requires a bit more time from me, cause i would need a account on ovh cloud, and then setup 2 cloud servers and everything, that need a bit more time on my side and doc reading than just reading python.

pascalheraud · 2016-05-19T22:10:32Z

Testing this module will be quite hard if you dont have a iploadbalancing and two servers with correct setup.
To setup a env like that will be time and money !

theyough · 2016-07-21T22:54:25Z

👍

abadger · 2016-07-26T17:02:31Z

cloud/ovh/ovh_ip_loadbalancing_backend.py

+- ovh_ip_loadbalancing name=ip-1.1.1.1 backend=212.1.1.1 
+  state=present probe=none weight=8 
+  endpoint=ovh-eu application_key=yourkey 
+  application_secret=yoursecret consumer_key=yourconsumerkey


The examples will still format funny. They should look liek this:

- ovh_ip_loadbalancing: name: ip-1.1.1.1 backend: 212.1.1.1 state: present probe: none weight: 8 endpoint: ovh-eu application_key: your_key application_secret: yoursecret consumer_key: yourconsumerkey

abadger · 2016-07-26T17:03:56Z

per our previous discussion of this code, this is looking good. I found a few things to change but I can do those when I merge.

abadger · 2016-07-26T17:56:51Z

I'm heading out to ansiblefest in San Francisco right now. So I'll get to it once I arrive unless you get to it first.

abadger · 2016-07-27T22:54:09Z

Merged. Starts here: a767da1

abadger · 2016-07-27T22:55:39Z

Thanks @pascalheraud for writing this, @mscherer and @hedii for reviewing and testing. Sorry it took so long.

pascalheraud · 2016-08-01T09:22:32Z

Thank you very much ! :)

Added OVH Ip loadbalancing module for managing backends

a38590b

mscherer reviewed Dec 4, 2015
View reviewed changes

gregdek added cloud needs_revision new_plugin labels Dec 4, 2015

gregdek added community_review and removed needs_revision labels Dec 6, 2015

mscherer reviewed Dec 6, 2015
View reviewed changes

Added a timeout param to prevent infinite loop while waiting for

6c4bca3

completion of a task.

Removed unnecessary moduleChanged=False

3634eba

Added missing exceptions handling

pascalheraud mentioned this pull request Dec 7, 2015

Pull request to ansible-modules-extra gheesh/ansible-ovh-dns#1

Open

Fixed the module to be compliant with pep8

85daed9

abadger reviewed Apr 20, 2016
View reviewed changes

gregdek added needs_revision and removed shipit labels May 4, 2016

Fixed multiline string formatting issues

65892df

gregdek added community_review and removed needs_revision labels May 8, 2016

abadger reviewed Jul 26, 2016
View reviewed changes

abadger closed this Jul 27, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OVH Ip loadbalancing module for managing backends #1324

Added OVH Ip loadbalancing module for managing backends #1324

pascalheraud commented Dec 3, 2015

mscherer Dec 4, 2015

mscherer commented Dec 4, 2015

gregdek commented Dec 4, 2015

pascalheraud commented Dec 5, 2015

pascalheraud commented Dec 5, 2015

gregdek commented Dec 6, 2015

mscherer Dec 6, 2015

mscherer commented Dec 6, 2015

pascalheraud commented Dec 6, 2015

pascalheraud commented Dec 6, 2015

pascalheraud commented Dec 7, 2015

pascalheraud commented Dec 7, 2015

gregdek commented Dec 8, 2015

abadger commented Apr 20, 2016

abadger Apr 20, 2016

gregdek commented May 4, 2016

pascalheraud commented May 8, 2016

gregdek commented May 8, 2016

hedii commented May 9, 2016

hedii commented May 11, 2016

Flaburgan commented May 19, 2016

hedii commented May 19, 2016

Flaburgan commented May 19, 2016

hedii commented May 19, 2016 •

edited

mscherer commented May 19, 2016

pascalheraud commented May 19, 2016

theyough commented Jul 21, 2016

abadger Jul 26, 2016 •

edited

abadger commented Jul 26, 2016

abadger commented Jul 26, 2016

abadger commented Jul 27, 2016

abadger commented Jul 27, 2016

pascalheraud commented Aug 1, 2016

Added OVH Ip loadbalancing module for managing backends #1324

Added OVH Ip loadbalancing module for managing backends #1324

Conversation

pascalheraud commented Dec 3, 2015

mscherer Dec 4, 2015

Choose a reason for hiding this comment

mscherer commented Dec 4, 2015

gregdek commented Dec 4, 2015

pascalheraud commented Dec 5, 2015

pascalheraud commented Dec 5, 2015

gregdek commented Dec 6, 2015

mscherer Dec 6, 2015

Choose a reason for hiding this comment

mscherer commented Dec 6, 2015

pascalheraud commented Dec 6, 2015

pascalheraud commented Dec 6, 2015

pascalheraud commented Dec 7, 2015

pascalheraud commented Dec 7, 2015

gregdek commented Dec 8, 2015

abadger commented Apr 20, 2016

abadger Apr 20, 2016

Choose a reason for hiding this comment

gregdek commented May 4, 2016

pascalheraud commented May 8, 2016

gregdek commented May 8, 2016

hedii commented May 9, 2016

hedii commented May 11, 2016

Flaburgan commented May 19, 2016

hedii commented May 19, 2016

Flaburgan commented May 19, 2016

hedii commented May 19, 2016 • edited

mscherer commented May 19, 2016

pascalheraud commented May 19, 2016

theyough commented Jul 21, 2016

abadger Jul 26, 2016 • edited

Choose a reason for hiding this comment

abadger commented Jul 26, 2016

abadger commented Jul 26, 2016

abadger commented Jul 27, 2016

abadger commented Jul 27, 2016

pascalheraud commented Aug 1, 2016

hedii commented May 19, 2016 •

edited

abadger Jul 26, 2016 •

edited