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

Return 12 character short_ids #2862

Merged
merged 5 commits into from
Jul 29, 2022
Merged

Return 12 character short_ids #2862

merged 5 commits into from
Jul 29, 2022

Conversation

benfasoli
Copy link
Contributor

Hi all 👋 Thanks for the work here - I've found it really helpful for several projects.

I'd like to discuss changing the behavior of Model.short_id from returning a 10 character string to a 12 character string which aligns more closely with the docker cli. It looks like this has been requested previously by #1491 and #2660.

For example (from the docs), running docker ps without explicitly setting --no-trunc:

$ docker ps

CONTAINER ID        IMAGE                        COMMAND                CREATED              STATUS              PORTS               NAMES
4c01db0b339c        ubuntu:12.04                 bash                   17 seconds ago       Up 16 seconds       3300-3310/tcp       webapp
d7886598dbe2        crosbymichael/redis:latest   /redis-server --dir    33 minutes ago       Up 33 minutes       6379/tcp            redis,webapp/db

It looks like 12 character ids were once the intended behavior - the object reprs in the docker-py documentation from #1186 show 12 digit ids:

>>> client.containers.list()
[<Container '45e6d2de7c54'>, <Container 'db18e4f20eaa'>, ...]

The specific issue I'm having is that the 12 character container ID is used by docker as a container's default hostname. Here's a minimal example of a container trying to connect to itself by hostname:

import docker


client = docker.from_env()


nginx_container = client.containers.run("nginx:latest", detach=True, auto_remove=True)


# Networking with 64 character id as hostname will fail.
exit_code, response = nginx_container.exec_run(f"curl {nginx_container.id}")
assert exit_code == 6
assert b"Could not resolve host:" in response

# Networking with 10 character id as hostname will fail.
exit_code, response = nginx_container.exec_run(f"curl {nginx_container.short_id}")
assert exit_code == 6
assert b"Could not resolve host:" in response

# Networking with 12 character id as hostname will succeed 🚀
exit_code, response = nginx_container.exec_run(f"curl {nginx_container.id[:12]}")
assert exit_code == 0, response
assert b"Welcome to nginx!" in response


nginx_container.stop()

API requests treat the id as a variable width prefix and returns valid responses as long as the prefix doesn't match multiple resources. So I think using 12 characters shouldn't break backwards compatibility for code that currently uses the 10 character short_id.

Here's a first pass at the implementation.

  1. Return a 12 character short_id for classes that inherit from docker.models.resource.Model and a 19 character short_id for Image ids prefixed by sha256:.
  2. Expand the fake_api ids to 64 characters.
  3. Modify ImageTest.test_short_id to expect a 12 character response.
  4. Add ContainerTest.test_short_id to expect a 12 character response.

This currently fail flake8's E501 (line > 79 characters) requirement in a few places but I left the formatting as is for now since there are existing docstrings which fail E501 and it made the changes more readable. Happy to tweak to your preference.

Closes #1491
Closes #2660

Signed-off-by: Ben Fasoli <benfasoli@gmail.com>
@ghost
Copy link

ghost commented Jul 19, 2022

Up, that doesn't seems like a big deal but it would improve the python API. I was thinking about creating a database storing container stats but I couldn't choose a coherent length for the short ID because of this python/docker cli discrepancy.
It is always possible to use the full length ID, but it is almost never used because it's non user-friendly.

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement and tests! Will get this included in the upcoming 6.0.0 release 🙂

@milas milas merged commit 23cf16f into docker:main Jul 29, 2022
@milas milas added this to the 6.0.0 milestone Jul 30, 2022
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.

Change container.short_id to 12 characters container id only 10 characters
2 participants