Prevent useless downloads of uefi-vars assets #4261
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4261 +/- ##
==========================================
- Coverage 97.93% 97.92% -0.02%
==========================================
Files 369 369
Lines 33441 33475 +34
==========================================
+ Hits 32752 32781 +29
- Misses 689 694 +5
Continue to review full report at Codecov.
|
|
Mind the decreased coverage in t/35-script_clone_job.t which should have 100% statement coverage as it's a test |
Martchus
force-pushed
the
clone-pflash-vars
branch
from
775a5ec
to
61e6555
Compare
* Test skipping asset downloads if the (grand) parent is cloned anyways * Test skipping download of uefi-vars assets * Test doing asset downloads if skipping download of dependencies * Test doing download of uefi-vars assets when a parent job is expected to generate one (has the `PUBLISH_PFLASH_VARS` setting)
* Skip the download similar to `PUBLISH_HDD_1` assets when the generating job is cloned as well anyways. * Skip the download if none of the parent jobs is actually supposed to publish the asset (corresponding `PUBLISH_PFLASH_VARS` is absent) because then the asset is not needed anyways and likely does not exist anyways. * See https://progress.opensuse.org/issues/99672 and https://progress.opensuse.org/issues/98388
So far transitive chained parents are not considered so HDDs would not be skipped as expected if they would be generated by e.g. a "grand parent" job.
to prevent the function from becoming too big.
Martchus
force-pushed
the
clone-pflash-vars
branch
from
61e6555
to
a5bc31b
Compare
| for my $j (@$parents, $job) { | ||
| for my $setting (qw(PUBLISH_HDD_1 PUBLISH_PFLASH_VARS)) { | ||
| return 1 if _job_setting_is $j, $setting => $file; | ||
| } | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
Would be nice to combine grep+map here, something like the following maybe?
| for my $j (@$parents, $job) { | |
| for my $setting (qw(PUBLISH_HDD_1 PUBLISH_PFLASH_VARS)) { | |
| return 1 if _job_setting_is $j, $setting => $file; | |
| } | |
| } | |
| return 0; | |
| grep { _job_setting_is $_, $setting => $file } … |
There was a problem hiding this comment.
But the early return makes sense and the for loops are quite easy to read (compared to squashing too much logic into one line).
| # skip downloading "uefi-vars" assets if not actually generated by | ||
| # any parent | ||
| if ($file =~ qr/uefi-vars/) { | ||
| my $parent_publishes_pflash_vars; |
There was a problem hiding this comment.
why declare the variable here and not immediately assign? If this is just to prevent line-break then maybe you need to extract more functions to reduce indentation further :)
There was a problem hiding this comment.
In Perl a variable is initialized to be undef by default. This is wanted here - although any falsy value would do. So I could assign 0. (Note that my current version is according to the coding style we generally use. We definitely don't generally use my $variable = undef.)
There was a problem hiding this comment.
I don't understand. To me it looks like you can combine this with the next line as you unconditionally assign a value there, don't you?
There was a problem hiding this comment.
The next line is a for loop and the lifetime of the variable is outside of that loop. Even if I could get away with it in Perl this seems conceptionally wrong to me.
There was a problem hiding this comment.
oooh, it's a for-loop. I misread the and last for @$parents; :D Awesome!
| # skip downloading "uefi-vars" assets if not actually generated by | ||
| # any parent | ||
| if ($file =~ qr/uefi-vars/) { | ||
| my $parent_publishes_pflash_vars; |
There was a problem hiding this comment.
I don't understand. To me it looks like you can combine this with the next line as you unconditionally assign a value there, don't you?
| # skip downloading "uefi-vars" assets if not actually generated by | ||
| # any parent | ||
| if ($file =~ qr/uefi-vars/) { | ||
| my $parent_publishes_pflash_vars; |
There was a problem hiding this comment.
oooh, it's a for-loop. I misread the and last for @$parents; :D Awesome!
See particular commit messages and https://progress.opensuse.org/issues/99672 and https://progress.opensuse.org/issues/98388.