Skip to content
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

Support passing in a Session object #13

Closed
oliverlockwood opened this issue Oct 18, 2017 · 7 comments
Closed

Support passing in a Session object #13

oliverlockwood opened this issue Oct 18, 2017 · 7 comments

Comments

@oliverlockwood
Copy link
Contributor

oliverlockwood commented Oct 18, 2017

Users may already have a requests.Session object and wish to reuse that rather than having httsleep create a blank one. This would save users from re-entering standard headers, settings etc. that they've already configured elsewhere.

Ideal usage afterwards:

    httsleep(url, until=until, alarms=alarms, session=my_session)

~~~Workaround~~~ Not usable

    poller = HttSleeper(url, until=until, alarms=alarms)
    poller.session = my_session
    poller.run()

Not usable because here the request is prepared without the context of the Session:

response = self.session.send(self.request.prepare()

The workaround would be usable if this line was instead:

response = self.session.send(self.session.prepare_request(request))
@oliverlockwood
Copy link
Contributor Author

@kopf I'm happy to work on this once we've got #11 resolved. Again it should be fairly simple, I believe.

@kopf
Copy link
Owner

kopf commented Oct 21, 2017

Hey - is this something you need yourself? I ask as I'd be surprised if this were a common use-case. I imagine 99.9% of calls using requests are directly to the requests.get/requests.post/etc methods - I don't think I've ever come much use of the requests.session API directly. Headers (for example) can be defined once, and simply passed into the httsleep call.

I really appreciate the interest and the offer to implement it, but I'd rather try to keep the API as thin and uncomplicated as possible. (Indeed, in the next version, I'll have deprecated the status_code, json, callback etc kwargs, forcing people to just use until).

So, I'll close this for the time being, and if there's more interest, I'll consider reopening it and implementing it.

@kopf kopf closed this as completed Oct 21, 2017
@oliverlockwood
Copy link
Contributor Author

Yes, this is something we use ourselves. Direct copy and paste from our codebase:

            # TODO: simplify to this one-liner once https://github.com/kopf/httsleep/issues/13 is addressed
            #   httsleep(url, session=self._session, until=until, alarms=alarms, max_retries=15, polling_interval=4)

            poller = HttSleeper(url, until=until, alarms=alarms, max_retries=15, polling_interval=4,
                                headers=self._session.headers)
            poller.session = self._session
            poller.run()

I can understand wanting to keep the API simple, but at very least I'd love to change self.request.prepare() to self.session.prepare_request(self.request) so that users like us who do want to override the session don't have to send the session headers separately as well. Would you consider a PR along those lines, which wouldn't change the API at all?

(As it happens, passing in the session obviates our use case for issue #11, since you can define verify=False on the session itself. But you may still want to accept PR #12 for your API anyway!)

@kopf
Copy link
Owner

kopf commented Oct 26, 2017 via email

@oliverlockwood
Copy link
Contributor Author

That's a fair question. The fact is that we're making a number of HTTP calls to various endpoints on the same service, as follows:

  • a GET to check the status of something
  • a PUT or a POST depending on the outcome of the first GET, to invoke an operation
  • a polling GET (using httsleep!) to monitor the status of the operation
  • two GETs at the end to retrieve the operation's output.

Since we need to pass an auth token header and verify=False to each of these, it's cleaner for us to allocate a Session like so:

        session = requests.Session()
        session.verify = False
        session.headers.update({'Authorization': 'token=%s' % my_token})

and then pass that an everywhere, rather than put the auth header (along with any endpoint-specific headers!) and verify=False in every single call we make.

@kopf
Copy link
Owner

kopf commented Nov 5, 2017

OK, you've convinced me! :) The API you've proposed (extra session kwarg) is fine by me.

It should be made clear to the end user what happens when they do something like:

sess = requests.Session()
sess.headers.update({'some': 'headers'})
httsleep(url, until=until, alarms=alarms, session=sess, headers={'xyz': 'abc'})

@kopf kopf reopened this Nov 5, 2017
@oliverlockwood
Copy link
Contributor Author

Glad to hear it! I'll wait until the outstanding PR #12 has been merged before working on this, to avoid clashes.

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

No branches or pull requests

2 participants