Review: Needs Fixing
Having a different name for the well-known NetworkManager.service is ugly IMHO
-- you are going to need to sprinkle this alternative name into every piece of
software that tries to talk to it. And you can't possibly co-install it with
the .deb as they would clash on D-Bus names.
So I'd really prefer to avoid patches like this, as they are super-ugly. I
acknowledge that snapd presumably wants a canonical name like "snap.something",
but the unit could just have "Alias=NetworkManager.service" to make it
compatible with the deb.
I also have some inline comments, but they are hopefully moot with the Alias.
Diff comments:
> diff --git a/src/netplan b/src/netplan
> index bdef802..84f71c8 100755
> --- a/src/netplan
> +++ b/src/netplan
> @@ -235,6 +235,15 @@ def command_generate():
> logging.debug('command generate: running %s', argv)
> os.execv(argv[0], argv)
>
> +def change_network_manager_service(action):
If we really need this function, can you please call it
"systemctl_network_manager", as this does not really "change" the service.
> + service_name = 'NetworkManager.service'
> +
> + # If the network-manager snap is installed use its service
> + # name rather than the one of the deb packaged NetworkManager
> + if
> os.path.exists('/etc/systemd/system/snap.network-manager.networkmanager.service'):
Let's please avoid hardcoding paths here. Can you please replace this with
something like
if subprocess.call(['systemctl', '--quiet', 'is-enabled',
'snap.network-manager.networkmanager.service'], stderr=subprocess.DEVNULL) == 0
> + service = 'snap.network-manager.networkmanager.service'
> +
> + subprocess.check_call(['systemctl', 'stop', '--no-block', service_name])
>
> def command_apply(): # pragma: nocover (covered in autopkgtest)
> if subprocess.call([path_generate]) != 0:
--
https://code.launchpad.net/~morphis/netplan/+git/netplan/+merge/306607
Your team Developers of netplan is subscribed to branch netplan:master.
--
Mailing list: https://launchpad.net/~netplan-developers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~netplan-developers
More help : https://help.launchpad.net/ListHelp
