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

cqfd: add support for the 'docker' option #157

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

Conversation

gportay
Copy link
Contributor

@gportay gportay commented Feb 18, 2025

This adds the option docker= in .cqfdrc to set the docker binary. It
allows to run the client using sudo(8) or to select the docker binary
(docker, podman).

@@ -168,6 +168,16 @@ inside the docker image.
``flavors``: the list of build flavors (see below). Each flavor has its
own command just like `build.command`.

``docker`` (optional): program used to invoke `docker` client.
Copy link
Member

Choose a reason for hiding this comment

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

You could add a test for this feature. It could pass a dummy command as docker, and check that it was executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if I should also add CQFD_DOCKER variable.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great, maybe even better than a docker option? We can expect users to have a custom docker installation, so they want to pass it through the variable. Passing it through the configuration file doesn't make a whole lot of sense with the many extra_run/build_args options we already have? A configuration would be useful if we want to distinguish which command to use per flavors or project, but it doesn't seem to be the use case you are addressing.

Copy link
Member

Choose a reason for hiding this comment

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

Since the use case rather depends on the user's docker installation than the cqfd project, I think it would be best if this feature was an environment variable rather than in the project file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I like both: environment CQFD_DOCKER and property docker=.

Also, prepending sudo to the docker command could be easy with that change:

diff --git a/cqfd b/cqfd
index 6d4a4ab..9748afd 100755
--- a/cqfd
+++ b/cqfd
@@ -27,6 +27,7 @@ cqfd_user='builder'
 cqfd_user_home='/home/builder'
 cqfd_user_cwd="$cqfd_user_home/src"
 cqfd_shell=${CQFD_SHELL:-/bin/bash}
+cqfd_docker=('docker')
 cqfd_docker_gid="${CQFD_DOCKER_GID:-0}"
 
 ## usage() - print usage on stdout
@@ -166,14 +167,14 @@ docker_build() {
 		args+=("$project_build_context" -f "$dockerfile")
 	fi
 
-	debug executing: docker build "${args[@]}"
-	docker build "${args[@]}"
+	debug executing: "${cqfd_docker[@]}" build "${args[@]}"
+	"${cqfd_docker[@]}" build "${args[@]}"
 }
 
 # image_exists_locally(): checks if image exists in the local image store
 # arg$1: the image name to check
 image_exists_locally() {
-	docker image inspect "$1" &> /dev/null
+	"${cqfd_docker[@]}" image inspect "$1" &> /dev/null
 }
 
 # docker_run() - run command in configured container
@@ -197,7 +198,7 @@ docker_run() {
 	if ! image_exists_locally "$docker_img_name"; then
 		# If custom image name is used, try to pull it before dying
 		if [ "$project_custom_img_name" ]; then
-			if ! docker pull "$docker_img_name" >& /dev/null; then
+			if ! "${cqfd_docker[@]}" pull "$docker_img_name" >& /dev/null; then
 				die "Custom image couldn't be pulled, please build/upload it first"
 			fi
 		else
@@ -255,6 +256,11 @@ docker_run() {
 		fi
 	fi
 
+	# Use sudo if user is not in the docker group
+	if [ "$cqfd_docker_gid" -eq 0 ]; then
+		cqfd_docker=(sudo "${cqfd_docker[@]}")
+	fi
+
 	# Display a warning message if using no more supported options
 	if [ -n "$CQFD_EXTRA_VOLUMES" ]; then
 		die 'Warning: CQFD_EXTRA_VOLUMES is no more supported, use
@@ -332,8 +338,8 @@ docker_run() {
 
 	args+=("$docker_img_name" cqfd_launch "$1")
 
-	debug executing: docker run "${args[@]}"
-	docker run "${args[@]}"
+	debug executing: "${cqfd_docker[@]}" run "${args[@]}"
+	"${cqfd_docker[@]}" run "${args[@]}"
 }
 
 # make_archive(): Create a release package.

@gportay gportay force-pushed the add-docker-option branch 2 times, most recently from c293154 to 38f207e Compare February 18, 2025 19:04
This adds the option docker= in .cqfdrc to set the docker binary. It
allows to run the client using sudo(8) or to select the docker binary
(docker, podman).
@florentsfl
Copy link
Contributor

While investigating the Docker rootless issue, I also tried podman and it seems to suffer from the same issues, that is to say file permissions errors etc.

For now it seems that cqfd is too tightly coupled to docker rootful so i would postpone this feature until cqfd is compatible with podman

@gportay
Copy link
Contributor Author

gportay commented Feb 19, 2025

For now it seems that cqfd is too tightly coupled to docker rootful so i would postpone this feature until cqfd is compatible with podman

Indeed. That change is useful to substitute the docker command by podman (or any other docker-like application).

We need to find out the proper options to run cqfd with a rootless setup first.

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.

3 participants