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