Enable NRI by default #9744
Enable NRI by default #9744
Conversation
klihub
force-pushed
the
devel/enable-nri-by-default
branch
from
21a89fb
to
1d0e24b
Compare
| @@ -42,7 +42,7 @@ type Config struct { | |||
| // DefaultConfig returns the default configuration. | |||
| func DefaultConfig() *Config { | |||
| return &Config{ | |||
| Disable: true, | |||
There was a problem hiding this comment.
I think we can suggest user to use disabled_plugins to disable NRI and remove this field. Anyway, our code should handle if NRI plugin is empty.
There was a problem hiding this comment.
I haven't though of that... But what about configuration migration ? I think with the current plumbing it would not be possible to automatically migrate a 1.7 configuration (which has NRI disabled in the plugin's own configuration) to a 2.0 configuration which then would have NRI disabled by the disabling the plugin itself in the configuration's global section, because IIUC the plugin cannot alter/does not have access to it.
There was a problem hiding this comment.
Yeah. It's hard to handle migration. However, we should remove the disable in the future since it's not good patten in plugin design. (I'm still thinking that 2.0 is good slot to remove it)
There was a problem hiding this comment.
Yeah. It's hard to handle migration. However, we should remove the
disablein the future since it's not good patten in plugin design. (I'm still thinking that 2.0 is good slot to remove it)
Is it completely out of the question to update the current configuration migration infra so that, in addition to changing its own configuration, a plugin could ask to be added to the list of disabled plugins (for instance via an extra boolean returned from ConfigMigration() ) ?
There was a problem hiding this comment.
We can document it, but agreed that we should keep the config for now. There are many cases where a plugin will skip itself based on the configuration.
There was a problem hiding this comment.
agree to keep it for now new 2.0 users can enable the disable for an extra security check and we can make explicit mention in the release notes.. possibly noting to admins what they should to do to verify NRI plugins
klihub
force-pushed
the
devel/enable-nri-by-default
branch
from
1d0e24b
to
4f41ce4
Compare
Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
devel/enable-nri-by-default
branch
from
4f41ce4
to
fe24b91
Compare
As discussed this in the last community meetings, we'd like to flip NRI on by default in 2.0. The consensus was that this should be safe to do and there was no opposing opinions voiced. Hence this PR flips the default from disabled to enabled and updates the documentation accordingly.