-
Notifications
You must be signed in to change notification settings - Fork 10
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
Rework the docker images #116
Conversation
We were doing wacky stuff like downloading all the installers and installing them. This is silly, we already have pre-built images for almost everything. With the removal of replace directives from the monorepo, we can also use `go install` to properly build other little bits like gravwellGenerator and reimport. Now that changes have been made, it's important to go through and test/update *every single lab*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we always use the latest version?
We don't have a large tail on dockerhub currently: 5.4.9
, 5.4.10
, 5.4.11
, latest
I feel like this could become broken/outdated pretty quickly if we tie it closely to a particular version. We always have a latest version though.
I think we also need to update the README in the root directory.
dockerfiles/createslim.sh
Outdated
@@ -5,8 +5,8 @@ if [ ! -f "$LICENSE" ]; then | |||
fi | |||
|
|||
docker rmi gravwell:slim #remove existing slim image | |||
docker pull gravwell/gravwell:latest # grabs latest gw image from dockerhub | |||
docker create --name slim gravwell/gravwell:latest #create temp container from latest image | |||
docker pull gravwell/gravwell:${VER} # grabs latest gw image from dockerhub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we continue to use latest?
If not, we should probably change the comment here that says "latest"
dockerfiles/build_all.sh
Outdated
LOGFILE=/tmp/build.log | ||
OUTDIR=../dockerimages/ | ||
VER=${VERSION:-5.3.0} | ||
VER=${VERSION:-5.4.10} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to update the README in the root directory. If we are listing a version there, it should also use latest, but more importantly, I thought the point of this change was to use docker packages without having to build, which is what the README is still telling you to do.
If I try to follow the README for what I am supposed to do, then I get an error "Must set GOPATH". If you are still going to have build instructions in the README, then maybe we need more details to (1) indicate that this is optional and that you can use public containers instead (?) and/or (2) provide more details about setting GOPATH as a prerequisite.
We build Docker images for a few reasons:
I will add a note about GOPATH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Per #113 and #110, our build process did a lot of excess work. These changes simplify the Docker build steps to mostly package up our existing Docker images with custom configs where needed. It also reworks the training document to deal with the tweaks.