Michquinn/consolidate

[michquinn]

Docker Compose has a feature known as Profiles. By leveraging this feature, we should be able to remove the redundancy of having three separate bash scripts and docker-compose.yaml files for the different services.

Please note that these changes haven't been tested yet. Feedback is welcome

[FlatBallFlyer]

I see how you used curl and docker command options to simplify the use of ~/local directory. I'm assuming that you will define service layers in a copy of the docker-compose-person.yaml file that can be referenced on the docker compose up command to run a specific sub-set of containers? I'm not sure what the change will be, but I think it's starts in the various shell scripts? It seems like it will be able to replace the different docker-compose files with a single marked up version. Have you given any thought to how this will scale to additional services? The DB layer will be shared by many services, but not all. Running a local triplet is the initial goal, but running multiple services is a good stretch goal. See the updated ARCHITECTURE.md file for some details about the services to-be state. The diagrams render properly in my VSCode mermaid plugin, but the GitHub render does not appear to be working yet

[michquinn]

I see how you used curl and docker command options to simplify the use of ~/local directory. I'm assuming that you will define service layers in a copy of the docker-compose-person.yaml file that can be referenced on the docker compose up command to run a specific sub-set of containers? I'm not sure what the change will be, but I think it's starts in the various shell scripts? It seems like it will be able to replace the different docker-compose files with a single marked up version

Essentially, there's now one shell script, run-local.sh, which can be used in place of any of the three shell scripts that existed previously. To specify whether db, person, or person-api should be run, a positional command-line argument is passed. The script reads the value of that positional argument, if present, and assigns it to the variable COMPOSE_PROFILES which will cause docker compose to, as per Docker's documentation, run "Services with matching profiles… as well as any services for which no profile has been defined." Since the DB layer appeared to be common to all services, I left it undefined.

Have you given any thought to how this will scale to additional services? The DB layer will be shared by many services, but not all.

Not yet, though now that I know this I see I have to add the "db" profile explicitly as well.

[FlatBallFlyer]

I fixed the mermaid rendering problem, the diagram now renders properly on GitHub if you don't have a plugin. NOTE: everything after the Service Diagram is still early draft. Have you tested it yet? Please test, let me know when you've tested it and I'll do the same.

[michquinn]

The run-local.sh script has been tested with the updated docker-compose.yaml file and it appears to be working as intended. However, a couple of issues remain:

1. The changes to docker-compose.yaml from c240e3e2a7d1bf8630f3fff47417d13466a29b83 broke the script until a16e5a4cb4cfa9e2ad1b455720bbdc7bdc91c737, as I had not yet been able to test it and did not realize that docker compose down also needed to reference the compose file. Editing and rebasing is possible, though of course that means this branch as it is currently published should be removed and a corrected branch submitted.
2. There was a discrepancy between the healthcheck parameters for the mentorhub-mongodb service from the original three docker-compose files:

https://github.com/agile-learning-institute/mentorHub/blob/731995ae11d52c388abb35304810d2ad61a2e61d/docker-configurations/docker-compose-db.yaml#L13-L15

https://github.com/agile-learning-institute/mentorHub/blob/731995ae11d52c388abb35304810d2ad61a2e61d/docker-configurations/docker-compose-person.yaml#L13-L15

https://github.com/agile-learning-institute/mentorHub/blob/731995ae11d52c388abb35304810d2ad61a2e61d/docker-configurations/docker-compose-person-api.yaml#L13-L15

The new, unified docker-compose.yaml went with the second option simply because that file had all the services in it. What the value of those numbers should be is an unresolved question.

3. From Docker Compose's documentation: "When a service with assigned profiles is explicitly targeted on the command line its profiles are started automatically". Would this be an undesired side effect?

[FlatBallFlyer]

The reason the db has a health check is to allow the database to get up and healthy before the database versioning script runs, and that versioning script needs to successfully complete before any API's are started. I would assume that those dependencies are also honored when a service is started by profile.... i.e. if I start the UI service, it depends on the API service, which requires the Version Service to have a successful exit, which requires the DB to be healthy.... but I am just a docker hack.... If you want this PR Merged, you will need to take it out of draft status. You can merge it yourself.

[michquinn]

My main question was whether interval should be 10 or 15 seconds, and whether retries should be 5 or 10. When there were 3 compose files, they had different combinations and I wasn't sure if the difference was important