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

compat: Add subnet mask behind IP address to match Docker API #12328

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

serverwentdown
Copy link
Contributor

@serverwentdown serverwentdown commented Nov 17, 2021

What this PR does / why we need it:

Mismatch between Podman and Docker APIs when listing network containers.

How to verify it

On both Docker and Podman, create a container on a network and run:

curl -X GET --unix-socket /var/run/docker.sock http://d/networks/<id> | jq

Docker (20.10.10):
Screenshot 2021-11-17 at 20 09 49
Podman:
Screenshot 2021-11-17 at 20-11-56

Which issue(s) this PR

Fixes: None

(Sidenote: I'm working on compatibility with k3d)

Special notes for your reviewer:

@TomSweeneyRedHat
Copy link
Member

@serverwentdown thanks for the PR. Our CI requires a test to be added with a PR. Could you create one or update one? It would be good to test that the format is as you want it to be.

Changes LGTM otherwise, but I'm not sure if there are dependencies elsewhere for the current format.

@rhatdan
Copy link
Member

rhatdan commented Nov 17, 2021

@Luap99 @mheon @baude PTAL

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.

One nit, could you also add a test to https://github.com/containers/podman/tree/main/test/apiv2?

pkg/api/handlers/compat/networks.go Outdated Show resolved Hide resolved
@serverwentdown
Copy link
Contributor Author

serverwentdown commented Nov 20, 2021

@Luap99 I have added an APIv2 test but because the API tests run as rootless, it won't populate the IP address field. I should write an E2E test instead, right? Oddly, the tests started working on my local machine now.

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.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, serverwentdown

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 Nov 20, 2021
@Luap99
Copy link
Member

Luap99 commented Nov 20, 2021

Thanks @serverwentdown, unfortunately you have to rebase to the latest main branch in order to pass the CI tests.

Signed-off-by: Ambrose Chua <ambrose@hey.com>
@Luap99
Copy link
Member

Luap99 commented Nov 21, 2021

/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 Nov 21, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2021
@rhatdan
Copy link
Member

rhatdan commented Nov 23, 2021

Forcing merge to save the planet.

@rhatdan rhatdan merged commit 400ccc6 into containers:main Nov 23, 2021
@rhatdan
Copy link
Member

rhatdan commented Nov 23, 2021

Thanks @serverwentdown

@serverwentdown
Copy link
Contributor Author

serverwentdown commented Dec 23, 2021

@rhatdan @Luap99 is there any plans for this bugfix to be released in v3?

@Luap99
Copy link
Member

Luap99 commented Dec 23, 2021

@rhatdan @Luap99 is there any plans for this bugfix to be released in v3?

@mheon PTAL

@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2021

No plans for that now.

@mheon
Copy link
Member

mheon commented Dec 23, 2021

Given when this merged, I probably already attempted to backport it to v3.4.2 or v3.4.3, but the patch had cherry-pick conflicts against that branch so I moved on.

@geraldwuhoo
Copy link

Has this feature been added in v4? Been waiting on this for k3d on podman.

@Luap99
Copy link
Member

Luap99 commented Feb 20, 2022

@geraldwuhoo It should be in v4.0

@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants