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.

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

distro: add FreeBSD code path for eject #4838

Merged
merged 1 commit into from Feb 14, 2024

Conversation

igalic
Copy link
Collaborator

@igalic igalic commented Feb 1, 2024

Proposed Commit Message

distro: add eject FreeBSD code path

OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation

Additional Context

Test Steps

Checklist

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Since the Dragonflybsd class inherits from the Freebsd class this actually overrides dragonfly's eject_media() function as well, which I don't think we want.

I think we also need the following:

diff --git a/cloudinit/distros/dragonflybsd.py b/cloudinit/distros/dragonflybsd.py
index 5032ff5a7..9a7a53eee 100644
--- a/cloudinit/distros/dragonflybsd.py
+++ b/cloudinit/distros/dragonflybsd.py
@@ -2,8 +2,13 @@
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
-import cloudinit.distros.freebsd
+from cloudinit.distros import Distro as base
+from cloudinit.distros import freebsd
 
 
-class Distro(cloudinit.distros.freebsd.Distro):
+class Distro(freebsd.Distro):
     home_dir = "/home"
+
+    @staticmethod
+    def eject_media(device: str) -> None:
+        base.eject_media(device)

@holmanb holmanb self-assigned this Feb 2, 2024
@igalic
Copy link
Collaborator Author

igalic commented Feb 2, 2024

yeah, we could do that, but dragonfly has camcontrol(8).

cam is very old, as you can see from the history section.

more importantly: i have misread https://man.dragonflybsd.org/?command=eject&section=ANY this is just the same (unmaintained) eject port: https://man.freebsd.org/cgi/man.cgi?eject

so, that commit message needs adapting.
I'll also add a test case. tomorrow.

@holmanb
Copy link
Member

holmanb commented Feb 6, 2024

yeah, we could do that, but dragonfly has camcontrol(8).

cam is very old, as you can see from the history section.

more importantly: i have misread https://man.dragonflybsd.org/?command=eject&section=ANY this is just the same (unmaintained) eject port: https://man.freebsd.org/cgi/man.cgi?eject

so, that commit message needs adapting. I'll also add a test case. tomorrow.

That sounds good, thanks @igalic!

@igalic
Copy link
Collaborator Author

igalic commented Feb 6, 2024

n.b.: Tomorrow has turned into much later, as I'm at a conference right now.

Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Setting to state "Request changes" state for now until commit message is updated - then I'll merge.

OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

LGTM

@holmanb holmanb merged commit 160b5ac into canonical:main Feb 14, 2024
29 checks passed
@igalic igalic deleted the fbsd/fix-eject branch February 15, 2024 10:49
blackboxsw pushed a commit that referenced this pull request Feb 15, 2024
OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation
blackboxsw pushed a commit that referenced this pull request Feb 15, 2024
OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation
blackboxsw pushed a commit that referenced this pull request Feb 16, 2024
OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation
blackboxsw pushed a commit that referenced this pull request Feb 16, 2024
OpenBSD, and NetBSD both have an eject(1), so they should be covered in
the default code path.

FreeBSD and Dragonfly however, do not have eject in base. Here, eject is
an (unmaintained) port. In base, we do however, have camcontrol(8) and
cdcontrol(1), both of which have an eject subcommand.

Let's use camcontrol(8) here.

Sponsored by: The FreeBSD Foundation
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.

None yet

2 participants