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

ci, dev: remove frontend nginx version, dev stack with bundled front … #10502

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ElysaSrc
Copy link
Member

@ElysaSrc ElysaSrc commented Jan 23, 2025

The goal of this PR is to simplify the stack by:

  • Removing the nginx front image
  • Using the gateway front image by default
  • Removing various duplications between dockerfiles
  • Adding a script to manage all the docker compose stuff

Due to a limitation in Docker Compose not supporting targeting custom targets inside other Dockerfile in additional contexts, I've moved a part of the building system for the front on the Gateway Dockerfile. Not ideal, but does the trick.

NOTE : Before merging we should modify the protrected branch requirement

@github-actions github-actions bot added area:front Work on Standard OSRD Interface modules area:ci Work on Continous Integration Pipeline Service area:gateway labels Jan 23, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.82%. Comparing base (d651580) to head (a88e08a).
Report is 17 commits behind head on dev.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10502      +/-   ##
==========================================
+ Coverage   81.80%   81.82%   +0.01%     
==========================================
  Files        1073     1073              
  Lines      106604   106829     +225     
  Branches      730      730              
==========================================
+ Hits        87208    87410     +202     
- Misses      19357    19380      +23     
  Partials       39       39              
Flag Coverage Δ
editoast 74.28% <ø> (-0.02%) ⬇️
front 89.34% <ø> (+<0.01%) ⬆️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 88.14% <ø> (+1.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch 4 times, most recently from 0cbd80a to 26def6a Compare January 23, 2025 13:36
@ElysaSrc ElysaSrc marked this pull request as ready for review January 23, 2025 13:38
@ElysaSrc ElysaSrc requested review from a team as code owners January 23, 2025 13:38
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch 2 times, most recently from 2cb59cf to 7327fa6 Compare January 23, 2025 15:36
@flomonster flomonster self-requested a review January 23, 2025 15:57
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe we should delete the scripts/host-compose.sh file?

@emersion emersion self-requested a review January 23, 2025 17:18
docker/docker-compose.front.yml Outdated Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
osrd-compose Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
osrd-compose Outdated Show resolved Hide resolved
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from 7327fa6 to cf4852e Compare January 23, 2025 19:22
@ElysaSrc ElysaSrc requested a review from Khoyo January 23, 2025 19:23
osrd-compose Outdated Show resolved Hide resolved
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from cf4852e to 9ca9543 Compare January 23, 2025 19:35
@ElysaSrc ElysaSrc requested a review from Khoyo January 23, 2025 19:36
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from 9ca9543 to 46fc9d4 Compare January 23, 2025 19:37
osrd-compose Outdated Show resolved Hide resolved
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from 46fc9d4 to c1ea288 Compare January 23, 2025 19:41
@ElysaSrc ElysaSrc requested a review from Khoyo January 23, 2025 19:41
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch 3 times, most recently from 96338f2 to 1b70748 Compare January 24, 2025 09:31
@ElysaSrc
Copy link
Member Author

Various updates : the order of the overrides matters more than I thought, so I switched the way I pars env flags.

@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from 1b70748 to 8e8af32 Compare January 24, 2025 12:53
Copy link
Contributor

@bougue-pe bougue-pe left a comment

Choose a reason for hiding this comment

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

Thanks! ❤️

Only tested and reviewed the script, and it suits my (core debug) need at least as well as before:
./osrd-compose host up -d --build osrdyne osrd-images jaeger gateway postgres valkey rabbitmq && cd editoast && diesel migration run (then launching aside editoast no-cache & single-worker + core single-worker)
Improving a "without-back" dev-ex (no-cache, single-worker, exclude service instead of include) is another topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not naming it osrd-compose.sh?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the sake of brevity

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Oh, forgot about this remark: should we document it in the main readme, then? (I'm OK to do this, once we agree on it, if you prefer)

Copy link
Member Author

@ElysaSrc ElysaSrc Jan 24, 2025

Choose a reason for hiding this comment

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

I'd rather have it in the documentation than the readme since the instructions in the readme are still properly working for someone that is pulling the repository to start the project locally and we should cover the topic more than just "type this command".

Maybe we should add a page for "starting the development environment" with various scenarios depending on the components you'd like to work on ? Wdyt ?

osrd-compose Show resolved Hide resolved
…by default

- Use the gateway-front in the e2e tests
- Simplify the integration tests step removing gateway and front images
- Use the gateway front image for the front, removing the container front
  of the default stack
- Create a Docker compose for the front devel stack

Signed-off-by: Élyse Viard <[email protected]>
@ElysaSrc ElysaSrc force-pushed the ev/trash-nginx-front-devenv branch from 8e8af32 to a88e08a Compare January 24, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ci Work on Continous Integration Pipeline Service area:front Work on Standard OSRD Interface modules area:gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants