Refactor: prepare base #2246
Refactor: prepare base #2246
Conversation
This reverts commit 806d189.
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on fixing file permissions for base volumes and improving ISO download handling. The changes ensure proper file permissions are set on volumes and base images, fix ISO URL handling, and improve test reliability.
- File permissions are now properly set to read-only (0400) for base volumes and read-write (0600) for regular volumes
- ISO download logic improved with better URL handling and test mode support
- Test reliability improvements including debug flag changes and conditional test execution
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Ravada/Volume/QCOW2.pm | Refactored prepare_base to use _copy/_convert methods and handle file permissions |
| lib/Ravada/Volume/Class.pm | Added _chmod helper method and integrated permission setting in prepare_base/clone workflows |
| lib/Ravada/Domain.pm | Simplified permission handling in _do_remove_base and fixed _create_filesystem parameters |
| lib/Ravada/Domain/Void.pm | Added chmod call when creating volumes |
| lib/Ravada/VM/KVM.pm | Enhanced ISO download with better URL handling, test mode support, and filename detection |
| lib/Ravada/VM.pm | Updated VM insertion to set is_active and enabled defaults |
| lib/Ravada/Request.pm | Made uid required for refresh_storage requests |
| lib/Ravada/Front.pm | Fixed ISO file_re handling with unlock_keys |
| lib/Ravada/WebSocket.pm | Moved variable initialization outside conditional block |
| lib/Ravada.pm | Updated ISO URLs, Alpine URLs restored, and improved error handling |
| t/vm/20_base.t | Added comprehensive file permission tests and refactored test_remove_base |
| t/20_volumes.t | Added file permission checks throughout volume tests |
| t/request/50_refresh_storage.t | Improved test reliability with unique filenames and retries |
| t/request/15_download.t | Added exit on failure for better test debugging |
| t/vm/v20_volatile_clones.t | Disabled debug output and commented out test_internal_shutdown |
| t/vm/s10_spinoff.t | Removed debug warn statement |
| t/lib/Test/Ravada.pm | Added NBD unload calls |
| templates/ng-templates/new_machine_template.html.ep | Fixed ISO availability condition logic |
| CHANGELOG.md | Updated with section headers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| sub _download($self, $url) { | ||
| $url =~ s{(http://.*)//(.*)}{$1/$2}; |
There was a problem hiding this comment.
This regex resolves '../' path components in URLs but lacks explanation. Add a comment: # Resolve parent directory references in URL path (e.g., 'path/dir/../file' -> 'path/file')
| $url =~ s{(http://.*)//(.*)}{$1/$2}; | |
| $url =~ s{(http://.*)//(.*)}{$1/$2}; | |
| # Resolve parent directory references in URL path (e.g., 'path/dir/../file' -> 'path/file') |
| <div ng-show="id_iso.name && ( !id_iso.device && id_iso.url ) | ||
| <div ng-show="id_iso.name && ( id_iso.url && !iso_file ) | ||
| && !id_iso.downloading | ||
| && (iso_file == '<NONE>' || !iso_file) "> |
There was a problem hiding this comment.
The condition logic has changed but the closing condition on line 58 && (iso_file == '<NONE>' || !iso_file) is redundant with line 56's !iso_file check. Line 58's condition will always be true when line 56 is true, making it unnecessary.
| && (iso_file == '<NONE>' || !iso_file) "> | |
| "> |
| my @clones; | ||
| for ( 1 .. 3 ) { | ||
| my $clone = $base->clone(name => new_domain_name, user => user_admin); | ||
| push @clones,($clone); |
There was a problem hiding this comment.
[nitpick] The parentheses around $clone in the push statement are unnecessary. Use push @clones, $clone; instead for cleaner Perl idiom.
| for ( 1 .. 3 ) { | ||
| for my $base2 ($base, @clones ) { | ||
| my $clone = $base2->clone(name => new_domain_name, user => user_admin); | ||
| push @clones2,($clone); |
There was a problem hiding this comment.
[nitpick] The parentheses around $clone in the push statement are unnecessary. Use push @clones2, $clone; instead for cleaner Perl idiom.
| push @clones2,($clone); | |
| push @clones2, $clone; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Improved download ISO and prepare base