Merged
Conversation
Three main focuses: * Consolidate version handling. We had version numbers all over the place and many different variables with slightly different meanings. Now we only have the version defined in one place (VERSION.txt), and the vars have been consolidated (Dockerfiles only use CKAN_REF, and the ones in the shell script should be hopefully named as what they are) * Centralize building (and pushing). Rather than 8 different Makefiles mostly identical we now only have one bash script at the root of the file which builds the relevant images according to the passed parameters, e.g.: ``` ./ckan-docker-base.sh build 2.11 ./ckan-docker-base.sh build master ./ckan-docker-base.sh build 2.10 base ./ckan-docker-base.sh build 2.0 dev ``` It also centralizes commands for pushing the images to DockerHub, but that will be soon done via GitHub Actions so it's discouraged. * Reduce number of Dockerfiles. While we still keep separate images for prod and dev environments, they are all now built from the same Dockerfile using staged builds and the Buildkit ability to just build the stages needed to build the final image (so buildkit must be enabled with `DOCKER_BUILDKIT=1` when running `docker build`)
wardi
reviewed
Nov 13, 2024
Contributor
|
love it, this is a huge improvement |
Contributor
|
Can we use a script name that tab-completes a little easier? Maybe |
Co-authored-by: Ian Ward <[email protected]>
kowh-ai
reviewed
Nov 14, 2024
kowh-ai
reviewed
Nov 14, 2024
Co-authored-by: Brett Jones <[email protected]>
ckan-sys only exists in the Python based images (Dockerfile.py*) after #80
Contributor
|
@amercader - maybe a function to change a eg: We could then call it here: |
Merged
Member
Author
|
Merging, @kowh-ai we can discuss your proposal in a separate PR |
This file contains hidden or 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
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.
While working on the building automation in #73 it become clear that the current structure of the repo was becoming a bit unwieldy. This PR is a proposal to simplify and consolidate various things.
TL;DR:
Here's more detail on the changes from less to more potentially controversial (I think):
Consolidate version handling. We had version numbers all over the place and many different variables with slightly different meanings. Now we only have the version defined in one place (VERSION.txt), and the vars have been consolidated (Dockerfiles only use
CKAN_REF, and the ones in the shell script should be hopefully named as what they are)Centralize building (and pushing). Rather than 8 mostly identical but not quite Makefiles we now only have one bash script at the root of the repo (
ckan-docker-base.sh) which builds the relevant images according to the passed parameters, e.g.:It also centralizes commands for pushing the images to DockerHub, but that will be soon done via GitHub Actions so it's discouraged. Of course the bash script is a bit more complex than all the individual Makefiles but I think it's easy enough to follow and extend (please do shout if you think it's not!)
Reduce number of Dockerfiles and remove the
baseanddevfolders. While we still keep separate images for prod and dev environments (I personally think this is good), they are all now built from the same Dockerfile using staged builds and the Buildkit ability to just build the stages needed to build the final image (so buildkit must be enabled withDOCKER_BUILDKIT=1when runningdocker build). It's easier to understand looking at one of the new Dockerfiles but basically we are not building the "dev" stage unless we pass a build param (ENV=dev).With these changes we end up with this (per CKAN version):
@wardi @kowh-ai It would be great if you could have a look and see if there's something you are not happy with. To implement the automations in #73 we need to settle on a repo structure. I'm happy to wait until #80 is merged and port the changes here myself if that will happen soon.