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

docs: Add shell alias and trafe-offs explaination #703

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

travier
Copy link
Member

@travier travier commented Dec 3, 2021

@travier
Copy link
Member Author

travier commented Dec 8, 2021

Could also be extended like coreos/butane#296

@lucab
Copy link
Contributor

lucab commented Dec 8, 2021

General doubt: a coreos-installer somewhere which is not the same thing as coreos-installer somewhere else seems problematic. Could we consider adding some kind of highly visibile prefix/suffix to those aliases?

```

If you want to be able to cancel `coreos-installer` downloads, you
will have to add the `--tty` option to your podman comand line or shell alias:
Copy link

@PhrozenByte PhrozenByte Dec 9, 2021

Choose a reason for hiding this comment

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

I'd advise against using a Bash alias for coreos-installer due to this reason. A script however can easily solve this by simply checking whether a tty is connected to the script and choose accordingly:

#!/bin/sh
if tty -s; then
    podman run … --tty …
else
    podman run … # without --tty
fi

Choose a reason for hiding this comment

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

By the way: This more or less also addresses @lucab's concerns (at least if I understand him right), because with a script it's a matter of $PATH which coreos-installer is used (i.e. the "usual" way), instead of an alias overruling $PATH.

@travier
Copy link
Member Author

travier commented Dec 9, 2021

Nice suggestion, we can then do:

coreos-installer() {
    local TTY=""
    if tty -s; then
        TTY="--tty"
    fi
    podman run --pull=always                    \
        --rm --interactive ${TTY}               \
        --security-opt label=disable            \
        --volume "${PWD}":/pwd --workdir /pwd   \
        quay.io/coreos/coreos-installer:release \
        "${@}"
}

@bgilbert
Copy link
Contributor

bgilbert commented Jan 7, 2022

The alias (and @travier's shell function) is also confusing because it won't work for the install subcommand: it doesn't set --privileged and the bind mounts for /dev and /run/udev, and can't be run under sudo. On the other hand, we shouldn't encourage running subcommands like download and iso customize in a privileged container as root.

The shell function (or a script) could escalate privileges only if install is passed as the first argument. (coreos-installer doesn't allow arguments before the subcommand name, so that check should be sufficient.) If that makes the fragment too long for a code sample pasted into docs, we could commit it in the docs dir and link it from the getting-started page.

@PhrozenByte
Copy link

Good point 👍 However, I'm not so sure whether adding this just to the docs is the best way to go. It no longer is an alias, it's "real" code and strongly bound to the implementation of coreos-installer. IMHO it shouldn't be added to the docs dir, but somewhere else (I'm not sure whether the scripts dir is the right place...) - and it should be a script now, neither an alias, nor a function. IMHO aliases and functions are for one-liners. Obviously we must still add a link to the Getting Started page.

@travier
Copy link
Member Author

travier commented Jan 10, 2022

In general, those aliases are only useful for the non root and non install use case as generally an installation is a one shot command and you don't need it as an alias on the system. We could add a note about this: "Don't use those aliases to install a system".

Moreover, they are all suggested convenience wrappers, not "this is the only way" so I would prefer we keep them simple.

@bgilbert
Copy link
Contributor

coreos-installer may be used to prepare a boot device for a system other than the one being installed. For example, an Ubuntu VM host might use the coreos-installer container to create boot disks for VMs, using the example command immediately above the one being added here.

It seems awkward to provide a convenience alias that doesn't work in reasonable cases. What about something like this:

coreos-installer() {
    local sudo=""
    local args=(-i --security-opt label=disable)
    if [ "${1:-}" = install ]; then
        sudo=sudo
        args=(--privileged -v /dev:/dev -v /run/udev:/run/udev)
    elif tty -s; then
        args+=(-t)
    fi
    ${sudo} podman run --pull=always --rm -v .:/data -w /data "${args[@]}" \
        quay.io/coreos/coreos-installer:release "${@}"
}

I think that's small enough to not require a separate script, so we can avoid the issue of storing a file somewhere and linking to it from docs.

@travier
Copy link
Member Author

travier commented Jan 10, 2022

OK, if we go this route, we should probably also update the alias from the tutorial docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants