What about configs like include, rename-command, etc?
Maybe we should determine if it is a module_config.
ref: #10285 (comment)

I thought we had a test for this (config rewrite of loaded modules added here #4848), can you look into it, and add what's needed to make it fail on that bug?

@oranagra The loadmodule configuration is rewritten successfully, so the test does not fail.
The reason for the ???? is because loadmodule is not defined in static_configs, so it is treated as a non-existent configuration when rewriting.
Just like include, rename-command, sentinel, etc.

This PR is mainly to avoid the following situation where there is an extra line of comment.

# ??? loadmodule ./helloworld.so
loadmodule ./helloworld.so

Ohh got it. So it adds warning comments when trying to read the old file, but it doesn't prevent it from injecting them to the new file (not commented) so the test passes.

So that means that this regression is harmless? (other than looking bad)

@oranagra Yes.
@zhugezy WDYT?

Ohh got it. So it adds warning comments when trying to read the old file, but it doesn't prevent it from injecting them to the new file (not commented) so the test passes.

So that means that this regression is harmless? (other than looking bad)

@oranagra Correct. Just something that can cause users' confusion. Don't have any negative effects on redis runtime.

looking more at the history of that block and the recent changes to it, the relevant discussions i could find are:

• Module Configurations #10285 (comment)
• Module Configurations #10285 (comment) (looking back at that comment i think i was wrong)

i may be missing something so i'd like to call @nichchun and @yoav-steinberg to take a look.

in retrospect, i think i'd like to revert that condition to the same as it was originally (i.e. argv == NULL, without lookupConfig).
IIUC the implication is is that if someone has module configs in a config file and he removes the module and then does a CONFIG REWRITE, redis will fail to start, right?

the alternative would be to keep the current code, but extend it with more exceptions, i.e. everything that's explicitly supported at the bottom of loadServerConfigFromString: ["include", "rename-command", "user", "loadmodule", "sentinel"]

@zhugezy can you check why the tests are failing and report?

@zhugezy are you gonna pick this up?
looks like this chance fails the module test that use CONFIG REWRITE.

    test {test 1.module load 2.config rewrite 3.module unload 4.config rewrite works} {
        # Configs need to be removed from the old config file in this case.
        r module loadex $testmodule CONFIG moduleconfigs.memory_numeric 500 ARGS
        assert_equal [lindex [lindex [r module list] 0] 1] moduleconfigs
        r config rewrite
        r module unload moduleconfigs
        r config rewrite
        restart_server 0 true false
        # Ensure configs we rewrote are no longer present
        assert_equal [r config get moduleconfigs.*] ""
    }

prints:

# Module Configuration detected without loadmodule directive or no ApplyConfig call: aborting

probably because that test involves two modules, and after the rewrite only one survives...

additionally, i'd like to make sure it properly handles the other special configs lines (rename-command, include, user, sentinal)

apparently the reason the tests fail is because the fix was broken (negated).
so when a module was unloaded, CONFIG REWRITE didn't remove the configs from the file, and then a server restart would fail.

more importantly, this bug was not harmless, a CONFIG REWRITE would have removed rename-command directive, and possibly expose a command that was meant to be hidden.
i added tests for rename-command and also ACL user.
For ACL user, the implications were that the original command got commented and then redis will inject a new one at the end of the file, instead of updating the original one (so the bug isn't severe)

would someone review my changes, so that i can merge them?

i also updated the top comment and description, please validate that.
(not a lot left from the original PR 😄)

i also updated the top comment and description, please validate that. (not a lot left from the original PR 😄)

@oranagra Looks fine. thanks!