-
-
Notifications
You must be signed in to change notification settings - Fork 864
Add default value of the MERCURE_TRANSPORT_URL
in Dockerfile
#768
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
Conversation
This should return the path that was changed in #766.
Sorry, but what's the point of these changes? The https://github.com/dunglas/mercure/blob/main/docs/UPGRADE.md#017 What did you achieve by using the deprecated It seems these PRs should be reverted: #766, #768, to make the Mercure configuration more transparent |
Hi, @lermontex! I agree, now it looks like not the most necessary changes. To summarize: The first PR was to refuse to use the deprecated directive The second PR, the fix, did not reverse the first, but added the declaration of the environment variable As a result of both PRs, there is a simplified configuration without using the deprecated directive. Configuring the project via the environment variable Next, I planned to make a PR to add the environment variable Perhaps this could be solved more concisely. @lermontex, do you have any ideas on how to refuse deprecated elements without BC? |
@7-zete-7, basically, you just moved the settings from the Not only does this not solve the issue of using the legacy Mercure configuration method, but it also makes the settings less obvious because you configure the Mercure transport in the Dockerfile instead of the Caddyfile (the file that is used for the default Mercure configuration) You don't have to abandon the legacy Mercure transport configuration methods, at least not until the product is stable (<v1.0.0) In an attempt to fix the BC issue, you already made a BC (#766), because these edits could affect applications using this pattern and lead to the loss of integrity of the Mercure database So sometimes you need to think twice before doing such things By the way, using I hope you understand Thanks! |
IMO, we don't apply a BC policy for these reasons:
We've made even more significant changes in the past, such as migrating from Nginx + PHP-FPM to Caddy. However, it's crucial to preserve the project's core functionality. In this case, ensuring that the Bolt database remains in a Docker volume is essential for maintaining a production-ready environment. I'm fully in favor of adopting the new Caddy configuration and renaming MERCURE_TRANSPORT_URL to MERCURE_TRANSPORT_PATH or MERCURE_BOLT_PATH, as long as the default path is stored in a Docker volume. WDYT @dunglas ? |
Thank you for the detailed description of your thoughts, @lermontex!
Mercury transport does not need to be configured in most cases. When it is required, it was preferable to change it via the Before Mercure v0.17, transport configuration in Caddyfile was a must. With the advent of support for the It is still preferable to configure Mercure transport via the
I agree, my bad. I rushed the changes and didn't pay enough attention to checking the default value for the
I agree, using multi-line environment variables is not a good idea. However, it works quite well for the environment variables |
The issue was that the database storage path was changed and this could have caused a database integrity violation, as instead of one database there would be two, in different locations (#766) I'm not saying it's a serious problem, but it was and it wasn't just a project startup error. It's very sad that you deny it instead of admitting the facts
Why don't you consider that the user can use any other transport than Bolt, which has its own configuration settings?
You should study the documentation before writing such things, much less making changes The MERCURE_TRANSPORT_URL environment variable and the transport_url directive have been deprecated. Use the new transport directive instead. https://github.com/dunglas/mercure/blob/main/docs/UPGRADE.md#017
I think these variables are probably designed to use a lot of single line directives (new line - new directive) and even with that you will have problems if you use anything more than So you should roll back your changes and not do nonsense |
@lermontex No need to be passive-aggressive... I understand that this change may have caused some inconvenience, but there was no intention to break anything. The goal of this PR was to fix an inconsistency introduced earlier and to ensure that Mercure uses a more appropriate default storage path.
For now, with this layout, we're back to the initial state. There's no urgent need to revert the PR, as I'm propose working on the next step to remove the deprecated configuration. As a reminder, we’re talking about changing this configuration:
To something like this, or another better solution:
Let’s keep the discussion focused on improving the project, and I’m confident we can move forward constructively. |
This is not the case, the Mercure transport configuration has become less obvious, since it is done in the Dockerfile, and not in the Caddyfile as it was before
There is currently no elegant way to stop using the legacy way of configuring Mercure transports while still allowing custom transports. Instead of making the configuration more complex (less obvious) it would be nice to revert the ill-considered PR and suggest alternatives (if any exist or will appear in the future) |
Yes, I saw that message. I didn't understand some of the nuances of this update and reviewed the Mercure source code regarding this change. These changes introduce native support for the I repeat: Previously, the There is no need to change the value of this environment variable in the Dockerfile, just as there was no need to do this in the Caddyfile. This can be done in the compose.yaml files, as it was before. And if a more complex configuration is needed, the Caddyfile provides the same capabilities as before. I also cannot understand your position regarding "the Mercure transport configuration has become less obvious". All Mercure directives are described in the documentation and can be used both with the initial presence of the
YAML provides very good support for multi-line strings. Multi-line values of environment variables are interpreted correctly and without problems in Caddyfile. Yes, it is not perfect, but extending the configuration of Caddy, Mercure, FrankenPHP, etc. via corresponding environment variables in compose.yaml files works correctly. Some examples of such configurations:
However, @maxhelias offers an interesting solution for moving to the new directive. And no multi-line environment variables required 😀 |
@7-zete-7, sorry, but right now it looks like this: you solved a non-existent problem, then it turned out that the problem was not solved at all, while you created another problem that you want to solve in the future, but you don’t know how yet, but you are still trying to justify your actions. Good luck in your difficult task, I am not going to prove obvious things |
After #766 the path for Mercure became
./bolt.db
, which is different from what it was before (/data/mercure.db
). This PR reverts the path back to the previous value.