fix: use correct encryption_key parameter, improve error messages#601
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns encryption-related parameter names between volume and pool structures, updates how LUKS encryption details are propagated, and refines several BlivetAnsibleError messages for clarity and to avoid dereferencing missing pool devices. Sequence diagram for applying encryption parameters in _look_up_devicesequenceDiagram
participant BlivetVolume
participant Pool
participant ParentDevice
participant LUKSDevice
BlivetVolume->>Pool: get(encryption_password)
Pool-->>BlivetVolume: passphrase
BlivetVolume->>Pool: get(encryption_key)
Pool-->>BlivetVolume: key_file
loop for each parent in device.parents
BlivetVolume->>ParentDevice: inspect parent.format.type
ParentDevice-->>BlivetVolume: type = luks
alt passphrase is set
BlivetVolume->>LUKSDevice: open_with_passphrase(passphrase)
LUKSDevice-->>BlivetVolume: opened
else key_file is set
BlivetVolume->>LUKSDevice: open_with_key_file(key_file)
LUKSDevice-->>BlivetVolume: opened
end
end
Updated class diagram for volume and pool encryption metadataclassDiagram
class BlivetVolume {
dict _volume
dict _pool
BlivetPool _blivet_pool
_update_from_device(param_name)
_look_up_device()
_get_params_create_vdo()
_create()
}
class VolumeEncryptionMetadata {
bool encryption
int encryption_key_size
string encryption_key
string encryption_cipher
string encryption_luks_version
}
class PoolEncryptionMetadata {
bool encryption
int encryption_key_size
string encryption_key
string encryption_cipher
string encryption_luks_version
string encryption_password
}
class BlivetPool {
Device _device
}
class Device {
list parents
Format format
string name
Size free_space
}
class Format {
string type
int key_size
string key_file
string cipher
string luks_version
}
BlivetVolume --> BlivetPool : uses
BlivetPool --> Device : wraps
Device --> Format : has
BlivetVolume .. VolumeEncryptionMetadata : stores_in__volume
BlivetVolume .. PoolEncryptionMetadata : stores_in__pool
VolumeEncryptionMetadata <.. Format : derives_from
PoolEncryptionMetadata <.. Format : derives_from
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_get_params_create_vdo, the updated error message now interpolatespool_size, but if_trim_sizeraisesBlivetAnsibleErrorbefore assigning topool_size, this will cause anUnboundLocalError; consider computing and storing the human‑readable size before the try, or guarding againstpool_sizebeing undefined in the exception path. - The renames from
encryption_key_filetoencryption_keyand fromencryption_passphrasetoencryption_passwordin_update_from_deviceand_look_up_deviceshould be checked for consistency with other uses of these keys within this module to avoid mismatched dictionary lookups at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_get_params_create_vdo`, the updated error message now interpolates `pool_size`, but if `_trim_size` raises `BlivetAnsibleError` before assigning to `pool_size`, this will cause an `UnboundLocalError`; consider computing and storing the human‑readable size before the try, or guarding against `pool_size` being undefined in the exception path.
- The renames from `encryption_key_file` to `encryption_key` and from `encryption_passphrase` to `encryption_password` in `_update_from_device` and `_look_up_device` should be checked for consistency with other uses of these keys within this module to avoid mismatched dictionary lookups at runtime.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
==========================================
- Coverage 16.54% 10.33% -6.22%
==========================================
Files 2 8 +6
Lines 284 2023 +1739
Branches 79 0 -79
==========================================
+ Hits 47 209 +162
- Misses 237 1814 +1577
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
self._blivet_pool._device is None in these error paths
It's 'encryption_key' for key file and 'encryption_password' for password/passphrase.
bdfff32 to
b89cafa
Compare
|
[citest] |
Summary by Sourcery
Align encryption parameter names with key-based encryption handling and clarify pool-related error messages in blivet volume operations.
Bug Fixes:
Enhancements: