-
-
Notifications
You must be signed in to change notification settings - Fork 206
Add client to talk to RackConnect api #521
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
base: master
Are you sure you want to change the base?
Conversation
I'm not too familiar with RackConnect, but what would it take to bring your patch from a simple client to a fully featured client? |
@briancurtin what do you mean by a "fully featured client"? |
I haven't reviewed this yet, but your first message is worded as if this is only a subset of the available functionality, as a simple client that only does a few things. Can you expand on what isn't supported in this change, if anything? |
@briancurtin will do. Thanks for the clarification! |
782af7f
to
79211ef
Compare
pyrax/__init__.py
Outdated
@@ -739,6 +745,8 @@ def _create_client(ep_name, region, public=True): | |||
client = cls(identity, region_name=region, management_url=ep, | |||
verify_ssl=verify_ssl, http_log_debug=_http_debug) | |||
client.user_agent = _make_agent_name(client.user_agent) | |||
if client is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to remove this block; print statements aren't pretty or needed here.
👎 for some minor nits inline. AFAIK, this client implements everything that's currently available via the RCv3 API; not sure what you think is missing @pratikmallya. |
a292f1c
to
4fa899a
Compare
@rs-randallburt update the code to take care of the minor nits. |
👍 Thanks! |
cd81029
to
5351467
Compare
Hey guys, what is the status on this PR? Is there a timeline for it to be reviewed and merged? @briancurtin @sivel |
pyrax/manager.py
Outdated
@@ -171,6 +171,8 @@ def _data_from_response(self, resp_body, key=None): | |||
listing responses the same way, so overriding this method allows | |||
subclasses to handle extraction for those outliers. | |||
""" | |||
if self.plural_response_key is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this may actually break listing monitoring zones and possibly other things that rely on the behavior cited in the comment starting on 180 of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I change 174-175 to:
# occasionally, its just a list (rackconnect for example)
if isinstance(resp_body, list):
return resp_body
Then everything works again.
@rs-randallburt made the change that you suggested. |
👍 thanks, lgtm. |
ping. I would really love for this patch to get merged in. |
Ping. Is there any kind of roadmap/plan to merge these patches in? |
It would indeed be nice to see this RCv3 functionality in pyrax. |
We're using this branch to successfully add and remove cloud servers to our load balancer pool. Anything we can do to help get it merged? Does it need some docs? |
I asked several times internally and never got any response about who will not only finish but also maintain this code. We are moving away from pyrax and will soon be announcing deprecation, so while this is something that has been around and would be acceptable to merge at some point before the deprecation, no one actively works on pyrax so this has to be taken on by some other person/team at Rackspace. @pratikmallya are you willing to write documentation and look after this code once it's merged? |
@briancurtin sure thing. Where should I add the documentation? Are we talking about inline code documentation? |
In the docs folder, similar to how the other services are documented. It's more of a user guide. |
I hate to say this, but you've already deprecated pyrax. If you are telling people not to invest in development of pyrax for the future, it's deprecated. No one with any sense would continue to develop anything except a trivial throw-away app with it. You just haven't given a path forward yet, and that's totally un-fanatical. |
As I said, we're working on it. |
This patch adds a client to talk with the rackconnect v3 api. It provides methods to add/remove cloudservers to rackconnected networks and to assign/remove public ip addresses to nodes in the loadbalancer pool.
@pratikmallya @briancurtin is there any update here? is there anything we can do to help if this is still blocked? i would very much like to write a few new ansible modules around this code, whether it is soon-to-be-deprecated or not (unless there is another option?), and i'm happy to help out here in whatever fashion to get this stuff live. fwiw we'll fork, rebase and test in the near-term. |
I don't have any plans of merging this because we don't have any plans of releasing or supporting it at this time. I've heard rumblings of adding RackConnect support to the Rackspace plugin for OpenStack SDK, but don't have any estimates for that yet. |
@davidquarles fwiw, we've resigned ourselves to maintaining our own fork of this code until there's a supported client. |
@rs-randallburt Is that fork available anywhere? We need to talk to a RC v3 in a new env (all the previous ones appear to be RC v2) and it would be great to be able to install your version of PyRAX into our venv's... |
@proffalken no, but you should be able to grab this patch and merge it locally. In the meantime, I can ask around and see what we can make available if anything. |
This patch adds a client to talk with the rackconnect api.
Currently, it only deals with adding/removing dedicated
loadbalancers and assigning/removing public IP's to
cloudservers. These are the only services which are
currently (at the time of this PR) exposed via the
rackconnect api: http://docs.rcv3.apiary.io/