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

Simplify repo #85

Merged
merged 7 commits into from
Nov 19, 2024
Merged

Simplify repo #85

merged 7 commits into from
Nov 19, 2024

Conversation

amercader
Copy link
Member

@amercader amercader commented Nov 13, 2024

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:

  • One single place to define versions
  • 8 Makefiles -> 1 bash script
  • No more base/dev folders, base and dev images built from the same Dockerfile

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.:

    ./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. 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 base and dev folders. 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 with DOCKER_BUILDKIT=1 when running docker 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):

ckan-X.XX
├── Dockerfile          # Dockerfile for the image (in older versions there might be one for alpine and one for python)
├── VERSION.txt         # Full CKAN version built (eg. 2.11.0, 2.10.5)
├── PYTHON_VERSION.txt  # Python version used (eg. 3.10)
└── setup               # Setup scripts used by the images
    └── ...

@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.

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`)
ckan-docker-base.sh Outdated Show resolved Hide resolved
@wardi
Copy link
Contributor

wardi commented Nov 13, 2024

love it, this is a huge improvement

@wardi
Copy link
Contributor

wardi commented Nov 13, 2024

Can we use a script name that tab-completes a little easier? Maybe build.sh or similiar?

Co-authored-by: Ian Ward <ian@excess.org>
ckan-docker-base.sh Outdated Show resolved Hide resolved
ckan-docker-base.sh Outdated Show resolved Hide resolved
amercader and others added 4 commits November 15, 2024 10:52
Co-authored-by: Brett Jones <54408245+kowh-ai@users.noreply.github.com>
ckan-sys only exists in the Python based images (Dockerfile.py*)
after #80
@kowh-ai
Copy link
Contributor

kowh-ai commented Nov 15, 2024

@amercader - maybe a function to change a full ckan version to a major.minor version could be useful as the build command will not work with a full ckan version string however running a ckan-docker-base.sh versions command will output the full ckan version which is a bit confusing

./ckan-docker-base.sh versions                                                  1 ✘  04:18:10 pm
Versions built currently:
* master
* 2.9.11
* 2.10.5
* 2.11.0

eg:

check_ckan_version_ref() {
    local ckan_version_ref="$1"
    # If input is in the format X.Y.Z (e.g., "2.11.0"), trim to X.Y (e.g., "2.11")
    if [[ "$ckan_version_ref" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
        ckan_version_ref="${ckan_version_ref%.*}"
    fi
    echo "$ckan_version_ref"
}

We could then call it here:

"build")
        
        ckan_version_ref=$2
        # Check if the version is a full version or a major.minor version
        # If it's a full version, we'll use the major.minor version
        ckan_version_ref=$(check_ckan_version_ref "$ckan_version_ref")

        if [ -z "$ckan_version_ref" ]; then
            echo "Missing version"
            show_usage
            exit 1
        fi

@amercader amercader mentioned this pull request Nov 19, 2024
@amercader amercader merged commit 612d0b9 into main Nov 19, 2024
@amercader
Copy link
Member Author

Merging, @kowh-ai we can discuss your proposal in a separate PR

@amercader amercader deleted the simplify-repo branch November 19, 2024 08:26
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.

3 participants