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

Add dev docker-compose file to manage supporting services #525

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented Oct 20, 2023

Changes

  • Added a new docker-compose file for development. It is supposed to be executed in conjunction with the main docker-compose.yml. This new file adds profiles attribute to control which services are needed for the development workflow. Additionally, network_mode is set to host driver so that bidirectional communication is possible. Bottleneck for bridge mode is Enketo which for some reason does not honor /etc/hosts for localhost, I was able to make it work with code modification on the fly, which looked dirty to me. One caveat for host mode: this is not available for "Docker Desktop for Mac", works great with OrbStack
  • Added dev config templates for nginx, enketo and service(central) services. These dev templates have hardcoded localhost endpoints

Two objectives I've kept in my mind to achieve this 1) Not to touch main docker-compose.yml that might cause unintended consequences 2) Not to modify application source code

What has been done to verify that this works as intended?

Verified locally. It would be great if all can try this and see if this is helpful.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Although I have modified enketo startup scripts, changes are contained in dev specific conditional block

Does this change require updates to documentation? If so, please file an issue here and include the link below.

None

Before submitting this PR, please make sure you have:

  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@sadiqkhoja sadiqkhoja marked this pull request as draft October 20, 2023 19:55
@sadiqkhoja sadiqkhoja force-pushed the docker-for-local-dev branch from 0821161 to 67c6397 Compare October 20, 2023 19:57
@sadiqkhoja sadiqkhoja marked this pull request as ready for review October 20, 2023 20:05
@matthew-white matthew-white changed the base branch from master to next October 24, 2023 19:33
@spwoodcock
Copy link
Contributor

Is work still ongoing on this PR?

I have just set up my own dev environment to work on central-backend and central-frontend & documented the process.
As a result, I have a slightly modified dev setup, also with a docker-compose.dev.yml.

I plan to submit my documentation for review, but should I also commit the dev setup changes?

@sadiqkhoja
Copy link
Contributor Author

Hi @spwoodcock, I will be working again on this PR next week, particularly removing duplicate *.json.dev.template from the individual services.

Can you please share your setup, is it a different approach? Maybe we can combine both.

@spwoodcock
Copy link
Contributor

Certainly!

I made a PR in our repo to more easily show the changes: https://github.com/hotosm/odkcentral/pull/1/files

My main goal was to make setting up a dev environment as easy as possible, with files mounted in the container for hot reload (on the backend). I planned to do the same for the frontend, running the dev server with hot reloading, but we may need to update vue-cli-service and node forman to vite.

I extended from the existing docker-compose.yml, so had to make a mock enketo service so nginx would start up correctly (as I only wanted to develop for central-backend and central-frontend).

Hope this helps - I'm more than happy to contribute to this PR if needed.

@sadiqkhoja sadiqkhoja force-pushed the docker-for-local-dev branch 4 times, most recently from 73d14ba to 5f75938 Compare February 12, 2024 23:37
Makefile Outdated Show resolved Hide resolved
postgres-upgrade.dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
* Added Makefile for quick start and stop
* Modified start-enketo.sh for dev with hardcode secrets, single redis config
* Modified start-postgres.sh to listen postgres14-upgrade even if data directory is absent
* Added instruction in README.md
@sadiqkhoja sadiqkhoja force-pushed the docker-for-local-dev branch from 1f84b0b to c32828c Compare November 4, 2024 20:32
@sadiqkhoja sadiqkhoja requested a review from alxndrsn November 4, 2024 20:40
@sadiqkhoja sadiqkhoja changed the base branch from master to next November 4, 2024 20:46
@alxndrsn
Copy link
Contributor

alxndrsn commented Nov 7, 2024

Anything to make this easier v welcome 👏

I've run make dev; what do I do now?

@lognaturel
Copy link
Member

@sadiqkhoja is away for the next couple weeks so I'll try to answer questions as best I can!

I've run make dev; what do I do now?

I believe the current intended usage is that you would then run frontend and backend locally and they would have access to Enketo, pyxform, etc.

@lognaturel lognaturel changed the title Docker for local dev Add dev docker-compose file to manage supporting services Nov 7, 2024
# Sets profiles for each service depending on whether that
# service is required for development of that particular application
# For Central be/fe development, we need postgres14, pyxform, enketo, redis_main
# For Enketo development, we need postgres14, service, nginx, redis_main
Copy link
Member

Choose a reason for hiding this comment

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

Remove stale comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@lognaturel
Copy link
Member

I believe the idea is that the supporting services are more complex to get up and running. The involve knowledge Central dev doesn't require and some have lots of dependencies to manage. The goal is to have a way to get them up and running that is as close to the production configuration as possible and that doesn't risk getting out of date.

You can see a different angle from @spwoodcock which targets also containerizing frontend and backend for dev: https://github.com/hotosm/odkcentral/pull/1/files#diff-97db29a7915320e63d41d38a0440360a87055ee8ed03757aa263116dbbb4aabe

That approach gets to what @moise10r is after with getodk/central-frontend#1048

If we go in that direction, we should make sure there is an answer for hot reloading and debugging. We should also make sure the maintenance burden/risk of it getting out of sync is worth it.

README.md Outdated Show resolved Hide resolved
@alxndrsn
Copy link
Contributor

Some queries inline, but in general I'd say if this PR is useful for @sadiqkhoja in its current form then it should be merged. If others use it, then they can iron out issues or improve things with fresh PRs going forwards.

profiles:
- central
# changing port here - can't use `ports` mapping with host networking
command: ["gunicorn", "--bind", "0.0.0.0:5001", "--workers", "5", "--timeout", "600", "--max-requests", "1", "--max-requests-jitter", "3", "main:app()"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a significant deviation from the parent docker-compose.yml. Is this running pyxform from source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just overriding the port in the command of the pyxform-http docker file: https://github.com/getodk/pyxform-http/blob/master/Dockerfile

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the standard command be used, and this container's port 80 be exposed locally on port 5001? That would seem to simplify this section, and also protect against future divergence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't expose ports when using host networking mode. And host networking is required so that Enketo can communicate with Central via http://localhost:8989, which is running on the host.

I know we can use host.docker.internal to access services running on the host from the bridge container but configuration in Enketo and Central doesn't really support that. For example, Central uses env.domain to generate URLs in OpenRosa manifest, whose value is http://localhost:8989 in local environment.

I had tried modifying /etc/hosts file in the Enketo container to map localhost to the IP assigned by docker to the host.docker.internal but Enketo using discontinued/deprecated request.js doesn't respects values in host file.

Copy link
Contributor

Choose a reason for hiding this comment

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

using discontinued/deprecated request.js doesn't respects values in host file.

Have you got a source for this? I couldn't find anything obvious in the request issue tracker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way requestjs resolves domain name has to do with it:

request/request#2491

Note: above issue closed due to staleness

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble reproducing this issue. Is it possible this has been resolved by enketo's move to @cypress/request (introduced by 37b8f8b2f0b057e7adaa2da472f88ab8a5e460e0 in April 2024)?

Copy link
Contributor Author

@sadiqkhoja sadiqkhoja Dec 13, 2024

Choose a reason for hiding this comment

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

I was trying to map localhost to the IP Address of host-gateway in Enketo container, that didn't work. Localhost was still resolving to 127.0.0.1.

So I ended up adding a custom domain in my local hosts file and changing default.env.domain to that host in the backend repository and now everything works. I have to access central via that custom host instead of localhost, which is fine. I have updated the instructions in readme file as well.

sed -i -e 's/enketo_redis_main/localhost/g' \
-e 's/enketo_redis_cache/localhost/g' \
-e 's/6380/6379/g' "$CONFIG_PATH.template"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change to reduce the number of redis containers required? If so, is the increased complexity in these scripts and the deviation from the standard docker-compose.yml worth the savings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Originally I wanted this PR to facilitate Enketo development as well and Enketo suggests to run only one redis in development for the easiness. I have now removed the port substitution from this command and starting two redis instances.

I still need to substitute enketo_redis_main and enketo_redis_cache with localhost because of host network mode.

Copy link
Contributor

@alxndrsn alxndrsn Dec 4, 2024

Choose a reason for hiding this comment

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

I still need to substitute enketo_redis_main and enketo_redis_cache with localhost because of host network mode.

Similar to pyxform config above, it would be great if there was a way to achieve this without having to modify files/enketo/start-enketo.sh and make fragile changes with sed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

Makefile Outdated Show resolved Hide resolved
- Start two instances of redis
- remove readlink
- use postgres service to create data dir and .postgres14-upgrade-successful
profiles:
- central
volumes:
dev_secrets:
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance of a trailing newline?

@alxndrsn
Copy link
Contributor

Looking nicely self-contained now 👍

@sadiqkhoja sadiqkhoja merged commit 26dc362 into getodk:next Dec 20, 2024
2 checks passed
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.

4 participants