Skip to content
This repository has been archived by the owner on Jul 17, 2023. It is now read-only.

OCI: shell conditional #70

Merged
merged 2 commits into from
May 18, 2021

Conversation

andrew-womeldorf
Copy link
Contributor

I'm very open to suggestion for this change! I know this is a common problem with shell scripting, and I'm not sure my solution is "correct".

The problem I'm having is that this conditional always evaluates to true on my machine. The outputted script looks like this:

#!/bin/sh

    if [ !  ]; then
        skopeo copy -q oci:plz-out/gen/src/api/api "docker-daemon:my-image"
        docker run -it my-image $@
    else
        podman run -it oci:plz-out/gen/my-image $@
    fi

I don't know if it's "correct" for the evaluation of $(command -v {CONFIG.PODMAN_TOOL}...) to occur in the build def, or if that should've been output as $(command -v podman...) to the shell script. I'm only doing local builds, so it's inconsequential for me.

At this point, I can say that I've had success with the conditional correctly evaluating when I remove the brackets. My machine does have podman installed, so I expect to evaluate false and run the podman command. If I replace CONFIG.PODMAN_TOOL with something my machine doesn't have, like docker, the conditional correctly evaluates to true.

@Tatskaari
Copy link
Member

Tatskaari commented May 17, 2021

I think it should be as simple as:

if command -v {CONFIG.PODMAN_TOOL} > /dev/null; then 
    {CONFIG.PODMAN_TOOL} run -it {img_loc} \\\$@
else 
    ...
fi

I don't think we need the subshell. We just want to check the exit code of command -v right?

@@ -193,7 +193,7 @@ def container_image(

# run_rule, runs the image using podman or docker if podman not found.
cmd = f'''
if [ ! $(command -v {CONFIG.PODMAN_TOOL} &> /dev/null) ]; then
if ! $(command -v {CONFIG.PODMAN_TOOL} &> /dev/null); then

Choose a reason for hiding this comment

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

This looks like the correct way of doing it. The enclosing brackets are a synonym for test expr, whereas the above is a command.

@andrew-womeldorf
Copy link
Contributor Author

Thank you both for the review!

@tiagovtristao
Copy link

Thank you both for the review!

Can you just remove the command being executed in a subshell as suggested by @Tatskaari? Happy to get this merged afterwards. Thanks!

@andrew-womeldorf
Copy link
Contributor Author

Can you just remove the command being executed in a subshell as suggested by @Tatskaari? Happy to get this merged afterwards. Thanks!

Yep, done! Thanks!

@Tatskaari Tatskaari merged commit 9903770 into thought-machine:master May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants