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

install: Rename install -> install to-disk, peer with to-filesystem #226

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

cgwalters
Copy link
Collaborator

install: Rename install -> install to-disk, peer with to-filesystem

Take the current install verb and change it to be
bootc install to-disk, and then install-to-filesystem becomes
bootc install to-filesystem - they are obvious peers.

The main motivation here is that in the end many use cases will
want nontrivial filesystem customization, and while I like having
a fully opinionated builtin flow to write to a target disk,
we should really think of to-filesystem as a fully equal peer
of to-disk.

Signed-off-by: Colin Walters [email protected]


install-to-disk: Verify target is a block device

I saw someone get confused and think bootc install could work
on a filesystem.

Signed-off-by: Colin Walters [email protected]


install-to-filesystem: Verify target is a dir+mountpoint

Similarly to previous patch for install to-disk, verify
that the target is a directory and that it's a mountpoint (we
can't sanely support installing to a subdirectory of a filesystem).

Signed-off-by: Colin Walters [email protected]


Copy link

@sallyom sallyom left a comment

Choose a reason for hiding this comment

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

LGTM

@jerpeter1
Copy link

Possible (more simple? ) alternative: Keep bootc install, and add an error if bootc install targets a non-device, non-mountpoint.

@cgwalters
Copy link
Collaborator Author

Possible (more simple? ) alternative: Keep bootc install, and add an error if bootc install targets a non-device, non-mountpoint.

There's a bit more rationale to why to make install a standalone verb in
osbuild/bootc-image-builder#18 (comment)

@rhatdan
Copy link
Member

rhatdan commented Dec 15, 2023

Should we keep bootc install to disk and
bootc install --file-system

If we expect one to happen more often then the other?

…tem`

Take the current `install` verb and change it to be
`bootc install to-disk`, and then `install-to-filesystem` becomes
`bootc install to-filesystem` - they are obvious peers.

The main motivation here is that in the end many use cases will
want nontrivial filesystem customization, and while I like having
a fully opinionated builtin flow to write to a target disk,
we should really think of `to-filesystem` as a fully equal peer
of `to-disk`.

Signed-off-by: Colin Walters <[email protected]>
I saw someone get confused and think `bootc install` could work
on a filesystem.

Signed-off-by: Colin Walters <[email protected]>
Similarly to previous patch for `install to-disk`, verify
that the target is a directory *and* that it's a mountpoint (we
can't sanely support installing to a subdirectory of a filesystem).

Signed-off-by: Colin Walters <[email protected]>
@cgwalters
Copy link
Collaborator Author

Should we keep bootc install to disk and bootc install --file-system

That doesn't feel very different from the status quo though...and more importantly the reason there's a separate verb (i.e. CLI command) today is that the two paths have a few mutually-incompatible extra arguments (e.g. install has --filesystem but obviously can't do that with to-filesystem; it's already made), and to-filesystem has --root-mount-spec which doesn't make sense when targeting a blockdev.

Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@cgwalters cgwalters merged commit 1a36314 into containers:main Dec 15, 2023
10 checks passed
jeckersb added a commit to jeckersb/osbuildbootc that referenced this pull request Jan 10, 2024
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.

4 participants