docker-py icon indicating copy to clipboard operation
docker-py copied to clipboard

Bump default API version to 1.45 (Moby 26.0/26.1)

Open thaJeztah opened this issue 1 year ago • 16 comments

  • similar to https://github.com/docker/docker-py/pull/3199

Update API version to the latest maintained release.

thaJeztah avatar May 22 '24 13:05 thaJeztah

We still need to look at raising the minimum version, but I guess that can wait;

We should also start looking at raising this minimum, but I don't know if there's code specific in docker-py that handles differences between versions @milas ?

We're starting to deprecate old API versions, and the initial step is to disable (by default) API versions < 1.24. API 1.24 is the last version of the API before introduction of API version negotiation, so that's the version the CLI falls back to if it's unable to negotiate.

thaJeztah avatar May 22 '24 13:05 thaJeztah

oh! some failure; let me see if it's related

thaJeztah avatar May 22 '24 13:05 thaJeztah

Looks like it may be, and ISTR @robmry looked at some of that, but now wondering if it needs to be API-version aware (or the test rewritten to take both scenarios into account);

=================================== FAILURES ===================================
___________ ContainerCollectionTest.test_run_with_networking_config ____________
tests/integration/models_containers_test.py:136: in test_run_with_networking_config
    assert attrs['NetworkSettings']['Networks'][net_name]['Aliases'] \
E   AssertionError: assert ['hello'] == ['hello', '90b209030388']
E     Right contains one more item: '90b209030388'
E     Use -v to get more diff
_ ContainerCollectionTest.test_run_with_networking_config_only_undeclared_network _
tests/integration/models_containers_test.py:196: in test_run_with_networking_config_only_undeclared_network
    assert attrs['NetworkSettings']['Networks'][net_name]['Aliases'] == [attrs["Id"][:12]]
E   AssertionError: assert None == ['0ed603731bb5']
=========================== short test summary info ============================

thaJeztah avatar May 22 '24 13:05 thaJeztah

☝️ I think this PR was related to that, but looks like that one wasn't merged; maybe it got included in another PR?

  • https://github.com/docker/docker-py/pull/3233

thaJeztah avatar May 22 '24 13:05 thaJeztah

I remember seeing # In API version 1.45, the short-id will be removed.. Looks like that didn't happen? 🤔

krissetto avatar May 22 '24 13:05 krissetto

Wondering if TEST_API_VERSION is passed through everywhere; I see at least one dockerfiles using DOCKER_API_VERSION (not TEST_.....) 🤔

thaJeztah avatar May 22 '24 13:05 thaJeztah

I remember seeing # In API version 1.45, the short-id will be removed.. Looks like that didn't happen? 🤔

Oh; maybe it is removed, and that's the failure? AssertionError: assert None == ['0ed603731bb5']

But the test doesn't have a conditional on API version, so removing it would mean it'd fail on older API versions? 🤔

thaJeztah avatar May 22 '24 13:05 thaJeztah

I remember seeing # In API version 1.45, the short-id will be removed.. Looks like that didn't happen? 🤔

Oh; maybe it is removed, and that's the failure? AssertionError: assert None == ['0ed603731bb5']

But the test doesn't have a conditional on API version, so removing it would mean it'd fail on older API versions? 🤔

nah, the test seems to be assert ['hello'] == ['hello', '90b209030388'], indicating it's still present (?)

krissetto avatar May 22 '24 13:05 krissetto

i don't see it when running a docker inspect locally though, with engine and client at 26.0.1

krissetto avatar May 22 '24 13:05 krissetto

Tried updating the test, but now getting

=================================== FAILURES ===================================
___________ ContainerCollectionTest.test_run_with_networking_config ____________
tests/integration/models_containers_test.py:136: in test_run_with_networking_config
    assert (attrs['NetworkSettings']['Networks'][net_name]['Aliases']
E   AssertionError: assert ['hello'] is None
_ ContainerCollectionTest.test_run_with_networking_config_only_undeclared_network _
tests/integration/models_containers_test.py:202: in test_run_with_networking_config_only_undeclared_network
    assert attrs['NetworkSettings']['Networks'][net_name]['DNSNames'] \
E   AssertionError: assert None == ['hello', 'e293f5effd7e']

So, looks like that wasn't right 🙈

thaJeztah avatar May 22 '24 13:05 thaJeztah

Getting closer; only one failure remaining;

___________ ContainerCollectionTest.test_run_with_networking_config ____________
tests/integration/models_containers_test.py:136: in test_run_with_networking_config
    assert (attrs['NetworkSettings']['Networks'][net_name]['Aliases']
E   AssertionError: assert ['hello'] is None

thaJeztah avatar May 22 '24 14:05 thaJeztah

what is our expectation here? meaning, are the tests "wrong" or is the engine's behavior slightly out-of-spec from what we were expecting?

krissetto avatar May 22 '24 14:05 krissetto

Trying to figure that out honestly; thought let me update the test to see what it actually sees, then check with @robmry and @akerouanton if this is the expected result here. 😂

thaJeztah avatar May 22 '24 15:05 thaJeztah

LOL, and now it's back to;

___________ ContainerCollectionTest.test_run_with_networking_config ____________
tests/integration/models_containers_test.py:138: in test_run_with_networking_config
    assert attrs['NetworkSettings']['Networks'][net_name]['DNSNames'] \
E   AssertionError: assert None == ['hello', '34fe1fead4b8']

Is there some race condition? Or did I just broke it again? (the "expected" vs "actual" part is also confusing me for sure here 😂 )

thaJeztah avatar May 22 '24 16:05 thaJeztah

Unfortunately, I don't actually know anything about version negotiation in the Python code.

However, I would imagine it's mostly only ever been exercised in the other direction, e.g. to avoid sending a newly added field/query param to an older daemon.

The test logic might also be more for broad test suite compatibility rather than comprehensive matrix coverage.

(That is, I'm suggesting it's not unreasonable to be somewhat suspicious of both the library code & tests in this regard.)

milas avatar May 22 '24 16:05 milas

Oh! Looks like it's green now! Guess it's time for me to squash commits, and to discuss if these changes are all correct with the networking folks 🥹

thaJeztah avatar May 22 '24 17:05 thaJeztah