Make port arg optional in `docker compose port` #11874

crazybolillo · 2024-06-01T18:58:02Z

What I did

This comand no longer needs a mandatory port. If no port is provided it will instead list all published ports. This behavior falls in line with the behavior provided by docker port.

Related issue
Closes #11859.

glours

Thanks for your contribution, I have some small suggestions to improve your changes

glours · 2024-06-03T06:22:17Z

cmd/compose/port.go

-		Short: "Print the public port for a port binding",
-		Args:  cobra.MinimumNArgs(2),
+		Use:   "port [OPTIONS] SERVICE [PRIVATE_PORT]",
+		Short: "List port mappings or a specific mapping for the service",


Suggested change

Short: "List port mappings or a specific mapping for the service",

Short: "List port mappings or print the public port for a specific mapping for the service",

this is just the same help used in docker/cli

cmd/compose/port.go

crazybolillo · 2024-06-04T02:05:14Z

Thanks for the review, I have taken care of your observations.

glours

LGTM

ndeloof · 2024-06-11T05:36:57Z

cmd/compose/port.go

 		PreRunE: Adapt(func(ctx context.Context, args []string) error {
+			if len(args) == 1 {
+				opts.list = true


incompatibility with --index should be tested

Should --index be prohibited if no specific port was provided?

ndeloof · 2024-06-11T05:38:25Z

cmd/compose/port.go

 func runPort(ctx context.Context, dockerCli command.Cli, backend api.Service, opts portOptions, service string) error {
 	projectName, err := opts.toProjectName(ctx, dockerCli)
 	if err != nil {
 		return err
 	}
+
+	if opts.list {
+		summaries, err := backend.Ps(ctx, projectName, api.PsOptions{Services: []string{service}})


better update backend.Port so it can manage multiple services. Could return PortPublishers

ndeloof · 2024-06-11T05:39:25Z

cmd/compose/port.go

+		if err != nil {
+			return err
+		}
+		if len(summaries) <= opts.index {


[]ContainerSummary index is unrelated to replica index in service set by --index.

ndeloof · 2024-06-11T05:59:11Z

cmd/compose/port.go

+		}
+
+		var format string
+		if strings.Contains(publisher.URL, ":") {


would make sense to me PortPublisher has a String() method to handle this, aligned with docker/cli

ndeloof · 2024-06-11T06:02:15Z

while not strictly required, I think we should use this opportunity while revisiting this command to add support for app_protocol and port name (https://github.com/compose-spec/compose-spec/blob/master/05-services.md#long-syntax-3)

crazybolillo · 2024-06-12T04:58:46Z

while not strictly required, I think we should use this opportunity while revisiting this command to add support for app_protocol and port name (https://github.com/compose-spec/compose-spec/blob/master/05-services.md#long-syntax-3)

Wow, did not even know those existed. How do you envision it? Allow filtering? Or just displaying the name and app_protocol if they exist for the given port?

This comand no longer needs a mandatory port. If no port is provided it will instead list all published ports. This behavior falls in line with the behavior provided by `docker port`. Closes docker#11859. Signed-off-by: Antonio Aguilar <[email protected]>

crazybolillo · 2024-06-12T06:11:14Z

Went ahead and changed backend.Port so it returns a slice of publishers. That should fix other issues like dealing with --index. Since I was modifying PortPublisher I went ahead and also changed its port members to be uint16 so it falls in line with types.Port

I think the only thing left is the question about --index incompatibility, and support for app_protocol and port name,

ndeloof · 2024-06-12T06:19:57Z

pkg/compose/port.go

@@ -26,18 +26,40 @@ import (
 	moby "github.com/docker/docker/api/types"
 )

-func (s *composeService) Port(ctx context.Context, projectName string, service string, port uint16, options api.PortOptions) (string, int, error) {
+func (s *composeService) Port(ctx context.Context, projectName string, service string, port uint16, options api.PortOptions) (api.PortPublishers, error) {


as signature changes, and breaks compatibility, makes sense to also rename Ports(plural)

Makes sense to denote information about more than one port can be returned. However wouldn't it be weird since it breaks the pattern of backend methods having the same name as their CLI command? Also if we were to rename the method, I think we should also rename the file to ports.go?

@ndeloof Sorry to bother but do you have any thoughts on my comments? Should we still perform the renaming?

ndeloof · 2024-06-12T06:21:55Z

--index is used to select service replica, already implemented in backend.Port calling getSpecifiedContainer
Adding support for app_protocol would not be that simple, as such metadata only exists in compose model, not in actual resources. Let's keep this for a future improvement

crazybolillo force-pushed the 11859-crazybolillo branch 2 times, most recently from 9670fd8 to c87db39 Compare June 1, 2024 19:05

glours requested changes Jun 3, 2024

View reviewed changes

crazybolillo force-pushed the 11859-crazybolillo branch 2 times, most recently from 357c13b to 21c0a70 Compare June 4, 2024 02:03

crazybolillo requested a review from glours June 5, 2024 04:45

glours approved these changes Jun 5, 2024

View reviewed changes

glours requested a review from ndeloof June 5, 2024 07:30

ndeloof requested changes Jun 11, 2024

View reviewed changes

ndeloof reviewed Jun 11, 2024

View reviewed changes

crazybolillo force-pushed the 11859-crazybolillo branch from 21c0a70 to 96514e4 Compare June 12, 2024 06:07

ndeloof approved these changes Jun 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make port arg optional in `docker compose port` #11874

Make port arg optional in `docker compose port` #11874

crazybolillo commented Jun 1, 2024

glours left a comment

glours Jun 3, 2024

ndeloof Jun 11, 2024

crazybolillo commented Jun 4, 2024

glours left a comment

ndeloof Jun 11, 2024

crazybolillo Jun 12, 2024

ndeloof Jun 11, 2024 •

edited

Loading

ndeloof Jun 11, 2024

ndeloof Jun 11, 2024

ndeloof commented Jun 11, 2024

crazybolillo commented Jun 12, 2024

crazybolillo commented Jun 12, 2024

ndeloof Jun 12, 2024

crazybolillo Jun 12, 2024

crazybolillo Jun 17, 2024

ndeloof commented Jun 12, 2024

	Short: "List port mappings or a specific mapping for the service",
	Short: "List port mappings or print the public port for a specific mapping for the service",

Make port arg optional in docker compose port #11874

Are you sure you want to change the base?

Make port arg optional in docker compose port #11874

Conversation

crazybolillo commented Jun 1, 2024

glours left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazybolillo commented Jun 4, 2024

glours left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof commented Jun 11, 2024

crazybolillo commented Jun 12, 2024

crazybolillo commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndeloof commented Jun 12, 2024

Make port arg optional in `docker compose port` #11874

Make port arg optional in `docker compose port` #11874

ndeloof Jun 11, 2024 •

edited

Loading