Container build script changes #760
Conversation
This avoids problems if any of the environmental variables used ever have a value which the shell tries to interpret.
This avoids leaking the Docker login credentials into the log
There was a problem hiding this comment.
I added bd0d88a and 074bc52 while I was testing this. One thought I had is that it seems like the docker tag / push blocks could be a loop for clarity but that was also making me wonder whether it's worth the trouble (or rewriting it in Python since it's around the threshold where that should be less code.
| set -euox pipefail | ||
|
|
||
| BUILD_ALL=${BUILD_ALL:=0} | ||
| BUILD_NUMBER=${BUILD_NUMBER:=1} |
There was a problem hiding this comment.
Is it reasonable to have a default here or should we fail if it's not defined? I was wondering whether that would be something where we'd want to try to avoid conflicts — e.g. maybe using the current Unix timestamp if it's not set:
| BUILD_NUMBER=${BUILD_NUMBER:=1} | |
| BUILD_NUMBER=${BUILD_NUMBER:=$(date +%s)} |
There was a problem hiding this comment.
This is normally set by Jenkins, but if you try to use the script locally it needs to be something. I'd prefer it to be an integer across the board since it gets used in things like cloudformation resource names that have character limits. I'll put a comment in there.
There was a problem hiding this comment.
A timestamp will satisfy the integer goal but we could have conflicts if it's started multiple times at the same second which is unlikely except in a CI parallel build scenario.
| set -euox pipefail | ||
|
|
||
| BUILD_ALL=${BUILD_ALL:=0} | ||
| BUILD_NUMBER=${BUILD_NUMBER:=1} | ||
| TAG=${TAG:-test} |
There was a problem hiding this comment.
Similarly, I was wondering whether this should default to test or something like $(git branch) to avoid conflicts with the default values.
There was a problem hiding this comment.
Again, normally this would be set by Jenkins and would either be latest, release, or the git branch name. Perhaps latest would be a more reasonable default - there's always going to be a latest docker image in the ECR.
There was a problem hiding this comment.
I guess I'm wondering whether this means we should have defaults or just fail outright if those aren't set since we're only supporting the Jenkins-managed environment.
No description provided.