Don't fail on missing metadata #50
Conversation
|
This is related to SUSE-Enceladus/mash#898 I don't think the failed tests for newer python versions depends on my change. Some of the tests are implemented with deprecated test concepts compared to py >= 3.8 |
|
While I agree in principal that we should be able to download images from OBS that do not have the metadata, this change introduces another problem, IMHO. As implemented one could use obs-img-utils, specify a license to exclude but because there is no metadata the image would be downloaded without error anyway leaving the user to believe that everything is just fine, when in fact it may not be. I think if any filtering conditions are applied and no metadata is available the code should exit with an error. |
yes I fully agree, good point. This condition check needs to find it's implementation in mash if I understood you correctly. So going forward it means a pull request to mash that check for the above condition must be done first prior this change to eventually be accepted correct ? |
I think the condition should be in this code base. I have not tested this but I think with this change I could us the code with the |
yes after reading the code I saw that conditions are checked here and only the status is checked in mash. Will update the PR from here. To be honest I find this utils connection into mash quite confusing |
|
how about this direction now ? This also needs to be tested first |
|
I did not look into fine grain regarding which conditions are there. So there is still the hammer which does not allow to continue if there is any sort of condition specified. However if you don't set any condition we allow to continue, which would be ok from my perspective |
|
There might be some more changes necessary here. As far as I remember the packages/reports file is used as a way to find the version/build of the image. Without that or a version/build condition the regex might fail to find the image. Need to dig in to be sure.
The code was split out since it's used by many tools aside from just Mash. Even though the design is still Mash specific. This is something I wanted to clean up to at some point. |
Uh yes you are right. There is and check_image_conditions has So at least this part seems mandatory |
|
The other condition check methods are not mandatory to my understanding |
yes there is a coupling to mash in a way that conditions are part of a mash job doc and handed over to instances of OBSImageUtil. For me this already has caused some headaches as I though it's decoupled from mash. In fact a change in OBSImageUtil can easily impact mash's functionality unnoticed. So only yours and Robert's brain prevents regressions :-) |
|
@smarlowucf ok I need to understand this better. Hope you can help me. I see two properties in
In the mash code there is one instance of access to the property can cause an exception e.g OBSImageVersionException apart from running this check I don't see what could cause the download to fail ? In fact the download would just work because it uses the name of the image file found by Thanks |
|
Hmm, and I think there is already a check that prevents calling the check_ methods without conditions no call and no check_all_conditions() and everything that follows after it. ok so long story short. With the patch from here I did the following test: from obs_img_utils.api import OBSImageUtil
downloader = OBSImageUtil(
'https://download.opensuse.org/repositories/Virtualization:/Appliances:/Images:/Testing_x86:/fedora/images/',
'kiwi-test-image-live-disk',
profile='Disk'
)
print(f'_______ {downloader.packages}')
image_source = downloader.get_image()
print(image_source)This is one of my integration testing projects, no .package no .reports file in the repos. And the the downloader does its job ( I added some debug data ) |
|
As long as in your testing the download still works it should be good. The area I was concerned is here: https://github.com/SUSE-Enceladus/obs-img-utils/blob/main/obs_img_utils/api.py#L388. But if you dig down the base file name is no longer the packages file. Instead it's the image file as long as it matches one of the known extensions. |
yes the If I don't provide an extension to the constructor of OBSImageUtil the default extension list is and that matches images not anything else. I could imagine that I can confuse OBSImageUtil at several places by providing an extension at construction time. But also that I think is subject to a refactoring |
The method get_image_packages_metadata looks up either a .report or a .packages file from the given download URI. The information if present is used in mash to check on package conditions and for the build time of the image. In case no metadata information is present the returned package dict is empty and the object instance stored build time stays at its initial 'unknown' value. Not raising an exception but sending a log warning has two effects on mash: 1. An unknown buildtime causes a download of the image in any case 2. An empty packages information causes the optionally given package based conditions in the mash job description to be not verifiable On the positive side any URL just providing an image can then be consumed by mash with the mentioned restrictions. At the moment the scope of mash is limited to OBS build images and projects which has the "withreports" flags configured. Any other source location is not consumable by mash and this commit would allow to increase the scope for mash.
smarlowucf
force-pushed
the
no_fail_on_missing_metadata
branch
from
e3c0206
to
1f9c6a4
Compare
|
re-based against main, will merge when checks pass |
The method get_image_packages_metadata looks up either a .report or a .packages file from the given download URI. The information if present is used in mash to check on package conditions and for the build time of the image. In case no metadata information is present the returned package dict is empty and the object instance stored build time stays at its initial 'unknown' value. Not raising an exception but sending a log warning has two effects on mash:
On the positive side any URL just providing an image can then be consumed by mash with the mentioned restrictions. At the moment the scope of mash is limited to OBS build images and projects which has the "withreports" flags configured. Any other source location is not consumable by mash and this commit would allow to increase the scope for mash.