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

WIP: net: replace "ethtool" with a package #321

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: net: replace "ethtool" with a package #321

wants to merge 1 commit into from

Conversation

ffromani
Copy link
Collaborator

@ffromani ffromani commented May 10, 2022 •
edited

DEMO PR (please not merge!) to illustrate how it could
like to consume https://github.com/safchain/ethtool

Depends on unmerged feature (FeaturesWithState function)
atm only available on my fork (PR pending)

Fixes: #317

@ffromani
WIP: net: replace "ethtool" with a package
4c91a0a 
DEMO PR (please not merge!) to illustrate how it could
like to consume https://github.com/safchain/ethtool

Depends on unmerged feature (FeaturesWithState function)
atm only available on my fork (PR pending)

Signed-off-by: Francesco Romani <fromani@redhat.com>
@ffromani
Copy link
Collaborator Author

test failures are intentional (!!!) to make sure we don't merge without explicit action!

@ffromani
Copy link
Collaborator Author

proposed PR: safchain/ethtool#49

@jak3kaj
Copy link
Contributor

jak3kaj commented May 15, 2023

I'm glad to see refactoring with the native Go ethtool library removed many package dependencies and even simplified the code!

If we decide to go this route, I can contribute a PR which refactors #338 #342 to also use github.com/safchain/ethtool.

@ffromani
Copy link
Collaborator Author

I'm glad to see refactoring with the native Go ethtool library removed many package dependencies and even simplified the code!

If we decide to go this route, I can contribute a PR which refactors #338 #342 to also use github.com/safchain/ethtool.

I'm happy there's interest in this approach! The main and only blocker here seems that the upstream project is a bit slowmoving. I'll look for other alternatives. I'm also considering a fork tailored for ghw purposes, but I'm still considering the maintainership costs.

@ffromani
Copy link
Collaborator Author

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

@jaypipes
Copy link
Owner

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

If there isn't a viable option out there in the open source world that is well-maintained and documented, I'd be perfectly fine doing an internal/ package, yes!

@ffromani
Copy link
Collaborator Author

@jaypipes just wondering: would a minimal ethtool package (perhaps unexported, like in internal/?) tailored for the needs of the project be a welcome addition to ghw? I'd like to move forward here and I'm considering which options we have on the table.

If there isn't a viable option out there in the open source world that is well-maintained and documented, I'd be perfectly fine doing an internal/ package, yes!

perfect, my thoughts exactly. Let me try harder not to have this internal package if we can help it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

replace ethtool invocation with go packages
3 participants
@ffromani @jak3kaj @jaypipes