[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug#75145] [PATCH v2 0/1] services: network-manager: Add extra-conf
From: |
Arnaud Daby-Seesaram |
Subject: |
Re: [bug#75145] [PATCH v2 0/1] services: network-manager: Add extra-configuration-files field. |
Date: |
Thu, 09 Jan 2025 15:44:27 +0100 |
User-agent: |
mu4e 1.12.7; emacs 29.4 |
Hi,
Thank you for your patch.
45mg <45mg.writes@gmail.com> writes:
> As the first revision of this patch failed to get any attention in two weeks,
> I'm CC'ing guix-devel on this one, hoping it'll get reviewed this time. Let me
> know if I should have done anything differently; I'm still relatively new to
> contributing.
I am also new to contributing, so the following may not be fully
accurate.
I think that you did everything right. Unfortunately, some patches
"fall through the cracks". I have seen some people replying to
issues [0], so that emails would show up in people' mailboxes (similar
to what you did) and restart the discussion.
I think that it is also possible to suggest simple patches for patch
review events [1]. I do not know if proposing one's own patches is
frowned upon or not, but I would not expect it to be as long as there is
no abuse.
[0] Random example: https://issues.guix.gnu.org/64620#21
[1] Wiki page for last year:
https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024
> This patch allows users to specify configuration files for NetworkManager.
> While perhaps it would be more Guix-y to instead have a field in
> `network-manager-configuration` for every configuration option, this would be
> a monumental undertaking (look at the number of options listed in
> NetworkManager.conf(5)!). At any rate, I think any means of configuring
> NetworkManager is better than none.
I agree that an escape hatch enabling to configure NetworkManager would
be nice indeed.
> The difference from the first revision is that instead of specifying a single
> directory (file-like object) containing the configuration files (which was
> then symlinked to /etc/NetworkManager/conf.d), you now specify an alist
> mapping file names to file-like objects, like with `etc-service-type`; and
> those are then added to /etc/NetworkManager/conf.d. The rationale behind this
> change is that it doesn't rule out putting our own stuff in
> /etc/NetworkManager/conf.d. For example, if we wanted a default set of files
> in there, we could modify the procedure
> `network-manager-configuration-directory` to add the files supplied via the
> field to our default set; in the first revision, this wouldn't be possible as
> the user specifies the entire directory. (I don't know whether we'd ever
> actually want to do this, but I thought it best to leave our options open.)
On adding default files in .../conf.d/:
```````````````````````````````````````
Adding a default configuration could one day be interesting. However,
I would rather (personal opinion) see the default value in an exported
variable %default-networkmanager-files (non-contractual name) if needed.
This variable could become the default value of the field.
This way, it would be more transparent to users, and would enable them
to easily opt-out. Adding their files could be done with the following
configuration:
--8<---------------cut here---------------start------------->8---
(extra-configuration-files
(cons* `("file1" ,(plain-file ...))
...
%default-networkmanager-files))
--8<---------------cut here---------------end--------------->8---
On doing a similar job than `etc-service-type':
```````````````````````````````````````````````
As you said, you are doing something similar to `etc-service-type'. Is
there a reason not to extend [2] it directly (e.g. like
`greetd-service-type' does in `(gnu services base)')?
You could, for example, prepend "NetworkManager/conf.d/" to file names
and pass the value to `etc-service-type'. WDYT?
[2] more on service extension can be found in (guix)Service Reference:
https://guix.gnu.org/manual/devel/en/html_node/Service-Reference.html
Final thought:
``````````````
Would it make sense to allow the NetworkManager service to be extended?
(E.g to allow another service to add a configuration file.)
Note: this is not a blocker for your patch and could be done in a later
patch. It is simply a thought that I wanted to share.
Best regards,
--
Arnaud
signature.asc
Description: PGP signature