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

guides: deployment: add doc about ROOT_URL #199

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Conversation

Castavo
Copy link
Contributor

@Castavo Castavo commented May 21, 2024

No description provided.

@multun
Copy link
Contributor

multun commented May 21, 2024

I'm pretty sure more changes are needed to deploy OSRD on the internet using docker-compose. The docker compose setup is designed for development only: no authentication is supported, the front-end is served in development mode (rebuilt on the fly).

I'm not comfortable suggesting this setup unless someone puts real effort into making it safe

@Castavo Castavo force-pushed the bpt/deploy-root-url branch from c196655 to 4352e04 Compare September 11, 2024 09:00
@Castavo Castavo requested review from Khoyo and ElysaSrc and removed request for multun and bloussou September 11, 2024 09:00
@Castavo
Copy link
Contributor Author

Castavo commented Sep 11, 2024

I added a commit about the limited purpose of the docker compose deployment method

@Castavo Castavo force-pushed the bpt/deploy-root-url branch 2 times, most recently from 6273305 to 31f5986 Compare September 11, 2024 09:03
Copy link
Member

@ElysaSrc ElysaSrc left a comment

Choose a reason for hiding this comment

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

I feel these changes are not required anymore since ROOT_URL is not used anymore : the gateway serves both on the same domain and the front uses a relative path. This setup prevents the need for setting the url and also prevents a bunch of CORS issues.

@Castavo
Copy link
Contributor Author

Castavo commented Sep 11, 2024

@ElysaSrc neat ! Then should I close the PR completely or keep it to add only the detail about the docker-compose deployment being a tad unsafe / not protection ready ?

@ElysaSrc
Copy link
Member

@ElysaSrc neat ! Then should I close the PR completely or keep it to add only the detail about the docker-compose deployment being a tad unsafe / not protection ready ?

As you wish, adding a word about the docker compose deployment being unfit for production is a good idea.

@Castavo Castavo force-pushed the bpt/deploy-root-url branch from 31f5986 to 7cd5247 Compare September 25, 2024 12:36
@Castavo Castavo requested a review from ElysaSrc September 25, 2024 12:37
@Khoyo
Copy link
Contributor

Khoyo commented Sep 26, 2024

I feel these changes are not required anymore since ROOT_URL is not used anymore : the gateway serves both on the same domain and the front uses a relative path

It shouldn't be, but it is... (editoast uses it to determine the url the layers are served from)

@Castavo
Copy link
Contributor Author

Castavo commented Sep 26, 2024

Dang
So it's a bug ?

@Castavo
Copy link
Contributor Author

Castavo commented Sep 27, 2024

@ElysaSrc at least we can merge the warning

@Khoyo
Copy link
Contributor

Khoyo commented Sep 27, 2024

Discussed with @flomonster, ROOT_URL is still here because we can't give relative URLs to MapLibre... So it's probably here to stay

@ElysaSrc
Copy link
Member

Yes since it's still in use you can merge the PR like it was the first time.

Sorry for the confusion.

@Castavo Castavo force-pushed the bpt/deploy-root-url branch 2 times, most recently from b7b4bcb to 2d892ae Compare September 27, 2024 13:14
… is unsafe for production

Signed-off-by: Baptiste Prevot <[email protected]>
@Castavo
Copy link
Contributor Author

Castavo commented Sep 27, 2024

I re-added the part about ROOT_URL @ElysaSrc

@Castavo Castavo merged commit 42eabac into master Sep 27, 2024
4 checks passed
@Castavo Castavo deleted the bpt/deploy-root-url branch September 27, 2024 13:44
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