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.

Sign up for GitHub

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

Jump to bottom

Add requests.Session like class #37622

Merged
merged 7 commits into from Jun 1, 2018
Merged

Add requests.Session like class #37622

merged 7 commits into from Jun 1, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Mar 19, 2018

SUMMARY

Adds a simple class that adds session like functionality. Uses a common cookiejar across requests, and has "default" value support across requests.

from ansible.module_utils.urls import Request
r = Request(url_username='user', url_password='passwd')
r.open('GET', 'http://httpbin.org/basic-auth/user/passwd').read()
from ansible.module_utils.urls import Request
r = Request(headers=dict(foo='bar'))
r.open('GET', 'http://httpbin.org/get', headers=dict(baz='qux')).read()

Needs tests and stuff

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/urls.py

ANSIBLE VERSION
2.7
ADDITIONAL INFORMATION

@sivel sivel requested a review from bcoca March 19, 2018 20:28
@sivel sivel changed the title Adds requests.Session like class [WIP] Adds requests.Session like class Mar 19, 2018
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Mar 19, 2018
@sivel
Copy link
Member Author

sivel commented Mar 19, 2018

Cleaner diff: https://github.com/ansible/ansible/pull/37622/files?w=1

@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Mar 20, 2018
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 28, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 6, 2018
@sivel
Copy link
Member Author

sivel commented Apr 9, 2018

As is, this should now pass all current tests. However, I need more tests.

@ansibot ansibot added test This PR relates to tests. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 9, 2018
@ansibot

This comment has been minimized.

Sign in to view
@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 9, 2018
@ansibot
Copy link
Contributor

ansibot commented Apr 10, 2018

cc @mattclay
click here for bot help

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 10, 2018
@sivel sivel changed the title [WIP] Adds requests.Session like class Adds requests.Session like class Apr 10, 2018
@sivel
Copy link
Member Author

sivel commented Apr 10, 2018

Removed WIP. This has complete tests.

@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Apr 10, 2018
@sivel sivel changed the title Adds requests.Session like class Add requests.Session like class Apr 10, 2018
@sivel sivel mentioned this pull request Apr 10, 2018
Closed
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 18, 2018
@ansibot ansibot added the affects_2.6 This issue/PR affects Ansible v2.6 label May 20, 2018
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 28, 2018
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 29, 2018
@sivel
Copy link
Member Author

sivel commented Jun 1, 2018

@bcoca are you good with this feature making it in for 2.7?

@bcoca
Copy link
Member

bcoca commented Jun 1, 2018

fine with me, i was thinking more of a 'sessions' vs emulating requests, but that is just bikeshed

@sivel sivel merged commit d4930e6 into ansible:devel Jun 1, 2018
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
@sivel
Add requests.Session like class (ansible#37622)
4b0adfe 
* Adds requests.Session like class

* py2 syntax fix

* Add a few examples to the Request docstrings

* Add helper methods and docs

* Fix test failures

* Switch tests to test Request instead of open_url, add simple open_url test to validate funcitonality

* Fix filename in replace-urlopen code smell test
AmShaegar13
AmShaegar13 reviewed Jul 13, 2018
View reviewed changes
lib/ansible/module_utils/urls.py

Does not require the module environment
'''
method = method or 'GET'
Copy link

Choose a reason for hiding this comment

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

@sivel @bcoca Breaking change incoming here! From the documentation of urllib.request:

method should be a string that indicates the HTTP request method that will be used (e.g. 'HEAD'). If provided, its value is stored in the method attribute and is used by get_method(). The default is 'GET' if data is None or 'POST' otherwise.

This PR changes the default to always be 'GET' which breaks at least the rocketchat notification module.

Copy link
Contributor

Choose a reason for hiding this comment

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

This also breaks the rollbar_deployment module, see pull request #42876

This was referenced Jul 13, 2018
Closed
Merged
@abadger abadger mentioned this pull request Jul 23, 2018
Merged
@ansible ansible locked and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ecoutu ecoutu ecoutu left review comments

@AmShaegar13 AmShaegar13 AmShaegar13 left review comments

@bcoca bcoca Awaiting requested review from bcoca

Assignees
No one assigned
Labels
affects_2.6 This issue/PR affects Ansible v2.6 feature This issue/PR relates to a feature request. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sivel @ansibot @bcoca @ecoutu @AmShaegar13