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

lvm_vg: fix size=100% leading to crash #848

Merged
merged 1 commit into from
Oct 26, 2024
Merged

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented Oct 25, 2024

lvcreate -l does not accept a '100%' parameter which currently leads to a crash. THis change automatically changes 100% to 100%FREE leading to the intended behavior.

lvcreate -l does not accept a '100%' parameter which currently leads to a crash. THis change automatically changes `100%` to `100%FREE` leading to the intended behavior.
phaer
phaer previously requested changes Oct 25, 2024
Copy link
Member

@phaer phaer left a comment

Choose a reason for hiding this comment

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

This feels a bit too magic from my perspective; I'd prefer failing loudly on non-supported values instead of preprocessing them like this.

While %FREE is probably the most commonly used suffix, it's not the same as "%" in other backends semantically. There's also %VG, %PVS and %ORIGIN.

Maybe it's good enough to just update the example to use 100%FREE?

@iFreilicht
Copy link
Contributor

iFreilicht commented Oct 25, 2024

@phaer actually, sgdisk doesn't support 100% either. It uses -0, and we actively convert from 100% as a special value.

I do somewhat agree with your sentiment, the correct solution here is to fix this todo, but I think in the context of disko, being able to just use 100% for size everywhere is a nice, user-friendly abstraction for the most common usecase, so I am in support of this change.

I would like at least one other maintainer to agree on this, I don't wanna just merge something like this without some sort of consensus.

@phaer phaer dismissed their stale review October 26, 2024 05:39

precedence with sgdisk

@phaer
Copy link
Member

phaer commented Oct 26, 2024

Thanks for noticing that @iFreilicht. Still not the biggest fan in terms of UX here, but I guess it's consistent then at least. So Lgtm!

Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

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

Alright, great!

@iFreilicht
Copy link
Contributor

@mergify queue

Copy link
Contributor

mergify bot commented Oct 26, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 58cd832

@mergify mergify bot merged commit 58cd832 into nix-community:master Oct 26, 2024
4 checks passed
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