Use dedicated option to download iso and hdd with archive subcommand #6672

d3flex · 2025-08-21T18:11:25Z

Based on the history this did not meant to download other assets than 'other'
assets and it is implemeted using the asset-size-limit parameter, which was
set in a quite small size.
#1571 (comment)

With this size in place, the error is confusing and not intuitive. It makes
more sense to remove the limit in case you want to get the full assets.

Introduce ~~--all~~ no-limit to skip asset-size-limit. short -u as unlimit
keep asset-size-limit for the 'other' assets

lib/OpenQA/CLI/archive.pm

codecov · 2025-08-25T08:23:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.21%. Comparing base (7818419) to head (8c7879f).
⚠️ Report is 219 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6672      +/-   ##
==========================================
- Coverage   99.22%   99.21%   -0.01%     
==========================================
  Files         398      398              
  Lines       40876    40886      +10     
==========================================
+ Hits        40558    40567       +9     
- Misses        318      319       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

public/openqa-cli.yaml

kalikiana · 2025-08-27T09:01:31Z

lib/OpenQA/Client/Archive.pm


    $path->child('testresults', 'thumbnails')->make_path if $options->{'with-thumbnails'};
-    $self->_download_assets($url, $job, $path);
+    $self->_download_assets($url, $job, $path) if $options->{'no-limit'};


This is very misleading. --no-limit implies this is about the size. But the code no longer downloads assets by default.

I suggest to clarify what behavior we want. The PR description is not clear right now.

For instance "Only download test results by default unless --download-assets is specified".

And maybe --no-limit should really only set asset-size-limit=0? That's what I would expect as a user without digging through the code.

And if this ends up breaking compatibility it needs to be announced to a wider audience. Assuming people use this command and rely on it 🙃

just copied the answer from another comment.

by default download only testresults with limits.

download assets only with an explicit parameter which overrides the size limits. Side-effects will reset size limits for testresults as well.

it needs to be announced to a wider audience.

I will ask for reviews in the eng-testing, if this is good enough

Did this happen? 🤔 I don't see any related comments here

I also find this misleading. It would make sense to have an additional flag like $options->{'no-download'}. If you really want/need to change the default behavior than you could turn the flag around. Or you make it behave like this in case of --asset-size-limit 0 as suggested by @kalikiana (which could still be combined with my suggestion for --asset-size-limit infinity).

perlpunk · 2025-08-27T14:17:09Z

public/openqa-cli.yaml

    options:
      - asset-size-limit|l=i   --Asset size limit in bytes
      - with-thumbnails|t      --Download thumbnails as well
+      - no-limit|u      --Download all assets without size limits


Please align with the other options.

perlpunk · 2025-08-27T14:19:15Z

t/43-cli-archive.t

 use OpenQA::CLI::archive;
 use OpenQA::Test::Case;
 use OpenQA::Test::TimeLimit '10';
+use OpenQA::Utils qw(:DEFAULT assetdir);


Why are you importing everything with :DEFAULT? I think you only need assetdir(), right?

yea I just copy paste but forgot to remove it. will fix

perlpunk · 2025-08-27T14:19:48Z

t/43-cli-archive.t

+    my $target = $dir->child('archive-with-assets')->make_path;
+    my ($stdout, $stderr, @result)
+      = capture_stdout sub { $cli->run('archive', @host, '--no-limit', 99963, $target->to_string) };
+    like $stdout, qr{Downloading.*\/iso\/openSUSE-13.1-DVD-x86_64-Build0091-Media.iso},


Suggested change

like $stdout, qr{Downloading.*\/iso\/openSUSE-13.1-DVD-x86_64-Build0091-Media.iso},

like $stdout, qr{Downloading.*/iso/openSUSE-13.1-DVD-x86_64-Build0091-Media.iso},

Martchus · 2025-09-01T12:20:43Z

lib/OpenQA/Client/Archive.pm

    $options->{'asset-size-limit'} //= $default_max_asset_size;
-    $self->client->max_response_size($options->{'asset-size-limit'});
+    $options->{'no-limit'} //= 0;
+    $self->client->max_response_size($options->{all} ? 0 : $options->{'asset-size-limit'});


Where is $options->{all} set? It doesn't show up on openqa-cli archive -h and it also doesn't look like this PR is introducing it. And wouldn't it make sense to let the user simply specify --asset-size-limit 0 instead of having this special case?

Where is $options->{all} set?

variable renamed and I missed to update this. fixed.

And wouldn't it make sense to let the user simply specify --asset-size-limit 0 instead of having this special case?

Actually it makes sense, even if there is nothing wrong having two variables. To me, --asset-size-limit 0 looks unclear and confusing. Initially I was looking for a name that indicates download of isos and hdds. I went with the suggestions at the end.

Based on the history this did not meant to download other assets than 'other' assets and it is implemeted using the `asset-size-limit` parameter, which was set in a quite small size. os-autoinst#1571 (comment) With this size in place, the error is confusing and not intuitive. It makes more sense to remove the limit in case you want to get the full assets. - Introduce `--no-limit` to skip _asset-size-limit_ - by default download only testresults with limits. - download assets only with an explicit parameter which overrides the size limits. Side-effects will reset size limits for testresults as well. Signed-off-by: Ioannis Bonatakis <[email protected]>

Martchus · 2025-09-03T10:50:46Z

lib/OpenQA/Client/Archive.pm

    $options->{'asset-size-limit'} //= $default_max_asset_size;
-    $self->client->max_response_size($options->{'asset-size-limit'});
+    $options->{'no-limit'} //= 0;
+    $self->client->max_response_size($options->{'no-limit'} ? 0 : $options->{'asset-size-limit'});


Ok, so this was really just mean to be no-limit. I must still say I don't like the extra flag that makes the code ignore --asset-size-limit. It is always better to minimize interactions between flags. So I'd suggest to either:

Make this at least more explicit.

Call the flag --no-asset-size-limit to make it clear that it relates to the --asset-size-limit.

Perhaps check whether both arguments are specified and print a hard error in this case (as it makes never sense to specify both of these arguments at the same time).

Follow my previous suggestion and just allow --asset-size-limit 0. The help text could mention the special meaning of 0. That 0 means "unlimited" is conventional and we also already follow this convention in other places. Mojolicious follows this convention as well hence this way we needed zero code changes to make this actually work.

If you don't like 0 you could add code like $options->{'asset-size-limit'} eq 'infinity' ? 0 : $options->{'asset-size-limit'} and make this way instead infinity a special value.

Martchus · 2025-09-03T10:54:33Z

lib/OpenQA/Client/Archive.pm


    $path->child('testresults', 'thumbnails')->make_path if $options->{'with-thumbnails'};
-    $self->_download_assets($url, $job, $path);
+    $self->_download_assets($url, $job, $path) if $options->{'no-limit'};


I also find this misleading. It would make sense to have an additional flag like $options->{'no-download'}. If you really want/need to change the default behavior than you could turn the flag around. Or you make it behave like this in case of --asset-size-limit 0 as suggested by @kalikiana (which could still be combined with my suggestion for --asset-size-limit infinity).

kalikiana · 2025-10-20T09:55:17Z

I wonder if at this point this should be closed. Or maybe it is worth asking for feedback from a wider audience since there's no consensus on the changes?

d3flex · 2025-10-20T18:42:46Z

I wonder if at this point this should be closed. Or maybe it is worth asking for feedback from a wider audience since there's no consensus on the changes?

I will bring it up in the collab

d3flex added in progress acceptance-tests-needed Needed for code that is required to be tested on a production-like environment labels Aug 21, 2025

Wabri reviewed Aug 22, 2025

View reviewed changes

lib/OpenQA/CLI/archive.pm Outdated Show resolved Hide resolved

d3flex force-pushed the feat/new_archive_limits branch from 6c617b4 to 0422e9f Compare August 25, 2025 08:01

d3flex requested a review from Wabri August 25, 2025 08:18

perlpunk reviewed Aug 25, 2025

View reviewed changes

public/openqa-cli.yaml Outdated Show resolved Hide resolved

d3flex force-pushed the feat/new_archive_limits branch 2 times, most recently from c3ec1ac to 9bfaea4 Compare August 27, 2025 06:32

kalikiana requested changes Aug 27, 2025

View reviewed changes

d3flex force-pushed the feat/new_archive_limits branch from 9bfaea4 to 439c53d Compare August 27, 2025 11:38

d3flex removed in progress acceptance-tests-needed Needed for code that is required to be tested on a production-like environment labels Aug 27, 2025

d3flex marked this pull request as ready for review August 27, 2025 11:50

perlpunk reviewed Aug 27, 2025

View reviewed changes

d3flex force-pushed the feat/new_archive_limits branch 2 times, most recently from 70c6f1c to 6358204 Compare August 27, 2025 20:50

Martchus reviewed Sep 1, 2025

View reviewed changes

d3flex force-pushed the feat/new_archive_limits branch 3 times, most recently from 148f20f to 46e9556 Compare September 1, 2025 14:53

d3flex force-pushed the feat/new_archive_limits branch from 46e9556 to 8c7879f Compare September 1, 2025 15:27

Martchus requested changes Sep 3, 2025

View reviewed changes

	like $stdout, qr{Downloading.*\/iso\/openSUSE-13.1-DVD-x86_64-Build0091-Media.iso},
	like $stdout, qr{Downloading.*/iso/openSUSE-13.1-DVD-x86_64-Build0091-Media.iso},

Use dedicated option to download iso and hdd with archive subcommand #6672

Are you sure you want to change the base?

Use dedicated option to download iso and hdd with archive subcommand #6672

Conversation

d3flex commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perlpunk Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kalikiana commented Oct 20, 2025

Uh oh!

d3flex commented Oct 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

d3flex commented Aug 21, 2025 •

edited

Loading

codecov bot commented Aug 25, 2025 •

edited

Loading

perlpunk Aug 27, 2025 •

edited

Loading