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

Do not mark ancestors of device with source or stage2 as protected #5687

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Jun 3, 2024

No description provided.

@rvykydal rvykydal added the f41 label Jun 3, 2024
@rvykydal rvykydal marked this pull request as draft June 3, 2024 14:41
@rvykydal
Copy link
Contributor Author

rvykydal commented Jun 3, 2024

/kickstart-test --testtype smoke

@rvykydal rvykydal marked this pull request as ready for review June 4, 2024 11:38
Copy link
Contributor

@jstodola jstodola left a comment

Choose a reason for hiding this comment

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

It looks OK to me.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a nitpicks below.

However, please fix the Resolves in the commit message to point to RHEL issue and not BZ :).

@@ -285,6 +285,7 @@ def _mark_protected_devices(self):
identify protected devices.
"""
protected = []
protected_w_ancestors = []
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what w means here. Is it with or without? I would like to not use this abbreviations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -353,23 +358,31 @@ def protect_devices(self, protected_names):
# Update the list.
self.protected_devices = protected_names

def _mark_protected_device(self, device):
def _mark_protected_device(self, device, ancestors=False):
Copy link
Member

Choose a reason for hiding this comment

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

Could we please name the boolean to make it's obvious it's not a list of ancestors. I would suggest protect_ancestors or include_ancestors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


def _mark_unprotected_device(self, device):
def _mark_unprotected_device(self, device, ancestors=False):
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Resolves: RHEL-35701

I am not able to find any reason why we started to protect also
ancestors of the devices in this case (ie disk in case of source on a
partition).

The change (marking also ancestors of protected devices as protected in
general) was introduced during modularization of Payload.

commit 93fdbff

commit 26de57d

commit 19703fc
commit 71dda92

The patch tries to limit the changes to the case of the issue. From what
I was able to look at it seems that it should be OK not to include
ancestors also in other cases.
@rvykydal rvykydal force-pushed the rhel-35701-hdd-protecting-ancestor-upstream branch from fa67045 to 4779d5f Compare June 20, 2024 07:02
@rvykydal
Copy link
Contributor Author

/kickstart-test --testtype smoke

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rvykydal rvykydal merged commit c2c7e87 into rhinstaller:master Jun 25, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants