Skip to content

feat: update docker flow #496

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Conversation

choffmann
Copy link
Member

@choffmann choffmann commented May 11, 2025

  • only use one Dockerfile for multiple stage enviroments
  • move infra compose file to root directory as compose.yaml
  • add minio, keycloak and reverse proxy to infra compose
  • add compose.app.yaml to run backend and migrations with docker compose

@choffmann choffmann self-assigned this May 11, 2025
@choffmann choffmann force-pushed the feature/update-docker-flow branch 3 times, most recently from 39b858a to 0797175 Compare May 11, 2025 16:06
@choffmann choffmann requested review from a team as code owners May 11, 2025 16:06
Copy link

@akatranlp akatranlp left a comment

Choose a reason for hiding this comment

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

After the comments this thing is good to go ⭐

@choffmann choffmann force-pushed the feature/update-docker-flow branch from 0797175 to 1598874 Compare May 12, 2025 14:16
@choffmann choffmann requested a review from akatranlp May 12, 2025 14:17
@choffmann choffmann force-pushed the feature/update-docker-flow branch 2 times, most recently from 6b06c62 to 93f3f80 Compare May 12, 2025 18:07
Copy link

@akatranlp akatranlp left a comment

Choose a reason for hiding this comment

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

On little typo is missing,

@choffmann choffmann force-pushed the feature/update-docker-flow branch from 93f3f80 to f28b774 Compare May 13, 2025 20:10
@choffmann choffmann requested a review from akatranlp May 13, 2025 20:30
Copy link

@akatranlp akatranlp left a comment

Choose a reason for hiding this comment

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

In the file internal/storage/routing/vroom/vroom.go in the function toVroomVehicle you use the function toOrsVehicle but you now use Valhalla with Vroom instead of ORS.

There should be another config or so to differentiate between this or another idea that comes to your mind.

The problem that occurred to me was, that valhalla doesn't know the vehicle type driving-car. This should be changed to auto or so to work with the new valhalla backend for vroom.

compose.app.yaml Outdated
- GE_ROUTING_END_POINT=9.434764259345679,54.768731253913806
- GE_ROUTING_WATERING_POINT=9.434764259345679,54.768731253913806
- GE_ROUTING_VALHALLA_HOST=http://valhalla:8002
- GE_ROUTING_VALHALLA_OPTIMIZATION_VROOM_HOST=http://vroom:2525

Choose a reason for hiding this comment

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

The port of vroom is 3000, you had it bound to 2525 in the old compose.

Suggested change
- GE_ROUTING_VALHALLA_OPTIMIZATION_VROOM_HOST=http://vroom:2525
- GE_ROUTING_VALHALLA_OPTIMIZATION_VROOM_HOST=http://vroom:3000

@@ -1,5 +1,5 @@
cliArgs:
geometry: false # retrieve geometry (-g)
geometry: true # retrieve geometry (-g)

Choose a reason for hiding this comment

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

was this for testing or is this intended?

@choffmann choffmann force-pushed the feature/update-docker-flow branch from f28b774 to 3ca3db4 Compare May 21, 2025 11:47
@choffmann
Copy link
Member Author

choffmann commented May 21, 2025

I have this error from Valhalla when I create and run the infrastructure in Compose. I set it up with 1.5 GB of resources and measured a maximum of 1.2-1.3 GB with docker stats.

image

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.

2 participants