netplan-developers team mailing list archive

[Martin Pitt <martin.pitt@xxxxxxxxxx>]

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.