GNU bug report logs -
#70845
[PATCH] services: Add fancontrol-service-type
Previous Next
Full log
Message #23 received at 70845 <at> debbugs.gnu.org (full text, mbox):
Hi Adrien,
Thanks for this patch! It looks pretty good, though I do have a few
small bits of feedback.
First and foremost, this service needs documentation. Could you add
that as well? Speaking of documentation, the 'documentation' field of
your Shepherd service has an extraneous bit of whitespace.
Is it absolutely vital to use root for this service? Could you instead
create a user and usergroup with only the privileges required to run
fancontrol? You may need to do something with udev so that works. I'm
not sure exactly what privileges are required, but avoiding root seems
like a good idea.
That's the only real critique I have here. Consider the rest of this
email fun ideas rather than review per se :)
We had an out-of-band exchange about this patch that I'll summarize
here for the record. I echoed the sentiments of the reviewer who
suggested exposing the fancontrol package so that users could change
it. Your response was that the configuration is generated by pwmconfig
and therefore it wouldn't make sense to provide a Scheme interface to
configuration.
I don't know this package or how it works, but would it be possible for
this service to generate that config automatically when it's first
started? If the config is customizable about generation, you could then
expose various settings through the Guix service interface for users to
modify and rewrite the file for them. That would make using
define-configuration worthwhile for more than simply the ability to
change the package.
All that aside, you should be able to expose the package setting to
users without using define-configuration.
Best,
Juli
This bug report was last modified 1 year and 35 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.