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

Add support for backing up disks during test #2341

Merged
merged 5 commits into from Aug 4, 2023

Conversation

mimi1vx
Copy link
Member

@mimi1vx mimi1vx commented Jul 11, 2023

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching backend/**.pm:

  • Consider running manual verification tests, especially for non-qemu backends

Files matching testapi.pm:

  • Consider bumping the version number in OpenQA/Isotovideo/Interface.pm for changes in behaviour

This is an automatically generated QA checklist based on modified files.

@mimi1vx mimi1vx marked this pull request as draft July 11, 2023 15:09
@okurz
Copy link
Member

okurz commented Jul 11, 2023

s/[backuping/backing up/

backend/baseclass.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
testapi.pm Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #2341 (dba364a) into master (5f3132b) will increase coverage by 0.02%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head dba364a differs from pull request most recent head 510e09d. Consider uploading reports for the commit 510e09d to get more accurate results

@@            Coverage Diff             @@
##           master    #2341      +/-   ##
==========================================
+ Coverage   95.05%   95.07%   +0.02%     
==========================================
  Files         155      155              
  Lines       15277    15340      +63     
==========================================
+ Hits        14521    14584      +63     
  Misses        756      756              
Files Changed Coverage Δ
OpenQA/Isotovideo/Interface.pm 100.00% <ø> (ø)
t/04-check_vars_docu.t 100.00% <ø> (ø)
t/23-baseclass.t 100.00% <ø> (ø)
t/data/tests/main.pm 100.00% <ø> (ø)
backend/baseclass.pm 100.00% <100.00%> (ø)
backend/qemu.pm 100.00% <100.00%> (ø)
t/03-testapi.t 100.00% <100.00%> (ø)
t/18-backend-qemu.t 100.00% <100.00%> (ø)
t/99-full-stack.t 100.00% <100.00%> (ø)
testapi.pm 87.03% <100.00%> (+0.06%) ⬆️

testapi.pm Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
testapi.pm Show resolved Hide resolved
@mimi1vx mimi1vx force-pushed the backup_drive branch 5 times, most recently from 8b74252 to 18b8304 Compare July 18, 2023 11:46
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Show resolved Hide resolved
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
@mimi1vx mimi1vx force-pushed the backup_drive branch 2 times, most recently from 64b49b3 to 2916b8d Compare July 19, 2023 09:36
@mimi1vx mimi1vx force-pushed the backup_drive branch 2 times, most recently from 1201cd7 to aabe020 Compare July 20, 2023 08:44
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
doc/memorydumps.asciidoc Outdated Show resolved Hide resolved
t/data/tests/tests/save_storage.pm Outdated Show resolved Hide resolved
@mimi1vx mimi1vx force-pushed the backup_drive branch 3 times, most recently from aae6731 to fd43775 Compare August 3, 2023 13:38
@mimi1vx mimi1vx requested a review from okurz August 3, 2023 13:39
@mimi1vx mimi1vx marked this pull request as ready for review August 3, 2023 13:39
@mimi1vx mimi1vx changed the title Add support for backuping disks during test Add support for backing up disks during test Aug 3, 2023
t/18-backend-qemu.t Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated
my $my_node = "$node-$fname";
my $bck_file = "assets_public/$my_node-$vars->{NAME}.qcow2";
# create disk
runcmd('qemu-img', 'create', '-f', 'qcow2', "$bck_file", "$size");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
runcmd('qemu-img', 'create', '-f', 'qcow2', "$bck_file", "$size");
runcmd('qemu-img', 'create', '-f', 'qcow2', $bck_file, $size);

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, for $size yes but ab $bck_file are needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this variable need quotes?

backend/qemu.pm Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
t/18-backend-qemu.t Outdated Show resolved Hide resolved
@mimi1vx mimi1vx force-pushed the backup_drive branch 2 times, most recently from 41bc7e5 to 058d3ce Compare August 3, 2023 16:06
@mimi1vx mimi1vx requested a review from Martchus August 3, 2023 16:07
backend/qemu.pm Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
doc/backend_vars.asciidoc Outdated Show resolved Hide resolved
backend/qemu.pm Outdated Show resolved Hide resolved
t/data/tests/tests/save_storage.pm Outdated Show resolved Hide resolved
testapi.pm Outdated Show resolved Hide resolved
t/99-full-stack.t Outdated Show resolved Hide resolved
@mergify mergify bot merged commit a00cbb0 into os-autoinst:master Aug 4, 2023
15 checks passed
my $my_node = "$node-$fname";
my $bck_file = "assets_public/$my_node-$vars->{NAME}.qcow2";
# create disk
runcmd('qemu-img', 'create', '-f', 'qcow2', "$bck_file", $size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does $bck_file need quotes?

Copy link
Member Author

Choose a reason for hiding this comment

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

filepath .. so can contain space and other awful characters...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure it can. But the quotes around the variable don't make any difference.
We're using perl here, not bash.

Copy link
Contributor

@perlpunk perlpunk Aug 4, 2023

Choose a reason for hiding this comment

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

Look at what happens here:

% perl -wE'
sub runcmd {
  my ($path) = @_;
  say "path: >>$path<<";
}
my $arg = "some path with spaces";
runcmd($arg);
runcmd("$arg");'
path: >>some path with spaces<<
path: >>some path with spaces<<

So in the function runcmd it will end up without quotes in any case.

If you wanted to add quotes, you would have to do qq{"$bck_file"}. But that doesn't make sense. The runcmd function will take care of it anyway. Those variables aren't passed to a shell. Otherwise we would already have a problem with existing runcmd calls.

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