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

filesystem: guided creates logicals if in_extended #1379

Merged
merged 2 commits into from Aug 9, 2022

Conversation

dbungert
Copy link
Collaborator

@dbungert dbungert commented Aug 5, 2022

No description provided.

@dbungert dbungert requested a review from mwhudson August 9, 2022 01:30
Copy link
Collaborator

@mwhudson mwhudson left a comment

Choose a reason for hiding this comment

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

This is more or less fine, I have some quibbles about method names though :)

else:
# find what's left of the gap after adding boot
gap = gaps.within(disk, gap)
if gap is None:
raise Exception('failed to locate gap after adding boot')
return disk, gap
return gap
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this method still has the best name given the change in its signature here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a refactor branch that I want to do soon (next?). I have a slightly better name setup_gap_for_guided, but really the refactor branch should sort that out.

flag = None
if gap.in_extended:
flag = 'logical'
return self.create_partition(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aiee duelling abstractions I guess -- but maybe create_partition should handle (or at least check?) this logic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would let you delete this method, which has a funny name. This little corner doesn't feel completely thought out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I will move this to create_partition(). I actually started with that.
The lower-level add_partition() is a bit too clever for my liking (rounds up sizes!), so maybe we start to make create_partition() be the smart one.

subiquity/common/filesystem/gaps.py Show resolved Hide resolved
@dbungert dbungert merged commit 54531c2 into canonical:main Aug 9, 2022
@dbungert dbungert deleted the guided-create-logical branch August 9, 2022 17:49
@@ -78,8 +78,11 @@ def delete_filesystem(self, fs):
delete_format = delete_filesystem

def create_partition(self, device, gap, spec, **kw):
flag = kw.pop('flag', None)
if gap.in_extended:
flag = 'logical'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this yell at you if the passed flag is not None or logical?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think yelling about invalid flags is sensible.
But this could be any valid curtin partition flag I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants