roadrunner icon indicating copy to clipboard operation
roadrunner copied to clipboard

[πŸ’‘FEATURE REQUEST]: Allow merging two config files

Open hugochinchilla opened this issue 4 years ago β€’ 23 comments

In roadrunner 1.x we could have two different config files, one for production and other for develpment.

An example development config file looked like this:

include:
  - .rr.yaml

reload:
  interval: 1s
  patterns: [".php"]

Now the include block is not handled as spf13/viper does not have this feature implemented, (but is requested https://github.com/spf13/viper/pull/893).

As a workaround for this problem I can suggest two solutions:

  • Allow passing --config multiple times on the command line and merge the configuration from multiple files.
  • Add a key enabled under reload, so I can call the server with -o reload.enabled=false in production.

hugochinchilla avatar Apr 06 '21 13:04 hugochinchilla

FYI, we implemented this feature in RR 1 itself. Not via Viper.

wolfy-j avatar Apr 06 '21 13:04 wolfy-j

The ticket in the Discuss stage, so we can discuss, what option fits us and the community better. I guess that we should wait for the viper PR merged and try to implement let's say "native" includes. Also, we had a discussion, that will be good to have enabled option in every plugin.

rustatian avatar Apr 06 '21 14:04 rustatian

I would like having an enabled option in every plugin just because of the symmetry. This on it's own would suffice to have reload disabled in production.

But I would also like to be able to have very different logging configurations in each environment and this can be cumbersome with the -o options, so having an include mechanism would be the optimal solution.

hugochinchilla avatar Apr 07 '21 08:04 hugochinchilla

@hugochinchilla Yeah, I agree to include enabled keys to every plugin. As for the 'includes', let's wait for some time for the viper ticket to get merged (let's say 1-2 weeks) and then return to the discuttion. I plan it to 2021-05-11 release cycle.

rustatian avatar Apr 08 '21 09:04 rustatian

@rustatian Hello! viper ticket was closed https://github.com/spf13/viper/pull/893#issuecomment-890258466

"Allow merging config files" will come in viper v2. Maybe. Milestone for v 2.0 looks abandoned

Kaspiman avatar Feb 13 '23 15:02 Kaspiman

Hey @embargo2710 πŸ‘‹πŸ» Yeah, I saw this 😿 It might be an excellent feature to avoid some boilerplate configuration for the dev/prod/staging/etc.

But, to say in truth, I might implement this without waiting for the viper v2 πŸ˜‰ Just come to our open planning session (RR streams channel in our discord server) and send this ticket πŸ˜ƒ We will discuss it and prioritize.

rustatian avatar Feb 13 '23 16:02 rustatian

What about include feature? I want this)

zolotov88 avatar Jun 06 '23 15:06 zolotov88

Hey @zolotov88 πŸ‘‹πŸ» Hit the πŸ‘πŸ» button πŸ˜‰ Join our discord, attend the RR planning session (we do them online at twitch.tv/rustatian) and bring this ticket, we will discuss it and include it in some of the upcoming sprints. ⚑

rustatian avatar Jun 06 '23 16:06 rustatian

Where I can find your discord?

zolotov88 avatar Jun 06 '23 20:06 zolotov88

---> https://github.com/roadrunner-server

rustatian avatar Jun 06 '23 20:06 rustatian

@rustatian Thank you!

zolotov88 avatar Jun 06 '23 20:06 zolotov88

@rustatian Maybe there is still a time in the next release for this task?

Kaspiman avatar Aug 15 '23 13:08 Kaspiman

@Kaspiman I guess this is fairly easy to implement:

  1. Introduce a new key: include with the paths.
  2. Use temporary instances of the viper to read configurations by paths.
  3. Use AllKeys method (or other which allow you iterating over the key-vals) in every tmp viper instance. While iterating, set key-val pairs to the original configuration using Set method.
  4. This should be done before this line: https://github.com/roadrunner-server/config/blob/master/plugin.go#L71
  5. Done πŸ˜ƒ

As for the p.2 and temporary instances: I don't know if that possible to add configurations to the single instance (as far as I know - no).

rustatian avatar Aug 15 '23 16:08 rustatian

This feature will be in the next release under the -e (experimental) RR flag.

rustatian avatar Nov 08 '23 14:11 rustatian

Maybe I'm late, but based on a rich experience working with complex docker compose yaml configurations, I would expect the following merging functionality (based on the existing Docker compose functionality and requested by the community):

  • Override and extend any part of the config, including nested ones (both key-value and ordinal entries). I.e. I can add a new named value to anywhere, I can add a new item to an ordinal array.
  • Replace the entire array - both named and ordinal. I.e. I can replace some entire section or whole array without overriding every existing value.
  • Delete the entire array - both named and ordinal. I.e. I can delete some section or array as if it was never declared.

speller avatar Nov 10 '23 03:11 speller

Hey @speller :wave: Documentation for this experimental feature is here: https://roadrunner.dev/docs/experimental-experimental/current/en

  1. With the included config, you can override any of the root configuration values.
  2. Also supported.
  3. Not supported.

rustatian avatar Nov 10 '23 08:11 rustatian

Guys, while the issue is closed, but the discussion is not locked, feel free to send your feedback here or on our Discord server [RR channel].

rustatian avatar Nov 10 '23 10:11 rustatian

@rustatian Unfortunately, it doesn't work as expected for me. This is why I've wrote down my expectation in the details. My use case as an example:

I have a "production" rr.yaml file, then, in the dev move, I want to override/add/change something without duplicationg the code. I try the following command:

rr serve -e -c .rr.yaml -o include=".rr.dev.yaml"

With the following dev config:

version: '3.8'

server:
  env:
    - XDEBUG_TRIGGER: 1

And I get the following error:

handle_serve_command: Init error:
        server_plugin_init: command should not be empty

So the extension or merging of config doesn't work as the majority of users who have experience working with docker compose yaml configs and with Symfony yaml configs will expect.

Here I want to add just a single environment variable and it breaks the whole config making RR not working at all.

Also, I would expect passing multiple config files either this way:

rr serve -c .rr.yaml -c .rr.dev.yaml

or

rr serve -e -c .rr.yaml .rr.dev.yaml

without unintuitive workarounds with include.

Please treat it as feedback, not as blame.

speller avatar Nov 20 '23 12:11 speller

Additionally, I would expect a way to control the resulting configuration after merging - a dump function, or printing it to the console output on startup, or anything else. To see what RR got after its internal processing and is it what I intended to have.

speller avatar Nov 20 '23 12:11 speller

Hey @speller πŸ‘‹

Please treat it as feedback, not as blame.

Sure πŸ˜ƒ

What is in your .rr.yaml file? I mean, server section.

rustatian avatar Nov 20 '23 12:11 rustatian

@rustatian It's a very common PHP section from samples or documentation:

server:
  command: "php index.php"
  env:
    - APP_RUNTIME: Baldinof\RoadRunnerBundle\Runtime\Runtime

speller avatar Nov 21 '23 02:11 speller

@speller, Yeah, only env is different, and thus it should be applied. I guess this is just how viper (library I use under the hood) works. I'll check what I can do with that.

rustatian avatar Nov 21 '23 09:11 rustatian