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
[BUG] --dir on command line cannot override .conf #402
Comments
I think this is worth fixing and I would like to propose a generic fix that allows any command line argument to override its counterpart in the config file. @daniel-house I believe you had some concerns/questions about this use case? @jonathanspw it would be helpful if you could describe the alternatives for your use cases as well. |
The only alternative is to use I figure this might impact others as well and there's no real downside to fixing it, so this is the more ideal approach IMO. |
It doesn't seem to matter if it's a file or not. The thing is that for the
The error message is from Likewise, when getting the value (e.g. |
Does the issue exist in redis before or Not? Or it only happens in some special OS since Fedora/RH ecosystem is mentioned in the top comment? If the issue happens in redis OSS , I think it belongs to a new feature instead of a bug |
I don't see a need for this change. The default value of Line 517 in 8abeb79
NULL at Line 3280 in 8abeb79
Maybe my problem is an insufficient understanding of how this fits into |
@jonathanspw The problem isn't tied to file vs command line. The problem is the invalid value in the file. What value of dir do you have in the file? The default value is "./" which I believe always exists. How come there's a different value and how did that get into the file? |
Conversion from redis. The path can be defined as the old redis dir, for example |
OK, I get it. So maybe sed is the best you can do. Each 'dir' config does a chdir so we use the current dir in the file system to as the state. Therefore I can't see how we can fix this without breaking something else. Can you? |
I think we can fix it. The way we evaluate the options today essentially concatenates the conf file and the command line configs. Imagine that we have a config file This is a genetic enough solution that can be documented easily: "command lines configs always take precedence". |
How about just not immediately executing chdir? After setting all the config variables, then apply chdir to whatever is in the dir-field. Are there other directives that would cause a problem? For example, do we have something weird where
would break if it were replaced with
|
@PingXie @daniel-house In general, we can't just skip all but the last chdir. In general
Or any other relative walk in the file system. We can't assume We can ignore previous dirs if the last dir is an absolut path though. Otherwise we can concat them with "/". I mean for --dir d:
I'm not sure it covers all of chdir's behaviour but I hope so. Wdut? |
Describe the bug
Passing
--dir
option on command line cannot override what is already set by the config file.To reproduce
Set a non-existent path for
dir
in the config file. Start valkey, passing--dir <path/that/exists>
on command line. Valkey will fail to start.Expected behavior
CLI arguments should override the config as they are more specific.
Additional information
This is impacting the planning for the migration scripts from redis to valkey within the Fedora/RH ecosystem. I'd hoped to use /etc/sysconfig/valkey to set CLI options that are called in by the systemd service. This works for all options except for
dir
.The following slack threads have more context and the start of this discussion:
https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202807288929
https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713217457828379
https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202054590909
https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202054590909
The text was updated successfully, but these errors were encountered: