medic / cht-sync

Data synchronization between CouchDB and PostgreSQL for the purpose of analytics.
GNU General Public License v3.0
3 stars 5 forks source link

72 change node startup to docker compose #73

Closed witash closed 6 months ago

witash commented 7 months ago

see related issue #72 ; let me know if this is something we want to do or rather stick with the node script @njuguna-n

if it is @andrablaj this may overlap what you are doing to update the documentation (and is branched off that branch), since we would change the startup command from node to bash.

this currently doesn't work for local with local couch and postgres containers; I don't want to go too far down the rabbithole before we agree that this is even something we want to do.

Closes #72

andrablaj commented 6 months ago

@garethbowen do you know the history behind running this project with node or any particular reason to stick to node? I see both node and bash script approaches across the repos for running docker compose, and I would love to hear your recommendations/best practices.

garethbowen commented 6 months ago

@andrablaj This PR doesn't remove any JS so I'm not sure what Node dependency this is talking about. The coding style guide documentation talks about bash vs node best practice.

andrablaj commented 6 months ago

@garethbowen, the related issue gives more details about the purpose of this change, which is to run cht-sync via a bash script instead of with node. I believe this is still WIP, as @witash assesses whether this is the right approach. I don't find any guidelines about running docker-compose with node vs. bash in the coding style doc. Can you please share the documentation you were talking about?

garethbowen commented 6 months ago

@andrablaj I read the related issue and couldn't find the node script in there either. Can you link me to it?

The first section in the coding style talks about Language.

andrablaj commented 6 months ago

@garethbowen Some commands are defined here and some actions that @witash mentioned are not relevant anymore are defined here; I guess we will need to remove them and their occurrences if we want to move forward with using a bash script (and actually we should remove the not-relevant commands even if we keep the current node setup).

Per the style guide, should we keep the node scripts, even if that makes an extra dependency on the node @garethbowen, for the sake of using a language that everyone is familiar with ?

garethbowen commented 6 months ago

Sorry I don't have any insight into why those scripts were necessary or why TS was chosen.

for the sake of using a language that everyone is familiar with ?

The style guide is of course just a guide. There are no absolute rules and whatever the team feels is the right way to solve this problem is fine. However the guide does list three other reasons why JS is preferred over shell, not just because it's what everyone is familiar with.

andrablaj commented 6 months ago

@garethbowen, thank you! I will leave @witash to assess pros & cons and decide whether to move to a bash script.

Regardless of that decision, the existing scripts need some attention, as you mentioned that some of the actions were not relevant anymore @witash. Let's start that cleanup and see what's left.

And the bash script is only for convenience; if the dev and other stuff is removed, its possible to run by using the docker compose commands directly

You also mentioned this in the issue description. So do we actually need a script, or will just running some docker compose commands be sufficient?

njuguna-n commented 6 months ago

Hello @witash, is this PR ready for review?

witash commented 6 months ago

Hello @witash, is this PR ready for review?

No, tests are failing because the logstash config task is removed. Which works IF COUCHDB_DBS=medic because that matches the logstash config that is in git, but fails for tests because there COUCHDB_DBS=test

Can replace the hardcoded references to "medic" in the logstash config in git with the COUOCHDB_DBS environment variable, which would work for tests, but that only works if COUCHDB_DBS has a single value.

the bigger problem is that COUCHDB_DBS is currently allowed to have several values, which would mean several logstash config files so the single file in git can't work.

  1. we could move creating/copying the logstash config files to the docker entrypoint script
  2. we could just leave it in node and abandon this PR
  3. do we really need multiple couchdb databases? what is the use case for having logstash read from multiple databases?
witash commented 6 months ago

after discussion with @njogz we will go with option 1, move creating logstash config file to docker entrypoitn script; although we want to avoid having too much logic in shell scripts, this change is relatively small and simple and worth avoiding adding extra dependencies for.

andrablaj commented 6 months ago

@witash I checked this PR as you mentioned the tests were failing. Can you try merging main branch changes into your branch and update your .env according to the values in env.template?

Also, please let me know if you can't access the Sonar issues that need to be fixed before merging.

andrablaj commented 6 months ago

Great! It looks like only Sonar issues need some attention (they seem quite simple to fix), and this should be ready to go. 🚀

andrablaj commented 6 months ago

This PR should also remove the occurrences of scripts - prod & local from package.json, as those aren't necessary anymore.

medic-ci commented 3 weeks ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: