Rework script

[l0b0]

• Use logger module for debugging purposes.
• Read files from standard /etc/NetworkManager/system-connections
  directory rather than current directory, for ease of use.
• Use temporary file context manager to delete after use.
• Format using Black and isort.

[l0b0]

(I also have a few other additions like flake.nix and pre-commit if you're interested.)

[Janik-Haag]

@l0b0 thanks for the PR! Would you mind rebasing?
And sorry for the long wait, I moved apartments recently, so I was a bit preoccupied.

[l0b0]

No worries, here you go.

[l0b0]

Oh, I realize this doesn't gel with the possibility to run in /run/NetworkManager/system-connections. Let me fix that real quick.

[l0b0]

& done.

[shymega]

LGTM, no concerns from my end as a previous contributor to nm2nix.

[shymega]

Actually, there are several nitpicks.

Output-wise, each attrset's key in the list is the full path. This is confusing and should be the filename of the connection, minus .nmconnection.

Logging should be silent by default, I think it's a bit noisy.

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

Just my thoughts..

[shymega]

Output-wise, each attrset's key in the list is the full path. This is confusing and should be the filename of the connection, minus .nmconnection.

Logging should be silent by default, I think it's a bit noisy.

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

[l0b0]

Output-wise, each attrset's key in the list is the full path. This is confusing and should be the filename of the connection, minus .nmconnection.

Good idea, done.

Logging should be silent by default, I think it's a bit noisy.

Agreed, done.

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

I've skipped this one, as I think it's too much surprising behaviour. If anything, I'd expect this to take an arbitrary number of arguments listing the directories it should traverse.

[shymega]

Output-wise, each attrset's key in the list is the full path. This is confusing and should be the filename of the connection, minus .nmconnection.

Good idea, done.

Logging should be silent by default, I think it's a bit noisy.

Agreed, done.

I couldn't see the changes in-tree - have you pushed them?

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

I've skipped this one, as I think it's too much surprising behaviour. If anything, I'd expect this to take an arbitrary number of arguments listing the directories it should traverse.

Perhaps, yes. I think it should take an arbitrary number of directories though, rather than running in CWD. It feels like running in CWD could lead to exploits with specially crafted files (unlikely), or just plain up PEBKAC. Maybe use argparse for this?

[shymega]

@Janik-Haag Could you approve CI on this?

[l0b0]

I couldn't see the changes in-tree - have you pushed them?

Whoops, pushed to the wrong remote. Fixed.

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

I've skipped this one, as I think it's too much surprising behaviour. If anything, I'd expect this to take an arbitrary number of arguments listing the directories it should traverse.

Perhaps, yes. I think it should take an arbitrary number of directories though, rather than running in CWD. It feels like running in CWD could lead to exploits with specially crafted files (unlikely), or just plain up PEBKAC. Maybe use argparse for this?

I feel like this is going well beyond the scope of this branch. Feel free to add more commits, but I don't know how much more time I have to spend on this.

[shymega]

I think by default as well, it should prioritize the CWD. If the CWD isn't /etc/NetworkManager//var/run/NetworkManager, then run in two main NetworkManager state directories, if it is one of those directories, then it should scan that directory only.

I've skipped this one, as I think it's too much surprising behaviour. If anything, I'd expect this to take an arbitrary number of arguments listing the directories it should traverse.

Perhaps, yes. I think it should take an arbitrary number of directories though, rather than running in CWD. It feels like running in CWD could lead to exploits with specially crafted files (unlikely), or just plain up PEBKAC. Maybe use argparse for this?

I feel like this is going well beyond the scope of this branch. Feel free to add more commits, but I don't know how much more time I have to spend on this.

Well, the current branch now reads from a directory by default. I think this behaviour would be best left unchanged, as that's how it's been running for most people, and changing this behaviour would lead to confusion.