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.

Sign up for GitHub

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 --host and -H as equivalent options to --url #14947

Merged
merged 1 commit into from
Jul 18, 2022
+26 −5
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cmd/podman/registry/remote.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ func IsRemote() bool {
fs.SetInterspersed(false)
fs.BoolVarP(&remoteFromCLI.Value, "remote", "r", remote, "")
connectionFlagName := "connection"
ignoredConnection := ""
fs.StringVarP(&ignoredConnection, connectionFlagName, "c", "", "")
fs.StringP(connectionFlagName, "c", "", "")
hostFlagName := "host"
fs.StringP(hostFlagName, "H", "", "")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

urlFlagName := "url"
ignoredURL := ""
fs.StringVar(&ignoredURL, urlFlagName, "", "")
fs.String(urlFlagName, "", "")

// The shell completion logic will call a command called "__complete" or "__completeNoDesc"
// This command will always be the second argument
Expand All @@ -46,7 +46,7 @@ func IsRemote() bool {
}
_ = fs.Parse(os.Args[start:])
// --connection or --url implies --remote
remoteFromCLI.Value = remoteFromCLI.Value || fs.Changed(connectionFlagName) || fs.Changed(urlFlagName)
remoteFromCLI.Value = remoteFromCLI.Value || fs.Changed(connectionFlagName) || fs.Changed(urlFlagName) || fs.Changed(hostFlagName)
})
return podmanOptions.EngineMode == entities.TunnelMode || remoteFromCLI.Value
}
2 changes: 2 additions & 0 deletions cmd/podman/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,8 @@ func rootFlags(cmd *cobra.Command, opts *entities.PodmanConfig) {
urlFlagName := "url"
lFlags.StringVar(&opts.URI, urlFlagName, uri, "URL to access Podman service (CONTAINER_HOST)")
_ = cmd.RegisterFlagCompletionFunc(urlFlagName, completion.AutocompleteDefault)
lFlags.StringVarP(&opts.URI, "host", "H", uri, "Used for Docker compatibility")
_ = lFlags.MarkHidden("host")

// Context option added just for compatibility with DockerCLI.
lFlags.String("context", "default", "Name of the context to use to connect to the daemon (This flag is a NOOP and provided solely for scripting compatibility.)")
Expand Down
19 changes: 19 additions & 0 deletions test/system/250-systemd.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,23 @@ EOF
run_podman rm -f -t 0 $cname
}

@test "podman-system-service containers --host" {
skip_if_remote "N/A under podman-remote"

SERVICE_NAME=podman-service-$(random_string)
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

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"
Comment on lines +413 to +420
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes and info fails without socket.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}
# vim: filetype=sh