Allow setting and expanding . for the workdir

[mmlb]

This will make it easier to use docker-in-docker type builds that need to mount workdir relative files/directories in the child docker run.

[mmlb]

This is mostly a rough draft to solicit feedback. Documentation would still need to be added.

Alternatively, I was thinking that maybe . could be supported and then if used with expand_relative_volume_path there would be no need for a "magic" value. I can re-work the PR if that's a preferred route.

[toote]

@mmlb thanks a lot for the PR and opening it up for discussion! Unfortunately, I am very confused with it.

You mention you want to support docker-in-docker, but in those instances there is little to no changes necessary for the plugin to be used. The only reason to have paths match between host and container is if you are sharing the host's docker socket inside the container and want it to use that one instead.

Finally, I don't understand your code change. The logic states that if the workspace variable is set to whatever pwd is, it sets it to pwd_default... which is the result of running pwd in the first place. So basically it does nothing.

So, in conclusion, if you are interested in sharing the host docker, I believe that it is indeed an great idea but I would suggest implementing it as a completely separate option that in addition to updating the default place where the checkout is to be mounted on, would also share the docker socket as a volume automatically (and any other variable/place you can think of). I would also add a warning to ensure people know to be careful if they specify further volumes to be mounted.

[mmlb]

@mmlb thanks a lot for the PR and opening it up for discussion! Unfortunately, I am very confused with it.

You mention you want to support docker-in-docker, but in those instances there is little to no changes necessary for the plugin to be used. The only reason to have paths match between host and container is if you are sharing the host's docker socket inside the container and want it to use that one instead.

Sorry for the confusion here. I am definitely sharing the docker socket and not running dind, poor choice of words.

Finally, I don't understand your code change. The logic states that if the workspace variable is set to whatever pwd is, it sets it to pwd_default... which is the result of running pwd in the first place. So basically it does nothing.

The code isn't checking for the return of $(pwd)/$PWD, but the string "pwd". I don't know what the PWD is when running on the host without making some guesses that would probably be brittle. So the string "pwd" is a placeholder for "$PWD". :thinking: actually... would $PWD in the pipeline.yaml actually work? Oh not it shouldn't because that's going to be at pipeline upload not step execution. This is why I say maybe using expand_relative_volume_path and workspace: "." is another option since it seems more immediately obvious than a "magic string".

So, in conclusion, if you are interested in sharing the host docker, I believe that it is indeed an great idea but I would suggest implementing it as a completely separate option that in addition to updating the default place where the checkout is to be mounted on, would also share the docker socket as a volume automatically (and any other variable/place you can think of). I would also add a warning to ensure people know to be careful if they specify further volumes to be mounted.

I don't think this would be a good idea. Almost all of the pieces are available to enable the workflow that I'm trying to use w/o having to need to find the documentation for this one specific feature. We can already pass in env vars and we can already bind mount the docker socket and other directories. I would definitely prefer workspace: "." vs workspace: "pwd" and either one of those over share_host_docker and need to figure out what that means. I only started off with the workspace: "pwd" because I thought that the relative one would be less palatable but really I think its the best option. It fits in with other uses of expand_relative_volume_path and it matches what I kind of do when running docker commands locally docker run --rm -ti -v $PWD:$PWD -w $PWD ... but since I don't have $PWD in BK this seems like the next best thing.

[mmlb]

@toote here's what I'd actually prefer:

Author: Manuel Mendez <github@i.m.mmlb.dev>
Date:   Mon Oct 23 11:47:41 2023 -0400

    Extract relative path expansion to separate function

    Its going to be useful on its own in the next commit.

    Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

diff --git a/lib/shared.bash b/lib/shared.bash
index 1f14f89..cb46c98 100644
--- a/lib/shared.bash
+++ b/lib/shared.bash
@@ -47,6 +47,19 @@ function plugin_read_list_into_result() {
   [[ ${#result[@]} -gt 0 ]] || return 1
 }

+# docker doesn't do local path expansion, so we add very simple support for .
+function expand_relative_path() {
+  local path=$1
+
+  if [[ $path =~ ^\.: ]] ; then
+    printf "%s" "${PWD}${path#.}"
+  elif [[ $path =~ ^\.(/|\\) ]] ; then
+    printf "%s" "${PWD}/${path#.}"
+  else
+    echo "$path"
+  fi
+}
+
 # docker's -v arguments don't do local path expansion, so we add very simple support for .
 function expand_relative_volume_path() {
   local path
@@ -57,13 +70,7 @@ function expand_relative_volume_path() {
     path="$1"
   fi

-  if [[ $path =~ ^\.: ]] ; then
-    printf "%s" "${PWD}${path#.}"
-  elif [[ $path =~ ^\.(/|\\) ]] ; then
-    printf "%s" "${PWD}/${path#.}"
-  else
-    echo "$path"
-  fi
+  expand_relative_path "$path"
 }

 # shellcheck disable=SC2317  # Don't warn about unreachable commands in this function

commit f96113c90e36ab0a5a8e70c487cef4e540199df6
Author: Manuel Mendez <github@i.m.mmlb.dev>
Date:   Thu Oct 19 13:01:06 2023 -0400

    Allow setting and expanding . for the workdir

    This will make it easier to use docker via bind-mounted-docker.sock type
    builds that need to mount $PWD (on docker host) relative files/directories
    in the child docker run.

    Signed-off-by: Manuel Mendez <github@i.m.mmlb.dev>

diff --git a/commands/run.sh b/commands/run.sh
index cde573b..8f25200 100644
--- a/commands/run.sh
+++ b/commands/run.sh
@@ -61,6 +61,7 @@ workdir=''

 if [[ -n "${BUILDKITE_PLUGIN_DOCKER_WORKDIR:-}" ]] || [[ "${BUILDKITE_PLUGIN_DOCKER_MOUNT_CHECKOUT:-on}" =~ ^(true|on|1)$ ]] ; then
   workdir="${BUILDKITE_PLUGIN_DOCKER_WORKDIR:-$workdir_default}"
+  workdir=$(expand_relative_path "${workdir}")
 fi

 # By default, mount $PWD onto $WORKDIR

[toote]

I understand what you mean, but I think that there is a better (and smaller) change that would achieve the same result.

Adding a call to expand_relative_volume_path on the volume for the mount dir should be enough to get what you want/need. With that in place, if you turn on volume variable expansion and set the workdir option to $$BUILDKITE_BUILD_CHECKOUT_PATH (the doulbe $ would make it evaluate on step time instead of upload time) it would just work as you expect.

My suggestion to add a new option would avoid all the hassle of settting up all the correct options for it to work. It would encapsulate adding the socket mounting, keeping the workdir mountpath and would prevent you having to turn on the unsafe option of volume variable expansion if you don't want it for all volumes.

[mmlb]

I understand what you mean, but I think that there is a better (and smaller) change that would achieve the same result.

Adding a call to expand_relative_volume_path on the volume for the mount dir should be enough to get what you want/need. With that in place, if you turn on volume variable expansion and set the workdir option to $$BUILDKITE_BUILD_CHECKOUT_PATH (the doulbe $ would make it evaluate on step time instead of upload time) it would just work as you expect.

But volume variable expansion isn't happening for workdir so if I pass $$BUILDKITE_BUILD_CHECKOUT_PATH workdir is going to be the string $BUILDKITE_BUILD_CHECKOUT_PATH not the expanded value right?

My suggestion to add a new option would avoid all the hassle of setting up all the correct options for it to work. It would encapsulate adding the socket mounting, keeping the workdir mountpath and would prevent you having to turn on the unsafe option of volume variable expansion if you don't want it for all volumes.

Yeah this would definitely work too, I wasn't sure what else you were thinking when earlier you mentioned:

would also share the docker socket as a volume automatically (and any other variable/place you can think of)

Honestly I prefer my expand_relative_path support in workdir. Its a simpler step in my mind from using docker in my dev machine then using the docker plugin in BK. Also, its just a small refactor + 1 line change (I've force pushed that into this PR) :). But, if you would rather the explicit setting then I'd be happy to do that instead. Any thoughts on the name for it? Also what other variables/places do you think might need to be updated for the docker socket?

[toote]

But volume variable expansion isn't happening for workdir so if I pass $$BUILDKITE_BUILD_CHECKOUT_PATH workdir is going to be the string $BUILDKITE_BUILD_CHECKOUT_PATH not the expanded value right?

That is why my suggestion started with _Adding a call to expand_relative_volumepath on the volume for the mount dir. That would take care of that and it would be a single-line change to the whole code. In particular:

--- a/commands/run.sh
+++ b/commands/run.sh
@@ -65,7 +65,7 @@ fi

 # By default, mount $PWD onto $WORKDIR
 if [[ "${BUILDKITE_PLUGIN_DOCKER_MOUNT_CHECKOUT:-on}" =~ ^(true|on|1)$ ]] ; then
-  args+=( "--volume" "${pwd_default}:${workdir}" )
+  args+=( "--volume" "$(expand_relative_volume_path "${pwd_default}:${workdir}")" )
 fi

With that single line change, the way to get what you want would be to:

• activate the option expand-volume-vars
• set workdir to $$BUILDKITE_BUILD_CHECKOUT_PATH
• add the host docker socket to the shared volumes

[toote]

if you would rather the explicit setting then I'd be happy to do that instead. Any thoughts on the name for it? Also what other variables/places do you think might need to be updated for the docker socket?

I would name it share-host-docker and it would involve 3 changes:

• when the mount checkout option is enabled, it should be mounted on ${pwd_default}:${pwd_default}
• it would add the default docker socket to the volumes
• update the documentation

[mmlb]

ahh, sgtm. Will add that stuff in.

[pzeballos]

ahh, sgtm. Will add that stuff in.

Hey @mmlb! Thank you for your contribution! would you able to finish this PR? 🙂

[mmlb]

Oops I completely forgot about this. I think I avoided this need so its not pressing for me anymore. I say we close for now.

[pzeballos]

We'll close it, but we'll add this suggestion to the backlog so we can work on it