Major refactoring of docker containers/frontend #381
Conversation
|
Please, split this PR by feature. I also suppose you do not speak all this languages. Please, only make translation to languages you know. |
|
Hi, I already split the PR in 3 different commits. Could you be more specific as to which features should be split? Regarding the translations: I speak English, Italian, French, Spanish and German. |
|
Btw, I'm unsure as to how to deal with the bundle* files. They should probably be added to the .gitignore? |
I able to review some of the changes you made, but not all. Some may require talk or changes.
Yes |
fbarbe00
force-pushed
the
gh-pages
branch
2 times, most recently
from
cc34725 to
5526a79
Compare
|
Actually Russian translation was good, just stress signs were unnecessary ;) |
|
I have reverted the changes made to the translation files. Could you be more specific with what parts of the commit you'd like me to separate? Because this is a major refactoring, it's hard to really separate things, as they often depend on each other. Also, what do you think of adding the bundle files in the .gitignore? I'm really unsure what's common practices for node projects. Last but not least, why is the main branch gh-pages 🙃 ? |
|
Make a PR for the translations. That unrelated with refactoring.
El dom, 16 feb 2025, 15:46, Fabio Barbero ***@***.***>
escribió:
… I have reverted the changes made to the translation files. Could you be
more specific with what parts of the commit you'd like me to separate?
Because this is a major refactoring, it's hard to really separate things,
as they often depend on each other.
Also, what do you think of adding the bundle files in the .gitignore? I'm
really unsure what's common practices for node projects.
Last but not least, why is the main branch gh-pages 🙃 ?
—
Reply to this email directly, view it on GitHub
<#381 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANT5DRTWYKGNNDD7KQFUC32QCXFVAVCNFSM6AAAAABXHV4PPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRRGQ4TCOJTGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
[image: fbarbe00]*fbarbe00* left a comment
(Project-OSRM/osrm-frontend#381)
<#381 (comment)>
I have reverted the changes made to the translation files. Could you be
more specific with what parts of the commit you'd like me to separate?
Because this is a major refactoring, it's hard to really separate things,
as they often depend on each other.
Also, what do you think of adding the bundle files in the .gitignore? I'm
really unsure what's common practices for node projects.
Last but not least, why is the main branch gh-pages 🙃 ?
—
Reply to this email directly, view it on GitHub
<#381 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANT5DRTWYKGNNDD7KQFUC32QCXFVAVCNFSM6AAAAABXHV4PPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRRGQ4TCOJTGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Done: #382 |
|
Please could you make 2 PR, one for the Docker / npm update. An one for the unrelated config changes. About Docker, this PR does not already do the job ? #376 About config, I see the removing the environment variables as a regression. While the main js config file refactoring is a good idea. |
|
I can definitely make another two PRs. The challenge is that the "unrelated config changes" also impact the Docker image, but I can create a separate PR for the node updates. Regarding the PR you linked, it does something similar but uses an Alpine distribution I'm not familiar with. As for the config: the current approach (scripts/replace.js) opens the src/leaflet_options.js file in plaintext and replaces hardcoded values with environment variables. This feels quite messy and doesn't allow users to have multiple backends or layers, especially when using Docker. So I'm not entirely sure what you mean by "regression," but I'm open to alternative solutions if you have any suggestions. I’d really appreciate any help in making this process smoother, as I won’t have much time to dedicate to this project beyond today. Also, instead of saying, "I have too many questions," would you mind adding them as a GitHub question? That would make things easier to track. Thanks! |
|
@frodrigo I have done a major rework of this PR, separated into multiple commits, each with its own thorough description. |
|
Concerning the docker update, the #376 look better to me, because it use an LTS. |
|
I have no problem incorporating that commit in my PR, but #365 does not use an LTS. It uses node:iron-alpine3.19, which is a modified docker base image of alpine to include node. The latest version is 3.21 (based on... Alpine 3.21), and there seem to be no "Long Term Support" versions (source). Likewise, Alpine Linux also doesn't have "LTS", they just have stable releases, and 3.21 is the latest one. So I wouldn't argue that #376 is "better" because it's an LTS, because it is not. The only advantage of using that other version may be that it is lighter, but the current image is already pretty light, and I simply don't know whether that's true. I've also never heard/used node:iron-alpine before, whereas the base alpine image is widely used. Let me know what you think. I'm personally still in favour of regular Alpine |
No, in a separate PR. So we can start by merging just this change once it is ok.
That a node LTS, not an Alpine LTS. But the current node LTS is now jod-alpine.
Yes, that more conservative for the project. But switching to a node docker image is better to control the node (LTS) version. |
|
Hi @frodrigo, I appreciate the time you're taking to review these contributions. However, I won’t be creating a new PR for this, as it’s only a small change (just two lines of code), and this PR already updates the Dockerfile further. Before making additional adjustments, I’d really appreciate seeing my other PR reviewed and merged first, as well as getting clear feedback on whether the other proposed features in this PR are likely to be accepted or if changes are needed. I also wanted to share some general feedback. Had there been a clear code of conduct in the repository, I would have gladly followed the guidelines. However, this process has taken quite a bit of time without a clear sense of direction on whether these contributions will be merged. Before yesterday, the repository hadn’t been updated since 2023, and the code didn’t fully align with the README. My goal was simply to contribute and improve things, but unfortunately, the process has been more frustrating than expected. I hope this feedback helps make things smoother for future contributors. To be more concrete:
|
|
Hi! Are there plans for merging these two PRs? |
I have plan to pick first the Dockerfile changes. |
|
So simply updating the base image and the redundant dependency? I have made #383 to speed things up |
Update critical packages following npm audit fix command
Allows the user to switche between different services
This commit allows the user to have greater control of the services, layers and overlays in the frontend. Instead of only being able to send one backend variable, which would then be replaced into a javscript file read as a text file by hardcoded values, this file can be passed to the docker container with the -v command. The README file has been updated to describe the changes.
This commit simply updates the compiled bundle files. Perhaps they should be added to the .gitignore file in the future.
|
Only this PR left! How's this one looking? |
Big ;-) What do thing, first, make a PR about the commands from packages.json and also deal with that #324 ? I am still disagree with removing the environment variables, as, to me, it is the most easy way to configure a docker container. |
|
I get the cautiousness with env variables! Environment variables are great, and having a config file as a docker volume may seem messier. However, the current way environment variables are handled is really messy: the More importantly, there is currently no way to change multiple backends from the docker container, without pulling all the code and rebuilding the docker image. I can't think of a way to incorporate that with environment variables, since there is quite a lot of information to be passed: the URL, server name, other config options... That's why a JSON config file seemed like the best idea to me. This way, anyone can pass this (optional) file to the already built docker image and use their own backend(s). Let me know what you think. Regarding #324, I'm unsure what the command actually does. I could incorporate it if you know what the change is actually about. |
|
I think the default config file should use the environment variables. And so the json config file optional. |
|
Right, so how would you approach reading the environment variables sent by the docker command? |
|
Hi @frodrigo ! Any updates on this? |
Previously, a javascript file would manually replace environment
variables in another file, by opening it as a text file. This commit now
introduces a config.json file, where the user can configure the
backends, layers, overlays, and other options.
The docker now uses Alpine 3.21, and does not install unnecessary
dependencies.
The user can now pass a custom config.json file to the docker file.
Instructions have been written in the documentation.
npm packages have also been updated.