Artemis icon indicating copy to clipboard operation
Artemis copied to clipboard

`Development`: Use env-files instead of yaml configs for docker compose

Open dangcosta opened this issue 2 years ago • 6 comments

Checklist

General

Motivation and Context

The change tries to simplify the Artemis deployment through Docker Compose by including the variables that are currently spread in multiple yml files in the docker-compose.yml besides creating a central .env files for the environmental variables.

Description

Added the values of the application.yml, application-artemis.yml, and application-dev.yml to the .env and abstracted the environmental variables in the docker-compose.yml to the .env file.

Steps for Testing

First, we need to create a local version of Artemis image instead of using the remote Docker image. We also need to change the Dockerfile to use the env variables passed in the env files instead of yml ones. Then we change the docker compose file to use the local version and test it. Run cd src/main/docker, docker compose up artemis-mysql then later docker-compose up artemis-app.

Review Progress

Code Review

  • [ ] Review 1
  • [ ] Review 2

Manual Tests

  • [ ] Test 1
  • [ ] Test 2

dangcosta avatar Jun 26 '22 19:06 dangcosta

Just for clarification: Is this PR meant for test deployments or production deployments? If the latter is the case, there are missing some properties for Jenkins, GitLab, SAML2 :)

dfuchss avatar Jun 26 '22 20:06 dfuchss

Just for clarification: Is this PR meant for test deployments or production deployments? If the latter is the case, there are missing some properties for Jenkins, GitLab, SAML2 :)

Hi @dfuchss ! This is an early stage (almost a POC) for the docker file. We (me, Matthias, Simon and Ludwig) are testing if it is possible to centralize all variables needed in one single file. The next stage is to work with the .env file. As soon as we have a working version, we will add all the properties needed for both (prod and dev). You are more than invited to review and point to the properties we may have missed.

dangcosta avatar Jun 29 '22 17:06 dangcosta

Just for clarification: Is this PR meant for test deployments or production deployments? If the latter is the case, there are missing some properties for Jenkins, GitLab, SAML2 :)

For test development deployments for now ;)

4ludwig4 avatar Jul 28 '22 09:07 4ludwig4

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

github-actions[bot] avatar Aug 22 '22 12:08 github-actions[bot]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

github-actions[bot] avatar Sep 01 '22 12:09 github-actions[bot]

The properties of the saml2 profile are missing, or am I wrong :) Just to be sure that I'm not missing anything.

dfuchss avatar Sep 11 '22 08:09 dfuchss

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

github-actions[bot] avatar Sep 28 '22 12:09 github-actions[bot]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

github-actions[bot] avatar Oct 06 '22 12:10 github-actions[bot]

@dangcosta There are still some TODO comments where I am not sure how the syntax has to be for the env style. I wanted to double-check these but don't know how to do this. Also, there are some values that seem to be replaced at some later stage (by spring and gradle) for instance the following in artemis.env which still need some attention:

...
EUREKA_INSTANCE_METADATAMAP_VERSION=#project.version#
EUREKA_INSTANCE_METADATAMAP_GITVERSION=$${git.commit.id.describe:}
...

It seems like the 2nd key-value pair is using spring variables which get populated currently in both the environment and the config section. So I guess these are fine. Not sure if it works if you just use environment variables so.

The 1st key-value pair seems to be some custom solution for introducing the version with gradle which is a bit problematic for the env files as these are not packaged into the war file and are therefore not processed by gradle. You can see these problems in the following files: https://github.com/ls1intum/Artemis/blob/684eff1c8a2aede5e9b83559aae2d1fe6d49713f/gradle/profile_dev.gradle#L66 https://github.com/ls1intum/Artemis/blob/684eff1c8a2aede5e9b83559aae2d1fe6d49713f/gradle/profile_prod.gradle#L28

4ludwig4 avatar Oct 06 '22 14:10 4ludwig4

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

github-actions[bot] avatar Oct 14 '22 12:10 github-actions[bot]

Me and @dangcosta understood the requirement for this PR wrong. What should have happened and still should happen in the future is the following:

Defaults for configurations are located in the yaml files according to their profiles (application-dev.yml, ...) and the spring application.yml. The yaml file application-local.yml doesn't contain default values and is an override of these defaults for the local IDE. We might add a version-controlled copy of it with values you SHOULD override as they are secrets, etc. A .env file or environment variables should be an override for the docker-compose setups, NOT a replacement for all yaml config files! We should recheck the structure of the yaml files. Some profiles like lts are maybe not even used anymore!

I already got in contact with Daniel to discuss the future of this PR.

4ludwig4 avatar Nov 15 '22 21:11 4ludwig4

We assigned this task to me and I will reopen this PR when I have time to work on it.

4ludwig4 avatar Nov 23 '22 11:11 4ludwig4