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

Add compose port command #1628

Merged
merged 1 commit into from
Dec 9, 2022
Merged

Conversation

djdongjin
Copy link
Member

https://docs.docker.com/engine/reference/commandline/compose_port/

It has 2 changes:

  1. Add the command
  2. Refactor and move the task logic (print port) to a new pkg (containerutil).
    1. This gives better separation between command/arg logic and main task logic.
    2. Later on maybe we can move more main task logic into this package (or other pkg), so that they can be reused by other commands (e.g. relevant compose command).

(Please lmk if 2 doesn't look good, or any suggestion if there is better place for this kind of functions (e.g., finish a task given a container).

Also there is a incompatible behavior compared to docker compose (docker itself has incompatible behavior in v1 and v2), when a given port/protocol doesn't exist:

  1. nerdctl (in this PR) fails the command and prints an error.
  2. docker compose v1 prints nothing (\n).
  3. docker compose v2 prints :0\n.

Please lmk if this is okay.

Signed-off-by: Jin Dong [email protected]

// when no port mapping is found, docker compose v1 prints `\n` while v2 prints `:0\n`
// both v1 and v2 have exit code 0 (succeess)
// nerdctl compose will fail with error (no public port fouond).
testutil.DockerIncompatible(t)
Copy link
Member

@AkihiroSuda AkihiroSuda Dec 9, 2022

Choose a reason for hiding this comment

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

Can we split the incompatible part to another (sub)test?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

Signed-off-by: Jin Dong <[email protected]>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit a109452 into containerd:main Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants