fix: avoid redirect operator in justfile docker commands #1804
Conversation
Fraser999
requested review from
itamarreif and
quasystaty1
and removed request for
itamarreif
justfile
Outdated
| @@ -14,11 +14,11 @@ default_repo_name := 'ghcr.io/astriaorg' | |||
|
|
|||
| # Builds docker image for the crate. Defaults to 'local' tag. | |||
| docker-build crate tag=default_docker_tag repo_name=default_repo_name: | |||
| docker buildx build --load --build-arg TARGETBINARY={{crate}} -f containerfiles/Dockerfile -t {{repo_name}}/$(sed 's/^[^-]*-//' <<< {{crate}}):{{tag}} . | |||
| docker buildx build --load --build-arg TARGETBINARY={{crate}} -f containerfiles/Dockerfile -t {{repo_name}}/$(echo {{crate}} | sed 's/^[^-]*-//'):{{tag}} . | |||
There was a problem hiding this comment.
These commands are broken on my system (also before this patch) because I am neither running posix shell nor bash. Let's fix it for all users by prepending it with a #!/usr/bin/env bash (to make it work with the previous HERESTRING) or #!/usr/bin/env sh (to use your command substitution $(...) - we should probably do the latter).
|
On a second note: we should probably just list the commands to build services so that we don't have to rely on the mapping We only have a handful of services so this wouldn't cause too much boilerplate. |
justfile
Outdated
| docker-build crate tag=default_docker_tag repo_name=default_repo_name: (_crate_short_name crate "quiet") | ||
| #!/usr/bin/env sh | ||
| set -eu | ||
| short_name=`just _crate_short_name {{crate}}` |
There was a problem hiding this comment.
| short_name=`just _crate_short_name {{crate}}` | |
| short_name=$(just _crate_short_name {{crate}}) |
My comment re command substitution was due to my shell (fish), but setting #!/usr/bin/env sh fixes this (and backticks is probably more confusing nowadays than $()).
There was a problem hiding this comment.
I'll leave this as is, since Just uses backticks natively, so I guess this is a little less surprising in this context.
There was a problem hiding this comment.
Disagree: just uses that for its own DSL (the portion you linked), while here we have an inline script and command substitution is considered better practice and is strictly preferred over backticks.
justfile
Outdated
| docker-build-and-load crate tag=default_docker_tag repo_name=default_repo_name: (_crate_short_name crate "quiet") | ||
| #!/usr/bin/env sh | ||
| set -eu | ||
| short_name=`just _crate_short_name {{crate}}` |
There was a problem hiding this comment.
| short_name=`just _crate_short_name {{crate}}` | |
| short_name=$(just _crate_short_name {{crate}}) |
| # Maps a crate name to the shortened name used in the docker tag. | ||
| # If `quiet` is an empty string the shortened name will be echoed. If `quiet` is a non-empty string, | ||
| # the only output will be in the case of an error, where the input `crate` is not a valid one. | ||
| _crate_short_name crate quiet="": |
There was a problem hiding this comment.
Instead of prefixing with _, maybe an attribute makes it clearer?
| _crate_short_name crate quiet="": | |
| #[private] | |
| crate_short_name crate: |
You will note I also removed the quiet argument - I am not 100% we need it? I notice it is being used as an input to both docker-* recipes, but you also have set -eu to fail early. Can we just rely on that or is there extra use with having the arg?
There was a problem hiding this comment.
We're using the leading underscore style elsewhere in this file rather than the [private] attribute, so I'll leave as is.
We need the quiet arg to silence the output on success. The recipe is being invoked twice in each of the two parent recipes.
The first time it is being specified as a dependency so that it is invoked before running the parent. This is purely to validate the specified crate is a supported one. If that invocation fails, we get a clear error message and exit code 2. In the success case, without the quiet arg, we'd get the shortened crate name printed to the console, which is confusing and needless noise. So in that invocation, we set quiet to a non-empty string to suppress the output on success.
The second time it is called is in the body of the parent recipe, and in that case we require the successful output to populate our shell variable short_name, so we set quiet to an empty string.
Without the first call (when we call it as a dependency), if we only called in the body and the specified crate was invalid, we'd get no helpful error message.
There was a problem hiding this comment.
Thank you for the explanation!
|
There's a minor issue with the build logic for the cli Docker image unrelated to this PR. The |
|
Agreed with @quasystaty1 that the CLI issue will be handled in a follow-up PR. |
Summary
This replaces
sed '...' <<< textwithecho text | sed '...'in the main justfile.Background
On Ubuntu, the default shell doesn't recognise the redirect operator
<<<, hence thejustcommandsdocker-buildanddocker-build-and-loadboth fail on Ubuntu.Changes
<<<by pipingechooutput.Testing
Ran the commands on Ubuntu and macOS.
Changelogs
No updates required.