Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pratikmallya
Copy link

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/

@briancurtin
Copy link
Contributor

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?

@pratikmallya
Copy link
Author

@briancurtin what do you mean by a "fully featured client"?

@briancurtin
Copy link
Contributor

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?

@pratikmallya
Copy link
Author

@briancurtin will do. Thanks for the clarification!

@pratikmallya pratikmallya force-pushed the master branch 2 times, most recently from 782af7f to 79211ef Compare December 11, 2014 21:24
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 79211ef on pratikmallya:master into 6657a54 on rackspace:working.

@@ -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:
Copy link
Contributor

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.

@rs-randallburt
Copy link
Contributor

👎 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.

@pratikmallya pratikmallya force-pushed the master branch 2 times, most recently from a292f1c to 4fa899a Compare December 18, 2014 17:15
@pratikmallya
Copy link
Author

@rs-randallburt update the code to take care of the minor nits.
@rs-randallburt @briancurtin yes, this client implements everything that is currently available via the RackConnect API. I was referring to the features mentioned under "Coming Soon" in the api page.

@rs-randallburt
Copy link
Contributor

👍 Thanks!

@pratikmallya pratikmallya force-pushed the master branch 6 times, most recently from cd81029 to 5351467 Compare December 20, 2014 22:15
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.65%) when pulling 5351467 on pratikmallya:master into ab62b33 on rackspace:working.

@pratikmallya
Copy link
Author

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:
Copy link
Contributor

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.

Copy link
Contributor

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.

@pratikmallya
Copy link
Author

@rs-randallburt made the change that you suggested.

@rs-randallburt
Copy link
Contributor

👍 thanks, lgtm.

@pratikmallya
Copy link
Author

ping. I would really love for this patch to get merged in.

@pratikmallya
Copy link
Author

Ping. Is there any kind of roadmap/plan to merge these patches in?

@ghost
Copy link

ghost commented Mar 30, 2015

It would indeed be nice to see this RCv3 functionality in pyrax.

@mhsparks
Copy link

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?

@briancurtin
Copy link
Contributor

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?

@pratikmallya
Copy link
Author

@briancurtin sure thing. Where should I add the documentation? Are we talking about inline code documentation?

@briancurtin
Copy link
Contributor

In the docs folder, similar to how the other services are documented. It's more of a user guide.

@EdLeafe
Copy link
Contributor

EdLeafe commented Jul 29, 2015

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.

@briancurtin
Copy link
Contributor

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.
@davidquarles
Copy link

@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.

@briancurtin
Copy link
Contributor

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.

@rs-randallburt
Copy link
Contributor

@davidquarles fwiw, we've resigned ourselves to maintaining our own fork of this code until there's a supported client.

@proffalken
Copy link

@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...

@rs-randallburt
Copy link
Contributor

@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.

@drewbrew drewbrew changed the base branch from working to master January 28, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants