Add --host and -H as equivalent options to --url #14947

rhatdan · 2022-07-15T10:22:11Z

Docker supports -H and --host for specify the listening socket. Podman
should support them also in order to match the CLI.

These will not be documented since Podman defaults to using the
--connection option.

Signed-off-by: Daniel J Walsh [email protected]

Does this PR introduce a user-facing change?

Yes

podman now supports the -H and --host options as equivalent to --url, for Docker compatibility.

openshift-ci · 2022-07-15T10:22:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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:

~~OWNERS~~ [rhatdan]

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

rhatdan · 2022-07-15T10:22:20Z

@Luap99 PTAL

rhatdan · 2022-07-15T11:01:35Z

Addresses: #14917

Luap99 · 2022-07-15T11:09:59Z

--connection accepts a name not a url, I think docker --host needs a full url so this should map to --url

rhatdan · 2022-07-15T12:37:57Z

@Luap99 Your right, thanks.

vrothberg · 2022-07-18T07:26:00Z

test/system/250-systemd.bats

+    systemd-run --unit=$SERVICE_NAME $PODMAN system service $URL --time=0
+    wait_for_port 127.0.0.1 $port
+
+    # Start a long-running container.


The comment is confusing as the containers are short lived.

edsantiago · 2022-07-18T11:37:07Z

APIv2 failure is the deadlock bug (#14929). I'd restart it, except, I think there's something still broken because tests fail on my laptop when I run them manually:

Error: unknown flag: --H

I'll assume that should be a trivial one for you to fix (remove one of the dashes).

What would you think of adding:

diff --git a/test/system/250-systemd.bats b/test/system/250-systemd.bats
index 3afd760c3..8e6c7dadc 100644
--- a/test/system/250-systemd.bats
+++ b/test/system/250-systemd.bats
@@ -414,6 +414,10 @@ EOF
     run_podman --H $URL run --rm $IMAGE true
 
     systemctl stop $SERVICE_NAME
+
+    # Make sure the option is actually connecting
+    run_podman 125 --host $URL run --rm $IMAGE true
+    assert "$output" =~ "Cannot connect to Podman.*connection refused"
 }
 
 # vim: filetype=sh

mheon · 2022-07-18T14:21:05Z

LGTM

Docker supports -H and --host for specify the listening socket. Podman should support them also in order to match the CLI. These will not be documented since Podman defaults to using the --url option. Signed-off-by: Daniel J Walsh <[email protected]>

Luap99 · 2022-07-18T14:43:15Z

test/system/250-systemd.bats

+    run_podman --host $URL run --rm $IMAGE true
+    run_podman -H $URL run --rm $IMAGE true
+
+    systemctl stop $SERVICE_NAME
+
+    # Make sure the option is actually connecting
+    run_podman 125 --host $URL run --rm $IMAGE true
+    assert "$output" =~ "Cannot connect to Podman.*connection refused"


Why do we run containers? Could we just call info? That way we can also check for remote.

The goal here is to make sure the --host $URL is actually doing something including failing if the socket does not exist.

Yes and info fails without socket.

And you think info would be faster? I just used what was suggested by Ed.

I would say yes but it is not really important enough to argue over it. I am ok to merge this.

I just made my suggestion because I didn't think podman info would show the socket. (There's a bug open for that, or maybe it's been fixed, I don't know). Anyway, if podman info shows the socket name, that is a much better solution than running containers.

Hey, the info bug is fixed. Suggestion:

@test "podman-system-service containers --host" { skip_if_remote "N/A under podman-remote" SERVICE_NAME=podman-service-$(random_string) # (Used only in teardown if the test fails) touch $UNIT_FILE port=$(random_free_port) URL=tcp://127.0.0.1:$port systemd-run --unit=$SERVICE_NAME $PODMAN system service $URL --time=0 wait_for_port 127.0.0.1 $port for opt in --host -H; do run_podman $opt $URL info --format '{{.Host.RemoteSocket.Path}}' is "$output" "$URL" "RemoteSocket.Path using $opt" done systemctl stop $SERVICE_NAME rm -f $UNIT_FILE }

This also adds an important cleanup step to prevent leaving the podman server behind on failure.

And yes, I realize that the previous test (from which this was copied) also has a missing cleanup. That's now on my TODO list to fix. Unless you'd like to fix it.

Lets get this merged and then fix the test, unless something else fails.

OK, I'll fix it in a cleanup PR later

edsantiago · 2022-07-18T16:23:30Z

LGTM but after thinking about it a little, I'm pretty sure this test should not be skipped under podman-remote: that's exactly where this flag is intended.

Unfortunately, making this run under podman-remote is much more complicated than I expected. I will add that to my TODO list.

TomSweeneyRedHat · 2022-07-22T20:37:27Z

cmd/podman/registry/remote.go

-		fs.StringVarP(&ignoredConnection, connectionFlagName, "c", "", "")
+		fs.StringP(connectionFlagName, "c", "", "")
+		hostFlagName := "host"
+		fs.StringP(hostFlagName, "H", "", "")


I'm not a fan of having an upper cased single character option when most (all?) of our others are lower case. But Docker compatibility, ugh.

I assume that -h had already been taken by --help.

openshift-ci bot added the release-note label Jul 15, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2022

rhatdan force-pushed the root branch from ab38854 to 3196b54 Compare July 15, 2022 12:36

rhatdan changed the title ~~Add --host and -H as equivalent options to --connection~~ Add --host and -H as equivalent options to --url Jul 15, 2022

rhatdan force-pushed the root branch 2 times, most recently from 72dab61 to 53a2b8f Compare July 16, 2022 10:34

vrothberg reviewed Jul 18, 2022

View reviewed changes

rhatdan force-pushed the root branch from 53a2b8f to 1900e36 Compare July 18, 2022 10:05

rhatdan force-pushed the root branch from 1900e36 to c2ef00e Compare July 18, 2022 13:06

rhatdan force-pushed the root branch from c2ef00e to 18c0c19 Compare July 18, 2022 14:41

Luap99 reviewed Jul 18, 2022

View reviewed changes

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022

openshift-ci bot merged commit d890c4d into containers:main Jul 18, 2022

TomSweeneyRedHat reviewed Jul 22, 2022

View reviewed changes

rhatdan deleted the root branch December 1, 2022 22:01

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 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --host and -H as equivalent options to --url #14947

Add --host and -H as equivalent options to --url #14947

rhatdan commented Jul 15, 2022 •

edited

Loading

openshift-ci bot commented Jul 15, 2022

rhatdan commented Jul 15, 2022

rhatdan commented Jul 15, 2022

Luap99 commented Jul 15, 2022

rhatdan commented Jul 15, 2022

vrothberg Jul 18, 2022

edsantiago commented Jul 18, 2022 •

edited

Loading

mheon commented Jul 18, 2022

Luap99 Jul 18, 2022

rhatdan Jul 18, 2022

Luap99 Jul 18, 2022

rhatdan Jul 18, 2022

Luap99 Jul 18, 2022

edsantiago Jul 18, 2022

edsantiago Jul 18, 2022

edsantiago Jul 18, 2022

rhatdan Jul 18, 2022

edsantiago Jul 18, 2022

edsantiago commented Jul 18, 2022

TomSweeneyRedHat Jul 22, 2022

vrothberg Jul 25, 2022

Add --host and -H as equivalent options to --url #14947

Add --host and -H as equivalent options to --url #14947

Conversation

rhatdan commented Jul 15, 2022 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Jul 15, 2022

rhatdan commented Jul 15, 2022

rhatdan commented Jul 15, 2022

Luap99 commented Jul 15, 2022

rhatdan commented Jul 15, 2022

Choose a reason for hiding this comment

edsantiago commented Jul 18, 2022 • edited Loading

mheon commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Jul 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jul 15, 2022 •

edited

Loading

edsantiago commented Jul 18, 2022 •

edited

Loading