Merge ~alfonsosanchezbeato/network-manager:better-eth-handling into network-manager:snap-1.10

Proposed by Alfonso Sanchez-Beato
Status: Merged
Approved by: Tony Espy
Approved revision: 07d778db81356274768c9d3e4cd82dd65c47b632
Merged at revision: 5c72d5ecca966756aea09dbe347495741bc433b8
Proposed branch: ~alfonsosanchezbeato/network-manager:better-eth-handling
Merge into: network-manager:snap-1.10
Diff against target: 280 lines (+42/-97)
8 files modified
snap-common/bin/dhcp-lease-mover (+9/-9)
snap-common/bin/networkmanager (+1/-38)
snap-common/bin/snap-config.sh (+16/-19)
snap-common/bin/snap-prop.sh (+6/-13)
snap-common/bin/utils.sh (+2/-12)
snap-common/startup-hooks/99-wol-by-default.sh (+3/-3)
snap/hooks/configure (+3/-3)
snap/snapcraft.yaml (+2/-0)
Reviewer Review Type Date Requested Status
Tony Espy Approve
Review via email: mp+363488@code.staging.launchpad.net

Commit message

* Set sensible defaults for CFLAGS
* Add option to (un)set NM as default netplan renderer
* Remove ethernet.enable setting
* Check major series version to select DNS resolver
* Fix some shellcheck warnings

Description of the change

* Set sensible defaults for CFLAGS
* Add option to (un)set NM as default netplan renderer
* Remove ethernet.enable setting
* Check major series version to select DNS resolver
* Fix some shellcheck warnings

To post a comment you must log in.
Revision history for this message
Tony Espy (awe) wrote :

Some comments/questions:

 - Commit c8a1cd3 sets CFLAGS to a sensible default, but also removes 'make check'. Why? If there's a valid reason, I would argue that this should be done in a separate commit, with explanation provided in the commit message body.

 - What's the use case for dynamically changing the netplan renderer? Also have you tested the code, the configure hook plugs network-setup-observe, but this only allows read access to netplan files. Does the config hook inherit the default slot permissions of the network-manager interface?

review: Needs Information
Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

- Good catch on 'make check', I have reverted that (I usually remove that locally as it fails when I am cross-building).

- Use case for changing the renderer is to let users move back to networkd if desired. I created it so there is a way to revert the writing of the netplan file that sets NM as default renderer, thing that we want to do even if we remove this new setting.

- The writing of the netplan file is from the bin/networkmanager wrapper, not from the configuration hook - the application of the configuration happens when NM starts.

Revision history for this message
Tony Espy (awe) wrote :

Thanks for the explanations! LGTM

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches