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

Remove paramiko upper-bound #1005

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Remove paramiko upper-bound #1005

merged 3 commits into from
Jan 10, 2024

Conversation

gboutry
Copy link
Contributor

@gboutry gboutry commented Jan 9, 2024

Description

This change removes paramiko upper bound, allow using paramiko >3.0

Fixes: #1004

QA Steps

<Commands / tests / steps to run to verify that the change works:>

tox -e py3 -- tests/unit/...
tox -e integration -- tests/integration/test_model.py::test_add_manual_machine_ssh
tox -e integration -- tests/integration/test_model.py::test_add_manual_machine_ssh_root

All CI tests need to pass.

Notes & Discussion

I've had to pin urllib3 to urllib3==1.25.7 to have the test pass. but the error was from pylxd failing with TypeError: HTTPConnection.request() got an unexpected keyword argument 'chunked'. Not sure this is related to paramiko change, since paramiko does not depend on urllib3.

File "/home/gboutry/python-libjuju/tests/integration/test_model.py", line 596, in test_add_manual_machine_ssh
    await add_manual_machine_ssh(event_loop, is_root=False)
  File "/home/gboutry/python-libjuju/tests/integration/test_model.py", line 463, in add_manual_machine_ssh
    client = pylxd.Client()
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/pylxd/client.py", line 383, in __init__
    response = self.api.get()
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/pylxd/client.py", line 192, in get
    response = self.session.get(self._api_endpoint, *args, **kwargs)
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/requests/sessions.py", line 602, in get
    return self.request("GET", url, **kwargs)
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/requests/sessions.py", line 589, in request
    resp = self.send(prep, **send_kwargs)
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/requests/sessions.py", line 703, in send
    r = adapter.send(request, **kwargs)
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/requests/adapters.py", line 486, in send
    resp = conn.urlopen(
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/urllib3/connectionpool.py", line 790, in urlopen
    response = self._make_request(
  File "/home/gboutry/python-libjuju/.tox/py3/lib/python3.10/site-packages/urllib3/connectionpool.py", line 496, in _make_request
    conn.request(
TypeError: HTTPConnection.request() got an unexpected keyword argument 'chunked'

Remove paramiko upper-bound to allow using paramiko version >3.0.
@jujubot
Copy link
Contributor

jujubot commented Jan 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Jan 9, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

I've had to pin urllib3 to urllib3==1.25.7 to have the test pass.

This is because the transitive dependency coming from pylxd. So, pylxd wants urllib3 < 2, but additionally it depends on requests library, and requests in turn wants urllib3>=1.21.1,<3 , but pip's not gonna complain because to satisfy pylxd's initial constraint it chooses the urllib==1.26.somthing, however, tox doesn't care about any of these unless you tell it explicitly. I tried swapping these lines to install pylxd first and I'm surprised that pip didn't seem to handle the dependency constraint (it chose the urllib>2).

Anyways I'm fine with the paramiko version change. When I fix the urllib3 versions on my local the tests seem to be passing. Let's just remove the urllib3 lines from the tox.ini and let the pip install pylxd install it as a dependency. That one already satisfies the requests library. So it'll be fine until the requests raises the floor for the urllib3 to >= 2. Then we might need to deal with another macaroonbakery situtation with the pylxd guys :)

Thanks for the PR <3

PyLXD needs a specific urllib3 version range, remove urllib3
installation and let pick choose a version compatible with PyLXD.
@cderici
Copy link
Contributor

cderici commented Jan 10, 2024

/build

@gboutry
Copy link
Contributor Author

gboutry commented Jan 10, 2024

I think we'll need to put an upper limit on urllib3

@cderici
Copy link
Contributor

cderici commented Jan 10, 2024

Yeah I'm not sure why we keep picking urllib3=2.1.0, let's see what it'll pick with the upper bound in place

Copy link
Contributor

@cderici cderici left a comment

Choose a reason for hiding this comment

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

Yeah that seems to have done it 👍

@cderici
Copy link
Contributor

cderici commented Jan 10, 2024

/build

@cderici
Copy link
Contributor

cderici commented Jan 10, 2024

/merge

@jujubot jujubot merged commit 62d64d0 into juju:master Jan 10, 2024
6 of 8 checks passed
@gboutry gboutry deleted the fix/paramiko-bound branch January 11, 2024 08:32
@cderici cderici mentioned this pull request Feb 8, 2024
jujubot added a commit that referenced this pull request Feb 13, 2024
#1024

## What's Changed
* Remove paramiko upper-bound by @gboutry in #1005
* Remove explicit passing of event_loop into tests by @cderici in #1006
* chore: remove the upper restrictions on the websockets dependency by @tonyandrewmeyer in #1007
* Target ceiling version by @cderici in #1008
* Make it easier to run tests using `make` by @cderici in #1012
* Avoid installing signal handlers to the event loop by @cderici in #1014
* feat: remove app block until done by @yanksyoon in #1017
* feat: remove app timeout by @yanksyoon in #1018
* Forward port latest changes from 2.9 onto 3.x by @cderici in #1022

#### Notes & Discussion

JUJU-5414
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.

Remove upper-bound on paramiko
3 participants