Return error when error happens in caching assets

[Amrysliu]

See: https://progress.opensuse.org/issues/65840

[okurz]

Maybe you don't even need to check for ref($shared_cache) eq 'HASH' but just rely on the second condition

[Martchus]

Seems like do_asset_caching can return undef. To me it is not clear whether this is actually an error or not as well. Maybe it makes sense to adjust do_asset_caching to always return a meaningful error message.

[Amrysliu]

@Martchus sometimes it returns a dir. Do you mean we could return a hash like this {error => , shared_dir => } in do_asset_caching?

[Martchus]

That would at least be more readable. But so far the do_asset_caching function might exit without an explicit return statement. It would be better to have an explicit return in all code paths to make it clear in any case what the function returns.

[Amrysliu]

@okurz I remain the checking ref. Because sometimes the function return a string, if I only use $shared_cache->{error}, it will report a error message Can't use string ("xxx") as a HASH ref while "strict refs".
@Martchus I just added a return undef in do_asset_cacheing, is it enough?

[Amrysliu]

If I do not miss something, the do_asset_caching will return 3 types of values: return undef, return $shared_dir (string), return {error => $error_message}. when it return {error => } we need to stop the test and return error. Although checking the perl data type is not a good idea, I have no idea how to check the value to make it more simply.

[okurz]

well, you can streamline this by replacing return undef into return {error => $another_error_message} and then use

return $shared_cache->{error} if $shared_cache->{error};

[Amrysliu]

Do you mean using return {error => $another_error_message} to replace return undef at the end of function do_asset_caching ? But there is still a situation that do_asset_caching return a string. So the if $shared_cache->{error} will report a error message that I mentioned in above message.

[Amrysliu]

BTW return undef should be treated as success.

[Martchus]

If return undef should be treated as success you can't implemant the suggestion by @okurz exactly but you can still follow the idea. In my opinion it is also better to return a consistent type and then only check for the presence of the error key to determine whether an error occurred. In the good case with no additional information you could return an empty hash.

[codecov]

Codecov Report

Merging #2960 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2960      +/-   ##
==========================================
- Coverage   93.43%   93.40%   -0.03%     
==========================================
  Files         190      190              
  Lines       11967    11969       +2     
==========================================
- Hits        11181    11180       -1     
- Misses        786      789       +3     

Impacted Files	Coverage Δ
lib/OpenQA/Worker/Engines/isotovideo.pm	93.85% <100.00%> (+0.05%)	⬆️
lib/OpenQA/Worker/Job.pm	74.88% <0.00%> (-0.48%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a52551...4179437. Read the comment docs.

[okurz]

seems from my original proposal you turned this around. Can you now check for a correct value rather than check the ref type? I don't think checking perl types is a good style here.

[okurz]

hm, we might not have a better choice here as we can't check something like if $shared_cache->{error} if $shared_cache is actually a string. I think the current way as you provided is fine

[okurz]

hm, we might not have a better choice here as we can't check something like if $shared_cache->{error} if $shared_cache is actually a string. I think the current way as you provided is fine

[Amrysliu]

I was working on returning a consistent type. :)

[okurz]

sure, no problem. If you have something better on top no problem to have another PR.