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

api: remove unmapped ports from PortBindings #16818

Merged

Conversation

SoMuchForSubtlety
Copy link
Contributor

The docker API doesn't list exposed, but unmapped ports in HostConfig.PortBindings. Podman lists them with a value of null. This PR changes this behaviour to also omit them completely.

This is required for testcontainers/testcontainers-java#6158

Does this PR introduce a user-facing change?

None

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 12, 2022
@rhatdan
Copy link
Member

rhatdan commented Dec 12, 2022

For some reason the DCO check is failing.

Could you do a

git commit -a --amend -s
git push --force

To see if clears up the test.

@rhatdan
Copy link
Member

rhatdan commented Dec 12, 2022

@mheon @vrothberg @Luap99 PTAL

@Luap99
Copy link
Member

Luap99 commented Dec 12, 2022

I added this a while ago, according to #10777 docker displays null, I guess the difference is that it does not display the port when the container is not running?

@SoMuchForSubtlety
Copy link
Contributor Author

I guess the difference is that it does not display the port when the container is not running?

I don't think that that's the case. You can see the full test here. The container is running when the host config port bindings get checked.

I did not start the container in the test I added because I didn't think that it made a difference.

@Luap99
Copy link
Member

Luap99 commented Dec 13, 2022

Ok I see it now there are different port fileds, there is HostConfig.PortBindings but also NetworkSettings.Ports. The later will show exposed ports with null. So your patch is ok but it would only effect the API level.
Looking at docker they show the exposed ports under Config.ExposedPorts, I think we should match that and instead remove the exposed port from the HostConfig.PortBindings field. This should then allow the compat API to just work without extra changes.

@github-actions github-actions bot removed the kind/api-change Change to remote API; merits scrutiny label Dec 13, 2022
@SoMuchForSubtlety SoMuchForSubtlety force-pushed the api-port-bindings branch 3 times, most recently from dce2ab5 to c815e89 Compare December 13, 2022 16:47
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 13, 2022
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

code LGTM, can you squash your commits please?

@SoMuchForSubtlety
Copy link
Contributor Author

code LGTM, can you squash your commits please?

done 🙂

@mheon
Copy link
Member

mheon commented Dec 16, 2022

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@rhatdan
Copy link
Member

rhatdan commented Dec 16, 2022

/approve
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, SoMuchForSubtlety

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2022
@openshift-merge-robot openshift-merge-robot merged commit 536d3b8 into containers:main Dec 16, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 17, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants