The logic in the cli is inverted. If you pass --skip-stat to the cli, this sets createrepo_skip_stat to False, which actually blocks passing the option of the same name in the task handler. If we're only going to allow the option to block passing --skip-stat when the builder is otherwise configured to pass it, then we probably need a different cli option name, if not a different option name through the chain.

That said, it's not clear to me what we want to allow and when here. In a comment you have

True variant is not possible to reduce security risk

A True variant (modulo the logic issue above) would allow the user to force the use of --skip-stat even when the kojid config has changed createrepo_skip_stat to False. I get that this is not ideal, but is it a security risk? If --skip-stat is never a good idea for dist repos, then should we ignore the kojid option here?

Also, were we going to change the default for createrepo_skip_stat?

A few options to think about.

1. a new kojid option distrepo_skip_stat which is used instead of createrepo_skip_stat for dist repos
2. distrepo_skip_stat could default to False, or it could default to None (meaning use the value of createrepo_skip_stat)
3. --skip-stat/--no-skip-stat options in the cli that override the setting
4. perhaps some kojid config that indicates the degree of override allowed. This could be yet another option, or perhaps an explicit (non-None)distrepo_skip_stat can't be overridden
5. probably only error on the cli override if it is actually overriding in a way we want to block. I.e. if we're configured to skip-stat anyway, why complain?

I've added kojid option. None could be a bit confusing - maybe better to have it explicit? And allow both overrides from CLI.

Main handler is passing createrepo_skip_stat=createrepo_skip_stat to do_createrepo, but the method arg is now expecting distrepo_skip_stat.

The introduction of distrepo_skip_stat makes for a bit of naming confusion. It might be best to take the approach that we only use the name "distrepo_skip_stat" for the kojid option and use "createrepo_skip_stat" for the other parameter/var names involved here. In other words, maybe best to address the above by switching the do_createrepo arg back.

The handler still asserts that the createrepo_skip_stat opt is None/False, but we seem to want to allow True now.

   if self.options.distrepo_skip_stat or distrepo_skip_stat:
       cmd.append('--skip-stat')

This checks the kojid option first, so if it is True we will pass --skip-stat regardless of task parameters. This doesn't match the "override" verbage. Granted, we might want to limit the override options somehow?

I wonder if something like the following would make sense?

if self.options.distrepo_skip_stat is not None:
    skip_stat = self.options.distrepo_skip_stat
elif distrepo_skip_stat is not None:   # also maybe rename this param as mentioned above
    skip_stat = self.options.distrepo_skip_stat
else:
    skip_stat = self.options.createrepo_skip_stat
if skip_stat:
    cmd.append('--skip-stat')

i.e. the cli option allows you to override createrepo_skip_stat, but if kojid has distrepo_skip_stat that will always win. We could add an additional assertion to error on an invalid override.

Alternately, we could swap the first to stanzas and allow the cli to override.

What would be the default value in kojid.conf then? If we set None in defaults (kojid.get_options), it is a bit inconsistent with other booleans (none of them can be unset). Further, it takes ugly default from createrepo_skip_stat=True. Does it make sense like this?

if createrepo_skip_stat is not None: # renamed distrepo_skip_stat
    skip_stat = createrepo_skip_stat
else:
    skip_stat = self.options.distrepo_skip_stat:
if skip_stat:
    cmd.append('--skip-stat')

in such case we still can have distrepo_skip_stat=False as default

So:

• cli option wins, if given
• otherwise we use the new distrepo_skip_stat setting
• createrepo_skip_stat is not considered

Seems fine to me.

It doesn't address your initial concerns about limiting the override to one-way, but I'm not sure that's needed and we can always add another option for that later if it is an issue.

We still have this

if createrepo_skip_stat:
    raise koji.ParameterError("createrepo_skip_stat could be only None/False")

Even though we have an option in the cli that explicitly passes it as True

:thumbsup:

Commit 26287a8e fixes this pull-request