Skip to content

WIP: Update to support the API changes in consulate library. Fixes #22#23

Open
Imaclean74 wants to merge 3 commits intovsudilov:developfrom
Imaclean74:integrate-consulate-changes
Open

WIP: Update to support the API changes in consulate library. Fixes #22#23
Imaclean74 wants to merge 3 commits intovsudilov:developfrom
Imaclean74:integrate-consulate-changes

Conversation

@Imaclean74
Copy link

As described. The main change is really that much of what used to be on the Session class is now on the root Consul class in the consulate package. Also, consulate now throws its own error if the agent is unavailable or there is some other issue making a request to it.
Marking WIP - since the consulate library hasn't actually released its changes yet.

@Imaclean74
Copy link
Author

oh yeah - and the build will fail until we have an updated consulate package. I've pinged the maintainer to see when he will be able to push one.

@vsudilov
Copy link
Owner

Hi, thanks for the keeping it up to date! This seems good to merge if/when those upstream changes are release. The only suggestion I'd make would be to keep backwards compatibility so that anyone that already uses this library doesn't have to update their code.

That could be as easy as

   self.session = self.consul = ...

We could add a bit more complexity and print a deprecation warning for users if they access self.session instead of self.consul (via @Property would probably work) going forward.

Let me know if you're up for adding that to this PR or not. Thanks!

@Imaclean74
Copy link
Author

Imaclean74 commented Jul 17, 2018

Thanks. Those changes make sense. I've added back session as a @Property - with a deprecation warning message when its referenced.
btw - the upstream owner of consulate ( Gavin ) got back to me and is hoping to get a release out in the next few weeks.


@property
def session(self):
self.app.logger.warning("session field deprecated - please use .consul")
Copy link
Owner

Choose a reason for hiding this comment

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

I'm cool with this, and here's an alternative just in case you weren't aware: https://docs.python.org/3/library/warnings.html#warning-categories

Copy link
Author

Choose a reason for hiding this comment

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

thanks. I did give warnings.warn a try - but the output is captured by the flask framework in the same way as print() etc. So I figured people won't actually see the warning output. Unless I'm missing something.

Copy link
Owner

@vsudilov vsudilov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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