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

[💡FEATURE REQUEST]: Allow merging two config files #935

Open
hugochinchilla opened this issue Apr 6, 2021 · 23 comments · Fixed by roadrunner-server/config#38
Open

[💡FEATURE REQUEST]: Allow merging two config files #935

hugochinchilla opened this issue Apr 6, 2021 · 23 comments · Fixed by roadrunner-server/config#38
Assignees
Labels
C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. help-needed-easy Call for participation: Experience needed to fix: Easy / not much P-config Plugin: Config
Milestone

Comments

@hugochinchilla
Copy link

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 spf13/viper#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.
@wolfy-j
Copy link
Contributor

wolfy-j commented Apr 6, 2021

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

@rustatian
Copy link
Member

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.

@hugochinchilla
Copy link
Author

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.

@rustatian
Copy link
Member

@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 rustatian transferred this issue from roadrunner-server/roadrunner Sep 18, 2021
@rustatian rustatian changed the title Dec 25, 2021
@rustatian rustatian transferred this issue from roadrunner-server/roadrunner-plugins Jan 16, 2022
@rustatian rustatian added C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. P-config Plugin: Config labels Jan 16, 2022
@rustatian rustatian moved this to Backlog in General Jan 16, 2022
@Kaspiman
Copy link

Kaspiman commented Feb 13, 2023

@rustatian Hello! viper ticket was closed spf13/viper#893 (comment)

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

@rustatian
Copy link
Member

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.

@zolotov88
Copy link

What about include feature? I want this)

@rustatian
Copy link
Member

rustatian commented Jun 6, 2023

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. ⚡

@zolotov88
Copy link

Where I can find your discord?

@rustatian
Copy link
Member

@zolotov88
Copy link

@rustatian Thank you!

@Kaspiman
Copy link

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

@rustatian
Copy link
Member

@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 rustatian self-assigned this Sep 10, 2023
@rustatian rustatian removed this from General Sep 10, 2023
@rustatian rustatian added the help-needed-easy Call for participation: Experience needed to fix: Easy / not much label Sep 10, 2023
@rustatian rustatian moved this to 📋 Backlog in Jira 😄 Sep 10, 2023
@rustatian rustatian moved this from 📋 Backlog to 🏗 In progress in Jira 😄 Nov 8, 2023
@rustatian rustatian added this to the v2023.3.4 milestone Nov 8, 2023
@rustatian
Copy link
Member

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

@speller
Copy link

speller commented Nov 10, 2023

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.

@rustatian
Copy link
Member

Hey @speller 👋
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
Copy link
Member

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].

@speller
Copy link

speller commented Nov 20, 2023

@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
Copy link

speller commented Nov 20, 2023

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.

@rustatian
Copy link
Member

Hey @speller 👋

Please treat it as feedback, not as blame.

Sure 😃

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

@speller
Copy link

speller commented Nov 21, 2023

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

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

@rustatian
Copy link
Member

@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 rustatian reopened this Jun 19, 2024
@rustatian
Copy link
Member

Closed mistakenly. It should be open until the experiment is finished.
This feature is planned to be out of experiment in the next RR minor version (2024.2.0) with 1 addition - included configuration can be located in any path, specified in the include section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. help-needed-easy Call for participation: Experience needed to fix: Easy / not much P-config Plugin: Config
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

6 participants