Improved plugable component architecture #64
Conversation
All the other scripts that source default.env do not need to source each component default.env since those scripts are not component specific so should not rely on config env from each components. This can change in the future.
Could not move PAVICS_FQDN_PUBLIC since it depends on PAVICS_FQDN in env.local. default.env must not depend on any vars from env.local.
…t enabled Achieved by adding to the templating required vars list only if "monitoring" component is enabled in EXTRA_CONF_DIRS.
…component Also move defaults in generic_bird/docker-compose-extra.yml into generic_bird/default.env to centralize them all.
…ontained component Emu is also "generalized" like generic_bird, it can be used to deploy another bird. Only difference vs generic_bird is that Emu do not have a postgres DB.
Also make original thredds entrypointwrapper customization to also work with testthredds. Force default testthredds image to be same as regular thredds.
All changes are inside testthredds component, sweet first new improvement leveraging the improved plugable architecture.
There was a problem hiding this comment.
Looks good and I appreciate keeping extra stuff aside.
I see more and more image name moving from the compose file to an env, what is the rational for this? (My fear is for the component tracking system with auto PR that will have a hard time to update the image version)
Also if you are not in a rush before your vacation, I would like that Mathieu take a look at this PR too.
|
|
||
| # Re-read env.local to make sure it can override ALL defaults from all | ||
| # components. | ||
| [ -f env.local ] && . ./env.local |
There was a problem hiding this comment.
Maybe we should indicate the priority of all those envs file somewhere?
There was a problem hiding this comment.
Same priority as before with the docker-compose-extra.yml fragment
birdhouse-deploy/birdhouse/env.local.example
Lines 55 to 57 in cfaf768
I can elaborate a little bit more.
The reason for image moving from the I also found an unexpected benefit to this, having the image in the For the future auto image update detection, maybe we can move all images into the
I wanted to add @matprov as reviewer but he was not in the list of contributor for this repo so I sent him an invite last night already. |
Thank you for that. |
| $ALERTMANAGER_ADMIN_EMAIL_RECEIVER | ||
| $SMTP_SERVER |
There was a problem hiding this comment.
+1 on this one
I was wondering why these variable needed to be defined even if monitoring wasn't enabled.
This offers more flexibility, great.
| if [ -z "$WANTED_CONTEXT_ROOT" ]; then | ||
| WANTED_CONTEXT_ROOT="twitcher/ows/proxy/thredds" | ||
| fi | ||
| if [ -z "$WANTED_CONTEXT_ROOT_WARFILE_NAME" ]; then | ||
| WANTED_CONTEXT_ROOT_WARFILE_NAME="twitcher#ows#proxy#thredds" | ||
| fi |
There was a problem hiding this comment.
Not sure of both variable names starting with WANTED_.
Would only CONTEXT_ROOT do the job instead?
There was a problem hiding this comment.
Existing variable name, I did not want to change the existing code too much. There is EXISTING_CONTEXT_ROOT and this WANTED_CONTEXT_ROOT. Does this naming really bother you a lot?
There was a problem hiding this comment.
Not at all. Seemed strange when I first saw it but I would probably not even notice if I haven't reviewed this part.
| # Last dir/component in the EXTRA_CONF_DIRS list have highest override | ||
| # precedence, example: | ||
| # | ||
| # * Last docker-compose volume mount to same destination win over the | ||
| # previous docker-compose volume mount. | ||
| # | ||
| # * Last default.env can change the values of all previous default.env. | ||
| # | ||
| # * Last pre/post docker-compose-up script can potentially undo actions | ||
| # from previous scripts. | ||
| # | ||
| # Suggested to keep the private-config-repo last in the list to be able to | ||
| # override anything. |
There was a problem hiding this comment.
Thanks for specifying.
It is worth mentioning since with the pre/post hooks and custom configs, precedence can become tricky at a certain point.
There was a problem hiding this comment.
Agreed. The variable names in each component default.env file should not collide else one will override the other one. Probably not an issue for components in this repo but if we pull in components from other repos, this might become an issue.
Great benefits. Of course we need to manually update the I went through this PR and I agree with the changes, especially the one related to extra config not having to be defined if the extra is not used. @tlvu Please re-send me the invitation since it was a time limited offer |
|
Tagged |
|
The automated deployment is broken due to the rename of Error in logs: Easy and one time only work-around is to manually perform a |
Before this PR, components needing default values, needing template variable substitution, needing to execute commands pre and post
docker-compose upare hardcoding their needs directly to the "core" system, basically "leaking" their requirements out even when they are not activated (fixes #62).This PR provides true plugable architecture for the components so they can provide all their needs without having to modify the code of the "core" system.
All the components (monitoring, generic_bird, emu, testthredds) are modified to leverage the new plugable architecture, with additional customizations given it is cleaner/easier to have default configuration values.
Given this PR both changes the architecture and modify many components at the same time, it is best to read each commit separately to easier understand which code change belongs to which "goal".
Deployed here https://lvupavicsmaster.ouranos.ca with all the impacted components activated to test the change: