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 docker engine v20.10.6 compatibility #1217

Merged
merged 1 commit into from Apr 27, 2021

Conversation

frozenbonito
Copy link
Collaborator

Description

Add Handling for IPv6 line of docker port output.

Starting with docker engine v20.10.6, docker port outputs not only IPv4 mappings but also IPv6 mappings.

e.g.:

$ docker port container
9001/tcp -> 0.0.0.0:49153
9001/tcp -> :::49153

https://docs.docker.com/engine/release-notes/#20106

Fix implicit IPv6 port-mappings not included in API response. Before docker 20.10, published ports were accessible through both IPv4 and IPv6 by default, but the API only included information about the IPv4 (0.0.0.0) mapping moby/moby#42205

The container port parsing has been broken by the IPv6 mapping line.

Motivation and Context

Failing tests on master, visible e.g. here: https://github.com/dherault/serverless-offline/runs/2438945825?check_suite_focus=true

How Has This Been Tested?

Automated tests.

Copy link
Collaborator

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Looks good overall, just one minor comment about potential improvement to readability/understandability

'port',
containerId,
])
const containerPort = containerPortBinding.split(':')[1]
for (const line of dockerPortOutput.split('\n')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be great to add a comment with explanation as to why it's parsed this way so when someone later looks at it will right away understand what's going on, otherwise it might be hard to comprehend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for comment.
I added comments to help someone understand it.

@frozenbonito frozenbonito merged commit d319174 into dherault:master Apr 27, 2021
@frozenbonito frozenbonito deleted the fix-broken-ci branch April 27, 2021 15:43
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.

None yet

2 participants