Skip to content
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

Safer defaults for immutable znc config #30155

Merged
merged 3 commits into from Oct 7, 2017
Merged

Safer defaults for immutable znc config #30155

merged 3 commits into from Oct 7, 2017

Conversation

layus
Copy link
Member

@layus layus commented Oct 6, 2017

I just lost all the options I configured in ZNC, because the mutable config was overwritten.
I accept any suggestions on the way to implement this, but overwriting a mutable config by default seems weird. If we want to do this, we should ensure that ZNC does not allow to edit the config via the webmin when cfg.mutable is false.

I just lost all the options I configured in ZNC, because the mutable config was overwritten.
I accept any suggestions on the way to implement this, but overwriting a mutable config by default seems weird. If we want to do this, we should ensure that ZNC does not allow to edit the config via the webmin when cfg.mutable is false.
@@ -379,7 +379,8 @@ in
# If mutable, regenerate conf file every time.
${optionalString (!cfg.mutable) ''
${pkgs.coreutils}/bin/echo "znc is set to be system-managed. Now deleting old znc.conf file to be regenerated."
${pkgs.coreutils}/bin/rm -f ${cfg.dataDir}/configs/znc.conf
${pkgs.coreutils}/bin/echo "You can still recover you old config from '${cfg.dataDir}/configs/znc.conf.bak'."
${pkgs.coreutils}/bin/mv ${cfg.dataDir}/configs/znc.conf{,.bak}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would only move it, if it is not a symlink to the nix store. Otherwise a second restart will overwrite you existing configuration. This also avoids moving if no mutable configuration has ever existed.

Copy link
Member Author

@layus layus Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is never a symlink because if the file is not writeable, znc fails at startup. When it is not mutable, the file is overwritten at startup from the nix store file. Hence the issue.

Znc does not support immutable configuration files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could copy it only if it differs from the reference store file.

Copy link
Member

@Mic92 Mic92 Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would add a recognizable pattern in the file, which can be checked in the script.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patching ZNC seems the sane way to go, wit an error message when trying to change settings. I do not see how a patter would help, as probably ZNC overwrites the config on exit and on config change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not going to happen soon... znc/znc#975

I now think this is the safest way to go. Disabling immutability by default seems enough. This backup thing would have helped for me but is not that important if mutable is an opt-out.

There seems to be little need for backups if mutable becomes a voluntary opt-out.
@Mic92 Mic92 merged commit 15b7e10 into NixOS:master Oct 7, 2017
@layus
Copy link
Member Author

layus commented Oct 7, 2017

I think this is quite simple now, but changing the default may surprise old users, is there some way to warn them ?

@Mic92
Copy link
Member

Mic92 commented Oct 7, 2017

An entry in release-notes can help.

@Mic92
Copy link
Member

Mic92 commented Oct 7, 2017

That would be here:

nixos/doc/manual/release-notes/rl-1803.xml

layus added a commit to layus/nixpkgs that referenced this pull request Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants