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

stop using requests #1586

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from
Draft

stop using requests #1586

wants to merge 11 commits into from

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Apr 7, 2023

this is using Apipie/apypie#129 and some dirty tricks to make it work

don't try this at home, kids

@evgeni evgeni force-pushed the fake-requests branch 5 times, most recently from 11a111b to a2eb1af Compare April 8, 2023 10:04
@mdellweg
Copy link
Member

mdellweg commented Apr 8, 2023

Looks like a castling to me...
I'm about to take the exact opposite turn here: pulp/squeezer#106

@evgeni
Copy link
Member Author

evgeni commented Apr 8, 2023

"heh"

the ever-returning problem is the python-package-dependency-handling of ansible-collection (which is effectively non-existant)

@evgeni evgeni force-pushed the fake-requests branch 3 times, most recently from ef92e83 to 8d6f80e Compare April 8, 2023 12:26
Comment on lines -595 to -614
username=to_bytes(self._foremanapi_username),
password=to_bytes(self._foremanapi_password),
Copy link
Member Author

Choose a reason for hiding this comment

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

this was done as part of #609, but it seems ansible's Request does that for us instead.

@evgeni evgeni force-pushed the fake-requests branch 6 times, most recently from 4e591e1 to caed650 Compare April 12, 2023 08:35
plugins/module_utils/foreman_helper.py Outdated Show resolved Hide resolved
Comment on lines 145 to 147
params = kwargs.pop('params', None)
if params:
url += '?' + six.moves.urllib.parse.urlencode(params)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't Ansible's HTTP client code itself implement this?

Copy link
Member Author

Choose a reason for hiding this comment

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

So far, nope

@evgeni evgeni force-pushed the fake-requests branch 4 times, most recently from 2d7e0cd to cb59c06 Compare April 14, 2023 09:00
@evgeni evgeni force-pushed the fake-requests branch 2 times, most recently from 46e8cab to 88d7d5e Compare April 14, 2023 11:12
@evgeni
Copy link
Member Author

evgeni commented Apr 14, 2023

So this works (mostly?) and is a good ground for the discussion.

Currently open issues:

  • the url += '?' + six.moves.urllib.parse.urlencode(params) is ugly (as pointed out by Ewoud)
  • the callback is totally untested
  • it might have broken a shit ton unicode things
  • I am using a method only available in Ansible 2.10+ -- time to say good bye to 2.9? Or make that use requests?

@ekohl
Copy link
Member

ekohl commented Apr 14, 2023

the url += '?' + six.moves.urllib.parse.urlencode(params) is ugly (as pointed out by Ewoud)

Is there at least an upstream issue to support it? I think your approach to pop the parameter is safe, even if it's added, but not having to think about it is even nicer.

I am using a method only available in Ansible 2.10+ -- time to say good bye to 2.9? Or make that use requests?

That also means dropping the ability to run on EL7, right? I don't know how much that's used, but I'd consider keeping it for a bit longer.

@evgeni
Copy link
Member Author

evgeni commented Apr 14, 2023

I am using a method only available in Ansible 2.10+ -- time to say good bye to 2.9? Or make that use requests?

That also means dropping the ability to run on EL7, right? I don't know how much that's used, but I'd consider keeping it for a bit longer.

"On EL7 by using only official packages", yes. Nothing forbids to run pip installed Ansible there.

@evgeni
Copy link
Member Author

evgeni commented Apr 14, 2023

the url += '?' + six.moves.urllib.parse.urlencode(params) is ugly (as pointed out by Ewoud)

Is there at least an upstream issue to support it? I think your approach to pop the parameter is safe, even if it's added, but not having to think about it is even nicer.

I don't think upstream cares, but I can ask.
Their wrapper is not trying to be requests-compatible and just expects the url parameter to contain all parameters already. Which is totally fine.

@ekohl
Copy link
Member

ekohl commented Apr 14, 2023

If it's an issue closed as WONTFIX that at least is a good code comment why it's done.

@evgeni
Copy link
Member Author

evgeni commented Nov 7, 2024

If it's an issue closed as WONTFIX that at least is a good code comment why it's done.

ansible/ansible#84274

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.

3 participants