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

Fix docker/podman discovery script for podman-compose. #96

Merged
merged 4 commits into from
Jun 13, 2024
Merged

Fix docker/podman discovery script for podman-compose. #96

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d7b4f66
Fix docker/podman discovery script for podman-compose.
merlante Jun 13, 2024
3439d02
Fix docker/podman discovery in Makefile -- fixes lint and pr-check.
merlante Jun 13, 2024
bccb328
Update to correct kessel relations image location.
merlante Jun 13, 2024
60aa58c
Remove superfluous and obsolete -compose check.
merlante Jun 13, 2024
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
12 changes: 9 additions & 3 deletions spicedb/check_docker_podman.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,18 @@ else
exit 1
fi

# Check if Podman is installed
# Check if docker-compose is installed
if command_exists docker-compose; then
echo "docker-compose is installed."
else
echo "docker-compose not found. Please install either docker-compose or podman-compose to proceed."
exit 1

Copy link
Contributor

Choose a reason for hiding this comment

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

This update will only echo only.
Script: https://github.com/project-kessel/relations-api/blob/main/spicedb/start-insights-rebac.sh#L6
continue to use docker compose as the default compose tool.

Newer version of docker-compose follows: docker compose vs docker-compose. I think Will had issue on linux for docker-compose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add export COMPOSE=docker-compose or export COMPOSE=podman-compose

Update the https://github.com/project-kessel/relations-api/blob/main/spicedb/start-insights-rebac.sh#L6 to use the COMPOSE var

Copy link
Contributor Author

@merlante merlante Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's inconsistent as it is. It checks for docker-compose and podman-compose, but actually uses "docker compose" or "podman compose" in the scripts.

The hyphen format is obsolete (https://stackoverflow.com/questions/66514436/difference-between-docker-compose-and-docker-compose/66526176#66526176), so we can either assume that compose is directly included with docker/podman in the versions that users are using these days, or we can do some version detection on docker and fall back to the hyphen version.

Personally, I'm inclined to do the minimal thing, and just remove the compose checks from the above script. Wdyt?

(As you say, the checks don't set a var, just warn you if you don't have a compose command. And in fact, the check could be wrong in the future when the hyphen versions are no longer supported in installs.)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, remove them would make sense.

# Check if podman-compose is installed
if command_exists podman-compose; then
echo "podman-compose is installed."
else
echo "docker-compose or podman-compose not found. Please install either docker-compose or podman-compose to proceed."
exit 1
fi
fi
fi

Loading