GitHub Action + Docker tag suffix #16
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The composite action didn't work as expected (the action's `action.sh` and `build.sh` weren't accessible), so I replaced the shell script with a tiny JavaScript bootstrap, that will properly collect the action inputs and generate a simple local build.sh and calls it. Note that we couldn't use a Docker container action, because we couldn't access the host docker ending, and would need to add both docker cli and docker server to the docker container...
Comment on lines
+22
to
+31
| // generate a local build.sh script: | ||
| fs.writeFileSync("build.sh", `#! bash | ||
| source ${__dirname}/build.sh | ||
| dockerSetup ${setupOpts.join(" ")} | ||
| echo $VERSION > VERSION | ||
| dockerBuildAndPush ${buildOpts.join(" ")} | ||
| `) | ||
|
|
||
| // execute it: | ||
| const build = spawn("bash", ["build.sh"], { maxBuffer: 100 * 1024 * 1024 }) |
There was a problem hiding this comment.
Woa. Wasn't expecting that.
Don't we need more escaping here? We can discover that along the way, though.
There was a problem hiding this comment.
Do you mean that:
- someone can sneak something like
$SOME_SECRETintobuild.shwhich might be a security issue (but might be legit); - we should quote the params (probably not possible for
build-options);
There was a problem hiding this comment.
I think it's mostly fear (with no concrete issue spotted) of generating a bash script and executing it.
I'm wondering if the __dirname could be abused to source another script, or somthing like that.
But maybe not think that much about an attacker (it should be trusted people the ones that could configure the Action), but about accidental issues with corner values (a directory name with a whitespace?).
But, once again: it's mostly unfounded fear. I'd say we merge 👍
The old one was depracted due to its base NodeJS version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebased from the new
mainbranch and updated with tests for the new-tand-obuild options.-t 'suffix'allows to set an optional suffix to the docker image tag (but not tolatest). For example-t '-next'will create & push atag-nextimage instead oftag.-o 'options'will pass custom options to thedocker buildcommand. For example-o "--build-args gemfile=Gemfile.next".Both arguments are require to build alternate images when we dual boot a Ruby on Rails application for example: we can build a regular image
build.shas well as an alternate image using updated dependencies withbuild.sh -t '-next' -o '--build-arg gemfile=Gemfile.next'and then deploy the version we want.Also provides a GitHub action, that will install
ci-docker-builderand build the docker image with the options we want right from a GHA workflow. For example:Supersedes #13