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

Ability to pass a configuration file to Meilisearch #2558

Open
curquiza opened this issue Jun 28, 2022 · 8 comments
Open

Ability to pass a configuration file to Meilisearch #2558

curquiza opened this issue Jun 28, 2022 · 8 comments
Labels
contribution accepted enhancement good first issue impacts cloud impacts docs impacts integrations
Milestone

Comments

@curquiza
Copy link
Member

@curquiza curquiza commented Jun 28, 2022

Discussed with @gmourier and following this information: meilisearch/product#85 (comment)

We want Meilisearch to accept a configuration file that would be an alternative to the command-line options you can set when launching Meilisearch, for instance

meilisearch --master-key=XXX

Requirements

Format

The file format should be TOML.

Priority order

Configuration file < Env variables < command-line options

It means the options in the configuration file can be overwritten by the environment variable set (if they exist) and that can be themself overwritten by the command-line options (if they exist)

Example

With this config file

master_key = "thisisasuper1337masterkey@@@"
db_path = "./filepath1"
http_address = "127.0.0.1:7701"
no_anayltics = true

and with the following variables and command-line options:

export MEILI_DB_PATH=./filepath2
export MEILI_HTTP_ADDR=127.0.0.1:7702

meilisearch --master-key=abcd1234 --db-path=./filepath3

The final values kept by Meilisearch will be

  • No analytics thanks to no_analytics = true set in the config file
  • The db path is filepath3 because --db-path=./filepath3 has been set as command line options. ./filepath1 and ./filepath2 are ignored.
  • The HTTP address is 127.0.0.1:7702 because set as an environment variable. The value in the config file (127.0.0.1:7701) is ignored.
  • The master key is abcd1234 set in the command-line option. The value in the config file is ignored ( thisisasuper1337masterkey@@@ ).

Config file content

The config file will have sections gathering the same kind of variable.

Example:

master_key = "thisisasuper1337masterkey@@@"
no_analytics = true

[dump]
path = "dumps"

[snapshot]
enabled = true

[ssl]
require_auth = true

Config file command line

The default config file name is config.toml and will be retrieve by Meilisearch by default in the current working directory

You can pass the config file as command-line option

meilisearch --config-file-path="./config.toml"

So the default value of config-file-path is ./meili_config.toml.

This means by default, Meilisearch will try to read ./config.toml.

If no ./config.toml file is found and no --config-file-path option is passed, Meilisearch does not throw any error.

Errors (bonus)

We need to parse and display clear errors message for the configuration scheme. e.g. https://docs.rs/miette/latest/miette/

Implementations

We could probably start by using the toml crate and declare a struct with a simple derive on it. We could start the job in the options.rs file or maybe create a new one specifically for this config file.

If you have any questions regarding the feature or the implementation, please let us know about this issue! Every feedback or questions are welcome!


For Meili people

A spec will be open once we have implemented it. Some changes can still be applied, we need to see the final implementation to create the spec.
Since we let this issue (so far) to the community, we don't know yet in which release this new feature will be available (at minimum v0.29.0, not before).

Ping @meilisearch/cloud-team @meilisearch/docs-team who can be concerned by this issue.

@curquiza curquiza added enhancement good first issue contribution accepted labels Jun 28, 2022
@curquiza
Copy link
Member Author

@curquiza curquiza commented Jun 28, 2022

Also pinging @meilisearch/integration-team who can be interesting for their devops tools 😇

@curquiza curquiza added impacts docs impacts integrations impacts cloud labels Jun 28, 2022
@YJDoc2
Copy link

@YJDoc2 YJDoc2 commented Jul 4, 2022

Hey, this is a really cool project, and I would like to contribute to this is possible. As this issue is open, can I try to work on it?

As far as I understand, https://github.com/meilisearch/meilisearch/blob/main/meilisearch-http/src/lib.rs#L30 sets up the meilisearch with its options, so my thought for this is to declare a struct with all cmd line option, parse it using toml as suggested, and then in the set up function, conditionally give cmd opts or config file opts, as by the priority mentioned. I might even declare a new struct for the purpose of combining the config file ind cmd opts, which will be given to the set up function instead of giving it the clap parsed opts directly.

The issues that I'm currently thinking of in this are :

  • the requirements state that config file values must be overridden by env vars, which must be overridden by cmd options; but as far as I have found, clap handles checking env vars and setting values, so are we supposed to simply override values of config file by the opts structure values, or parse the env vars ourselves, removing it from clap and apply the priority order?
  • Defining a separate struct to store the config file values and then combining with cmd values is tedious as well as harder to maintain, as we'll have to duplicate any new options in both. I'll try to see if we can somehow use clap for using the same struct to parse toml file, or at least if the toml lib can be used on same struct, so there won't be duplication. (Although, config file is itself a cmd option, so will have to watch out for that!)

What do you think about the above? Can I work on this issue, or should I check something else first?

Thanks :)

@irevoire
Copy link
Member

@irevoire irevoire commented Jul 6, 2022

Hello, @YJDoc2 thanks for your interest!
Yes, you can totally work on it.

Configuration file < Env variables < command-line options

For your first point, clap is already handling the last part of the problem; env < cli.
This means you’ll need to load the config file BEFORE clap and update the env-var that are unspecified but keep the already existing one in place. Then you should be able to let clap do its job.

For the second point, I would like to keep a single structure for both clap and the config file. I don’t know if it’s possible/easy but if you try to tackle this issue, I would like you to try it, and if it doesn’t work explain to us why 😇

@YJDoc2
Copy link

@YJDoc2 YJDoc2 commented Jul 7, 2022

Hey @irevoire , one of the issues for using the same struct for toml and opts is there are some fields set as skip for serializing in serde . toml crate uses serde to deserialize the , so it will not be able to read those from the file, as of now. For example, the no_analytics is set to skip, so cannot be read from config file, but in the spec above that field is used.
A way to resolve this, is to change skip to skip_serializing, which will disable serializing but will still allow reading from file. (https://serde.rs/attr-skip-serializing.htm) . One of the changes needed for that is to add default values for non-option-skip fields, which is do-able.

Another issue is that the above spec suggests that the config file should ideally group the keys in sections by type :

The config file will have sections gathering the same kind of variable.

But as the fields in opts structs are separate, they cannot be grouped in toml. For that to be possible, the similar options must be moved into a struct which then appear as a section in the toml ; but that will cause changes in all places where those variables are used.

Also, to get clarification,

So the default value of config-file-path is ./meili_config.toml.
This means by default, Meilisearch will try to read ./config.toml.

Should the default be meili_config.toml or config.toml?

Thanks :)

@irevoire
Copy link
Member

@irevoire irevoire commented Jul 7, 2022

A way to resolve this, is to change skip to skip_serializing, which will disable serializing but will still allow reading from file.

Yes, that's perfect 👍

For that to be possible, the similar options must be moved into a struct which then appear as a section in the toml ; but that will cause changes in all places where those variables are used.

Don't you think you can make your multiple structures but still make thing shows correctly in clap with a bunch of flatten?

Should the default be meili_config.toml or config.toml?

I don't think we're going to auto-import a config file. Thus you'll need to add a cli option to import the config file, and its name can be anything 😁

@YJDoc2
Copy link

@YJDoc2 YJDoc2 commented Jul 7, 2022

Don't you think you can make your multiple structures but still make thing shows correctly in clap with a bunch of flatten?

It might be possible with flatten and manual naming, but it will also do changes in code code where these are being used, as instead of opts.ssl_require_auth , it will need to do opts.ssl.require_auth and so on. Just wanted to make sure those changes would be fine as well.

@curquiza curquiza added this to the v0.29.0 milestone Jul 7, 2022
@eskombro
Copy link
Member

@eskombro eskombro commented Jul 18, 2022

Would it be too much work, and would it be interesting, to support both YAML and TOML?

YAML is the most widely used format on cloud-native applications. But TOML is indeed way easier to parse, and we don't need to handle the whitespace hell of YAML files. Just wondering if being able to handle both would make life easier for many, or if it would introduce unnecessary complexity for the development/ maintenance of the feature!

@curquiza
Copy link
Member Author

@curquiza curquiza commented Jul 18, 2022

Hello @eskombro
We want to gather discussion about the product decision in the product repository. This kind of feature request is not handled here.
Here is the dedicated discussion about it, I linked it in this issue description.

I pinged the cloud team on this discussion to discuss it, feel free to add anything you would need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution accepted enhancement good first issue impacts cloud impacts docs impacts integrations
Projects
None yet
Development

No branches or pull requests

4 participants