Skip to content

feat(26.04): add ethtool#881

Open
Meulengracht wants to merge 5 commits intocanonical:ubuntu-26.04from
Meulengracht:feature/ethtool
Open

feat(26.04): add ethtool#881
Meulengracht wants to merge 5 commits intocanonical:ubuntu-26.04from
Meulengracht:feature/ethtool

Conversation

@Meulengracht
Copy link
Member

Proposed changes

Add ethtool for Ubuntu Core 26

Checklist

@lczyk
Copy link
Collaborator

lczyk commented Feb 2, 2026

  • blocked on ci error preparing lxd machine in test #828. working on a fix. for now cancelled the integration tests as they would never succeed and eventually time out, and they were taking up worker allocation. will re-trigger when ci is back to a working state. apologies for the inconvenience!

@lczyk lczyk added the Blocked Waiting for something external label Feb 2, 2026
@ROCKsBot ROCKsBot requested a review from a team February 3, 2026 01:41
@lczyk
Copy link
Collaborator

lczyk commented Feb 5, 2026

@lczyk lczyk removed the Blocked Waiting for something external label Feb 5, 2026
@github-actions
Copy link

github-actions bot commented Feb 5, 2026

Test Coverage

Average Total Coverage: 66.67%


Arch: aarch64

Coverage: 66.67%

ethtool: 🌂 66.67% (1 missing)
ethtool_network-if-hooks

Arch: x86_64

Coverage: 66.67%

ethtool: 🌂 66.67% (1 missing)
ethtool_network-if-hooks

Comment on lines +16 to +17
/etc/network/if-pre-up.d/ethtool:
/etc/network/if-up.d/ethtool:
Copy link
Member

Choose a reason for hiding this comment

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

These are scripts and we should test them.

Also I can see these scripts check for /usr/sbin/ethtool so ethtool_bins should be included in the essential of this slice.

Copy link
Member Author

@Meulengracht Meulengracht Feb 9, 2026

Choose a reason for hiding this comment

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

Testing the behavior of the scripts via ifup/ifdown inside a chroot is usually brittle (needs ifupdown installed/configured, interface state, env vars, etc.), so I would not think it's the best idea, but I can maybe add tests if we really want them tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd at the very least run them with some mocked /usr/sbin/ethtool (or even without it, since the scripts seem to test -x $ETHTOOL || exit 0), just to check they run.

see #881 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

( obviously if you can test them more then please do :) , but yeah, there is a point at which you've testing /usr/sbin/ethtool and not these scripts )

@ROCKsBot ROCKsBot requested a review from a team February 8, 2026 01:41
Comment on lines +15 to +16
essential:
- ethtool_bins
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
essential:
- ethtool_bins
essential:
- base-files_bin
- dash_bins
- ethtool_bins
- sed_bins

these require dash to run + the shebang references /bin/sh so they need the symlink from base files + if-up.d/ethtool wants sed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants