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

Sanity check variantstore images before publishing [VS-889] #8291

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

mcovarr
Copy link
Collaborator

@mcovarr mcovarr commented Apr 14, 2023

tl;dr Added sanity checks, a smoke test, and fixed sample invocations in Docker build scripts.

Last month I built a bad base image when I was unsuccessfully trying to combine GCP and Azure CLIs in one image. What's worse is I seem to have tagged this broken image with a tag previously used for a good version of the image. These changes harden the Docker image build script to refuse to write over an existing tag and execute a smoke test against the image before pushing to GCR. While I was in there I also fixed the sample invocations that we saw during mobbing did not work on all versions of date.

@@ -2,7 +2,7 @@

if [ $# -lt 1 ]; then
echo "USAGE: ./build_build_base_docker.sh [DOCKER_TAG_STRING] [OPTIONAL:LATEST]"
echo " e.g.: ./build_build_base_docker.sh \$(date -I)-alpine-build-base"
echo " e.g.: ./build_build_base_docker.sh \$(date -Idate)-alpine-build-base"

Choose a reason for hiding this comment

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

Is this correct or a typo? What does the -Idata thing to here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Weird-looking but intentional. From the date man page on my machine:

     -I[FMT]
             Use ISO 8601 output format.  FMT may be omitted, in which case the default is date.  Valid FMT values are date, hours, minutes, and seconds.  The date and time is
             formatted to the specified precision.  When FMT is hours (or the more precise minutes or seconds), the ISO 8601 format includes the timezone.

I think what we saw with @RoriCremer last week was that she had a different version of date where the FMT was mandatory. It would be great to confirm that date -Idate does work on Rori's machine.

Choose a reason for hiding this comment

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

makes sense to me. I'd like to have her check that too, but I see why that argument is put there now. Makes the behavior you are expecting--which is the default in some implementations--explicit

Copy link
Contributor

@RoriCremer RoriCremer Apr 18, 2023

Choose a reason for hiding this comment

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

hahaha yes good idea
Screen Shot 2023-04-18 at 4 08 18 PM

seems to work!

Copy link

@koncheto-broad koncheto-broad left a comment

Choose a reason for hiding this comment

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

LGTM. Happy to see a basic, sanity-checking smoke test happening inline right after building it

if [[ $RC -eq 0 ]]
then
echo "Error: Tag '${TAG}' already exists for image '${BASE_REPO}', refusing to tag again."
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

smart--but also wont this get really annoying if we want to overwrite a docker image that we just made in error?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok ignore me, I just saw the next section about deleting it!

@mcovarr mcovarr merged commit 17afee4 into ah_var_store Apr 18, 2023
@mcovarr mcovarr deleted the vs_889_docker_sanity_checking branch April 18, 2023 20:23
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.

4 participants