+1

Wait... is this right? Doesn't it change the option name instead of the value?

No because the variable autorebuild_default is set to 'enabled'. disabled was never valid

autorebuild_default is the option name.

This cfp.has_option('autorebuild', 'enabled') means: read the value from a file like this:

[autorebuild]
enabled=VALUE

The output of the command is still either "true" or "false".

I feel like we'd want autorebuild_default='false', and cfp.getboolean(...) else autorebuild_default.

Why put the option name in a variable if it's not changing?

I was just following your original comment. I thought you thought it would make it more readable.

I can't explain why I wrote that originally. :-)

+1

I'm still wondering how is this going to have any effect as it only changes log output? Is there a corresponding server side change that switches the default value?

I thought that's how the stop_rebuild_if_disabled plugin worked already, but now that I look at it again I'm not so sure.

I thought that's how the stop_rebuild_if_disabled plugin worked already, but now that I look at it again I'm not so sure.

The _is_rebuild_enabled method will only set result to True if the config file is found and the value can be parsed properly to True. Otherwise, it defaults to False: https://github.com/projectatomic/atomic-reactor/blob/master/atomic_reactor/plugins/pre_stop_autorebuild_if_disabled.py#L45

@lsedlar, the changes have been done in atomic-reactor a while back: https://github.com/projectatomic/atomic-reactor/pull/616
This change is just to keep parity in rpkg to avoid confusion.
We decided to default autorebuild to False to allow a gradual adoption of the auto rebuild feature. There were concerns that enabling by default could potentially cause havoc in our infrastructure :fearful:

Thanks for the clarification, it's clearer to me now.