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

Dropped use of env_file and moved to docker secrets. #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheDivine
Copy link

Dropped use of env_file and moved to docker secrets.

MBSD-312

@@ -32,13 +31,13 @@ services:
- MW_RECAPTCHA_SITE_KEY
- MW_RECAPTCHA_SECRET_KEY
- MW_ADMIN_USER
- MW_ADMIN_PASS
- MW_ADMIN_PASS_FILE=/run/secrets/mw_admin_pass
Copy link
Member

Choose a reason for hiding this comment

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

There is no MW_ADMIN_PASS_FILE environment and nothing handles reading the password from file on the wiki container so this won't work

Choose a reason for hiding this comment

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

this won't work

this will work fine. Yes we have no the MW_ADMIN_PASS_FILE env variable, but we have the code to read the /run/secrets/mw_admin_pass and use it if one exists

Copy link
Member

@vedmaka vedmaka Dec 13, 2024

Choose a reason for hiding this comment

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

That's right, I was unaware of the changes that made the image read from /run/secrets/mw_admin_pass , but since this is not related to the ENV variable let's have it removed then

- MW_DB_NAME
- MW_DB_INSTALLDB_USER=root
- MW_DB_INSTALLDB_PASS
- MW_DB_INSTALLDB_PASS_FILE=/run/secrets/db_root_password
Copy link
Member

Choose a reason for hiding this comment

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

Same here, we don't have MW_DB_INSTALLDB_PASS_FILE env

Choose a reason for hiding this comment

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

but it will work

Copy link
Member

@vedmaka vedmaka Dec 13, 2024

Choose a reason for hiding this comment

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

Same as for #51 (comment) , lets remove the ENV then?

compose.yml Show resolved Hide resolved
- MW_DB_PASS=$MW_DB_INSTALLDB_PASS
- MW_SECRET_KEY
- MW_DB_PASS_FILE=/run/secrets/db_root_password
- MW_SECRET_KEY_FILE=/run/secrets/mw_secret_key
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Choose a reason for hiding this comment

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

we don't need this

Copy link
Member

Choose a reason for hiding this comment

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

let's remove then?

compose.yml Show resolved Hide resolved
restart: unless-stopped
profiles:
- production
volumes:
- ./updateEFO:/updateEFO
- ./_logs/updateEFO:/var/log/updateEFO
- ./updateEFO/rotatelogs-compress.sh:/rotatelogs-compress.sh
secrets:
- efo_secrets
Copy link
Member

Choose a reason for hiding this comment

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

Noting that just mounting the secrets file won't do anything by itself aside from mounting the secret as a file into /run/secrets. Something still need to read this file and ensure the values from the file are propagated to the configuration.

In case of the updateEFO there are envs like MW_BOT_USER , MW_BOT_PASSWORD and MW_UPDATE_EFO_PAUSE in use. This case also nicely highlights the reason why we're getting rid of the env_file usage - with env_file it's challenging to list the variables needed on the Compose file due to various limitations applied by the Compose https://docs.docker.com/compose/how-tos/environment-variables/set-environment-variables/#use-the-env_file-attribute

profiles:
- production
restart: unless-stopped
links:
- db
environment:
- MW_DB_USER=root
- MW_DB_PASS=$MW_DB_INSTALLDB_PASS
- MW_DB_PASS_FILE=/run/secrets/db_root_password
Copy link
Member

Choose a reason for hiding this comment

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

Won't work by the same reason - there is nothing that uses MW_DB_PASS_FILE env to read a value from a file and store into MW_DB_PASS

Choose a reason for hiding this comment

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

It will work

Copy link
Member

@vedmaka vedmaka Dec 13, 2024

Choose a reason for hiding this comment

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

Same as #51 (comment) , let's get rid of the ENV then

compose.yml Show resolved Hide resolved
secrets:
- db_root_password
- mw_admin_pass
- mw_secret_key

Choose a reason for hiding this comment

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

remove the mw_secret_key secret

@@ -32,13 +31,13 @@ services:
- MW_RECAPTCHA_SITE_KEY
- MW_RECAPTCHA_SECRET_KEY
- MW_ADMIN_USER
- MW_ADMIN_PASS
- MW_ADMIN_PASS_FILE=/run/secrets/mw_admin_pass

Choose a reason for hiding this comment

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

this won't work

this will work fine. Yes we have no the MW_ADMIN_PASS_FILE env variable, but we have the code to read the /run/secrets/mw_admin_pass and use it if one exists

compose.yml Show resolved Hide resolved
profiles:
- production
restart: unless-stopped
links:
- db
environment:
- MW_DB_USER=root
- MW_DB_PASS=$MW_DB_INSTALLDB_PASS
- MW_DB_PASS_FILE=/run/secrets/db_root_password

Choose a reason for hiding this comment

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

It will work

compose.yml Show resolved Hide resolved
- MW_DB_NAME
- MW_DB_INSTALLDB_USER=root
- MW_DB_INSTALLDB_PASS
- MW_DB_INSTALLDB_PASS_FILE=/run/secrets/db_root_password

Choose a reason for hiding this comment

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

but it will work

compose.yml Show resolved Hide resolved
- MW_DB_PASS=$MW_DB_INSTALLDB_PASS
- MW_SECRET_KEY
- MW_DB_PASS_FILE=/run/secrets/db_root_password
- MW_SECRET_KEY_FILE=/run/secrets/mw_secret_key

Choose a reason for hiding this comment

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

we don't need this

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