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

fix: tag regex should allow ports #3196

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

artificial-intelligence
Copy link
Contributor

Closes: #3195
Related: opencontainers/distribution-spec#498

notice: there is a false positive warning from ruff during make test:

docker/utils/build.py:14:33: W605 [*] Invalid escape sequence: `\.`

but it's a valid escape sequence, because it's a regex, had no idea yet how to fix this without introducing more complexity.

@artificial-intelligence
Copy link
Contributor Author

further notice:

I did fix the ruff warning about \/ because that escape sequence is not needed.

@LombardiDaniel
Copy link
Contributor

Make sure to add a simple test like:

def test_build_with_ports_on_tag(self):
        self.client.build(".", tag="registry.example.com:443/my-img:latest")

on tests/build/api_build_and_test.py. And why not use raw string literals? i.e.

r"my-regex"

instead of

"my-regex"

@milas milas self-assigned this Jan 2, 2024
@milas milas added the kind/bug label Jan 2, 2024
There are some xfails here for cases that the regex is not currently
handling. It's too strict for IPv6 domains at the moment.

Signed-off-by: Milas Bowman <[email protected]>
@milas milas requested a review from glours January 3, 2024 15:41
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 bugfix!

I addressed the lint issue (it was valid but very subtle btw: needed to use r"" strings each time for the concatenation 😵) and added some test cases from the distribution/reference codebase.

(There's some xfails on the IPv6 cases which can be addressed separately.)

@milas milas merged commit 3ec5a68 into docker:main Jan 3, 2024
9 of 10 checks passed
@SecT0uch
Copy link

When can we expect a release including this fix ?

@artificial-intelligence
Copy link
Contributor Author

Thanks @milas for taking over (I was first on PTO, then this issue was drowned in my inbox due to other issues)!

a new release would be highly appreciated for our downstream usage!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7.0: Docker Tag Parsing Issue with Registry Ports
4 participants