Fix missing_assets to ignore repos, not 'hidden assets' #3017
Conversation
|
Failure looks like a test flake (my other current PR, which touches something completely different, has a similar failure). |
| my @relevant_assets | ||
| = grep { $_->{name} ne '' && !OpenQA::Schema::Result::Assets::is_type_hidden($_->{type}) } values %$assets; | ||
| = grep { $_->{name} ne '' && $_->{type} ne 'repo' } values %$assets; |
There was a problem hiding this comment.
@Martchus do you know if we need this for other assets, e.g. the uefi vars images?
There was a problem hiding this comment.
On our production instances we don't configure hide_asset_types so it is just repo anyways. So this change makes no difference for us.
| my @relevant_assets | ||
| = grep { $_->{name} ne '' && !OpenQA::Schema::Result::Assets::is_type_hidden($_->{type}) } values %$assets; | ||
| = grep { $_->{name} ne '' && $_->{type} ne 'repo' } values %$assets; |
There was a problem hiding this comment.
On our production instances we don't configure hide_asset_types so it is just repo anyways. So this change makes no difference for us.
|
Good points about the tests, I'll fix that up today. |
As discussed in os-autoinst#2676 (comment) and follow-ups, this check misunderstands the "hidden" attribute. The code assumes that "hidden" assets are the same thing as "assets we don't really want to copy down when cloning jobs", but they are not. The "hidden" attribute was written (by me) to mean "asset types not to be shown for downloading in the web UI". For SUSE, it happens to be the case that ["repo"] would be the right array of both "not-to-be-shown-in-the-web-ui assets" and "non-clonable assets", so this bug wasn't apparent, as SUSE deployments leave the `hide_asset_types` config setting at its default value of just 'repo'. But on Fedora deployments, this setting is changed to 'repo iso hdd' (because we don't want to show those asset types for download in the web UI), so this code also ignored ISO and HDD assets when checking for "missing" assets, which we don't want. As discussed in the pull request we could potentially make this a configurable attribute and have the clone_job script use it too, but doing that is a bit harder, and I don't think for now anyone wants a different definition of "non-clonable assets", so it doesn't seem really necessary. Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill
force-pushed
the
fix-missing-asset-hidden
branch
from
4a09ded to
43c290f
Compare
|
OK, test revised as per @Martchus 's suggestions. |
Codecov Report
@@ Coverage Diff @@
## master #3017 +/- ##
=======================================
Coverage 92.03% 92.03%
=======================================
Files 211 211
Lines 12846 12846
=======================================
Hits 11823 11823
Misses 1023 1023
Continue to review full report at Codecov.
|
As discussed in
#2676 (comment)
and follow-ups, this check misunderstands the "hidden" attribute.
The code assumes that "hidden" assets are the same thing as
"assets we don't really want to copy down when cloning jobs",
but they are not. The "hidden" attribute was written (by me) to
mean "asset types not to be shown for downloading in the web UI".
For SUSE, it happens to be the case that ["repo"] would be the
right array of both "not-to-be-shown-in-the-web-ui assets" and
"non-clonable assets", so this bug wasn't apparent, as SUSE
deployments leave the
hide_asset_typesconfig setting at itsdefault value of just 'repo'. But on Fedora deployments, this
setting is changed to 'repo iso hdd' (because we don't want to
show those asset types for download in the web UI), so this code
also ignored ISO and HDD assets when checking for "missing"
assets, which we don't want.
As discussed in the pull request we could potentially make this
a configurable attribute and have the clone_job script use it
too, but doing that is a bit harder, and I don't think for now
anyone wants a different definition of "non-clonable assets", so
it doesn't seem really necessary.
Signed-off-by: Adam Williamson awilliam@redhat.com