Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

networking: main-config: introduce NETWORKING_STACK to control network exts; allow "none"; fix typo #6786

Merged
merged 2 commits into from
Jun 24, 2024
Merged

networking: main-config: introduce NETWORKING_STACK to control network exts; allow "none"; fix typo #6786

merged 2 commits into from
Jun 24, 2024

Conversation

rpardini
Copy link
Member

@rpardini rpardini commented Jun 23, 2024 •
edited by ColorfulRhino
Loading

networking: main-config: introduce NETWORKING_STACK to control network exts; allow "none"; fix typo

  • networking: exts: introduce sanity checks, debug info
    • protect against enabling networking exts directly
    • debug logging
    • fix typo in ext name
  • networking: main-config: introduce NETWORKING_STACK to control network exts; allow "none"; fix typo
    • NETWORKING_STACK can be set to none (in config phase, pre-extensions) to not-add any networking extensions
    • keep defaulting to systemd-networkd if BUILD_MINIMAL and NetworkManager otherwise
    • fix typo in extension name
    • add NETWORKING_STACK to change-tracking

FOR DOCUMENTATION:

New build option NETWORKING_STACK:
Possible values:

  • systemd-networkd: Installs the extension net-systemd-networkd by default
  • network-manager: Installs the extension net-network-manager by default
  • none or any other: Does not install any networking in the image by default

@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines Framework Framework components labels Jun 23, 2024
@rpardini
Copy link
Member Author

This subject reveals that we really need a way to use extensions to conditionally enable extensions.
We might have that one day, but for now, introduce a config variable to control networking stack.
Specifically, this allows one to set it to none and avoid fighting the default stack.
Other fixes are mostly cosmetic and trying to catch some corner case situations (eg BUILD_MINIMAL=yes EXT=net-systemd-networkd)

@rpardini rpardini marked this pull request as ready for review June 23, 2024 08:46
@rpardini rpardini requested review from a team and igorpecovnik as code owners June 23, 2024 08:46
@igorpecovnik igorpecovnik added the Needs Documentation New feature needs documentation entry label Jun 23, 2024
@igorpecovnik igorpecovnik mentioned this pull request Jun 23, 2024
Merged
@rpardini
networking: exts: introduce sanity checks, debug info
6281a1d 
- protect against enabling networking exts directly
- debug logging
- fix typo in ext name
@rpardini
networking: main-config: introduce NETWORKING_STACK to control netw…
55d71bd 
…ork exts; allow "none"; fix typo

- `NETWORKING_STACK` can be set to `none` (in config phase, pre-extensions) to not-add any networking extensions
- keep defaulting to systemd-networkd if BUILD_MINIMAL and NetworkManager otherwise
- fix typo in extension name
- add `NETWORKING_STACK` to change-tracking
@rpardini rpardini force-pushed the pr/networking-main-config-introduce-NETWORKINGSTACK-to-control-network-exts-allow-none-fix-typo branch from 9c8889d to 55d71bd Compare June 24, 2024 07:22
@igorpecovnik igorpecovnik added Ready to merge Reviewed, tested and ready for merge 08 Milestone: Third quarter release labels Jun 24, 2024
@igorpecovnik igorpecovnik merged commit 8bc4335 into armbian:main Jun 24, 2024
8 checks passed
ColorfulRhino
ColorfulRhino reviewed Jun 24, 2024
View reviewed changes
lib/functions/configuration/main-config.sh
"network-manager")
display_alert "Adding networking extensions" "net-network-manager, net-chrony" "info"
enable_extension "net-network-manager"
enable_extension "net-chrony"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I specifically separated networking from time synching to make it more modular 😅

ColorfulRhino
ColorfulRhino reviewed Jun 24, 2024
View reviewed changes
extensions/network/net-network-manager.sh
@@ -2,6 +2,11 @@
# Extension to manage network interfaces with NetworkManager + Netplan
#
function extension_prepare_config__install_network_manager() {
# Sanity check
if [[ "${NETWORKING_STACK}" != "network-manager" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sanity check might be couterproductive, since its perfectly valid to use both networking extensions at the same time if one wishes to.

@igorpecovnik
Copy link
Member

I am totally open for more fixes to this ;) Just keep documentation aligned if design changes.

@ColorfulRhino
Copy link
Collaborator

I am totally open for more fixes to this ;) Just keep documentation aligned if design changes.

The label "needs doc" is pretty good for this. Maybe we need another label "documentation finished" so that we can always find the stuff easily which does not yet have proper documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ColorfulRhino ColorfulRhino ColorfulRhino left review comments

@igorpecovnik igorpecovnik igorpecovnik approved these changes

Assignees
No one assigned
Labels
08 Milestone: Third quarter release Framework Framework components Needs Documentation New feature needs documentation entry Ready to merge Reviewed, tested and ready for merge size/medium PR with more then 50 and less then 250 lines
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rpardini @igorpecovnik @ColorfulRhino