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

Use docker env file and setup config in entrypoint #1

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

pdelboca
Copy link
Owner

@pdelboca pdelboca commented Dec 17, 2024

@avdata99 I have been reviewing this repo. This is PR to discuss a few points mostly regarding env variables and configuration files. I'm pointing it to my own fork to not mess around with your project.

Context

While normally Django/Flask have a straightforward approach ENV Variables -> Run Application, CKAN has an outdated *.ini file in between, forcing us to do ENV Variables -> .ini File -> Run CKAN. In other words, Django/Flask usually run the application and then setup their configuration object, while CKAN has a strong dependency on having a .ini file to be able to even execute the application (and then setup the configuration object).

This is why we always need an extra step to dump the environment variables into a configuration file before even attempting to run CKAN. This is always mandatory, even extensions like ckanext-envvars do not work properly if we start from a bad ckan.ini file (see ckan/ckanext-envvars#10)

I think this typical workflow designed for CD/CI can be simplified by using a production.ini file to directly handle secrets but that's another story.

Main change

The main change of this PR is to move all code related to ENV Variables to the entrypoint so we can use docker run --env-file ... or the env_file variable of docker compose.

This has the benefit of bringing CKAN development closer to Django/Flask:

  • docker build ... will take care mostly of copying files, installing dependencies and requirements.
  • docker run ... will take care of the configuration and running the application (plus local development stuff).

This means that:

  • build-env-vars.sh is no longer needed, this is taking care by Docker while running the application
  • setup-ckan-ini-file.sh is moved to the entrypoint.
  • Install of local extensions for development is moved to the entrypoint as well
    • Note: when installing ckanext-unckan extension, we can copy it in the Dockerfile instead of cloning it.

Other small changes

  • There are two patches folder, so we can clean the one that it is not used.
  • The whole docker/ckan/setup is not being used, so we can clean it.
  • Use the local postgresql Dockerfile to build the databases.
  • In my personal opinion, we could use less environment variables. For several reasons:
    • Some of our variables are not environment dependant
    • Some are not variables at all (or they change every 4 months)
    • Docker forces us to pass them through build/run time, so the less the better.
    • Example: CKAN_STORAGE_FOLDER, CKAN_GITHUB_URL, CKAN_GIT_BRANCH.

Let me know your thoughts, with all this changes, this is properly working on my machine TM 😂


if [ "$IS_DEV_ENV" = "true" ] ; then
if [ $ENV_NAME == "local" ] ; then

Choose a reason for hiding this comment

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

Creo que aca seria un solo =

Suggested change
if [ $ENV_NAME == "local" ] ; then
if [ $ENV_NAME = "local" ] ; then

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.

2 participants