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

Optionally include the encryption status in the device description #1379

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Apr 17, 2024

Problem

  • The device labels in Agama do not include the encryption status
  • In YaST it is OK because it has a dedicated column with encryption status
  • In Agama we would like to have the status included directly in the label

Solution

  • Adapt the code to include the encryption status in the label

Notes

Testing

  • Tested manually

Screenshots

agama_encrypted_disk

@lslezak
Copy link
Member Author

lslezak commented Apr 17, 2024

Questions

  • Is this the right approach?
  • Which devices can be actually encrypted?
  • Should we check the encryption status for all kinds of devices or are there some cases where it does not make sense or is not supported? It could save some work for the translators...

@coveralls
Copy link

coveralls commented Apr 17, 2024

Coverage Status

coverage: 97.801% (-0.01%) from 97.815%
when pulling 60927eb on encrypted_device_label
into 600ef23 on master.

@joseivanlopez
Copy link
Contributor

Questions

  • Is this the right approach?

I think it is a valid approach.

  • Which devices can be actually encrypted?

All block devices could be encrypted, even logical volumes (Disk, Partition, Md, LvmPv, LvmLv, etc).

  • Should we check the encryption status for all kinds of devices or are there some cases where it does not make sense or is not supported? It could save some work for the translators...

In DeviceDescription there are two "branches": #formatted_device_type_label and unformatted_device_type_label. This PR covers the first branch, but I think we should add "Encryption" also in the cases of unformatted devices (e.g., "Encrypted PV of %s").

@lslezak
Copy link
Member Author

lslezak commented Apr 17, 2024

In DeviceDescription there are two "branches": #formatted_device_type_label and unformatted_device_type_label. This PR covers the first branch, but I think we should add "Encryption" also in the cases of unformatted devices (e.g., "Encrypted PV of %s").

Yes, this first commit was just a PoC and first I wanted to get the answer for the question about which devices can be encrypted. I'll add it in the following commit(s).

@dgdavid
Copy link
Member

dgdavid commented Apr 17, 2024

  • In YaST it is OK because it has a dedicated column with encryption status
  • In Agama we would like to have the status included directly in the label

Just thinking aloud: YaST could benefit from this approach too.

@lslezak
Copy link
Member Author

lslezak commented Apr 18, 2024

Possibly yes, but that's currently out of scope...

@lslezak lslezak marked this pull request as ready for review April 18, 2024 08:19
src/lib/y2storage/device_description.rb Outdated Show resolved Hide resolved
src/lib/y2storage/device_description.rb Outdated Show resolved Hide resolved
src/lib/y2storage/device_description.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@lslezak lslezak merged commit d185af5 into master Apr 18, 2024
13 checks passed
@lslezak lslezak deleted the encrypted_device_label branch April 18, 2024 12:02
lslezak added a commit to openSUSE/agama that referenced this pull request Apr 18, 2024
## Problem

- The device summary does not include the encryption status in the
device labels

## Solution

- Pass a new flag to `yast2-libstorage-ng` so the encryption status is
included in the labels
- See yast/yast-storage-ng#1379


## Testing

- Tested manually
@yast-bot
Copy link

✔️ Internal Jenkins job #1154 successfully finished
✔️ Created OBS submit request #1168841

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

5 participants