[WIP] Search more needle directory #1569
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1569 +/- ##
==========================================
+ Coverage 60.38% 60.41% +0.02%
==========================================
Files 56 56
Lines 6548 6545 -3
==========================================
Hits 3954 3954
+ Misses 2594 2591 -3
Continue to review full report at Codecov.
|
|
So if I understand the logic correctly, this might be better phrased as "Fallback to initial productdir if the needledir is not found", where #3578 is needed to record the initial productdir in case it gets overwritten |
There was a problem hiding this comment.
The commit message could have a bit more details, e.g. what issue is fixed, why this needs to change, to which versions of which product it applies, link to a bug or a fate entry, the choices you made, etc. Also see https://commit.style/ or http://chris.beams.io/posts/git-commit/ as a helpful guide how to write good commit messages.
Also I would appreciate if you can look into adding tests.
| @@ -327,6 +327,7 @@ sub default_needles_dir { | |||
| sub init { | |||
| $needles_dir = ($bmwqemu::vars{NEEDLES_DIR} // default_needles_dir); | |||
| $needles_dir = File::Spec->catdir($bmwqemu::vars{CASEDIR}, $needles_dir) unless -d $needles_dir; | |||
| $needles_dir = "$bmwqemu::vars{INIT_PRODUCTDIR}/needles" unless -d $needles_dir; | |||
There was a problem hiding this comment.
hm, I am thinking that this isn't a very clear design because the variable INIT_PRODUCTDIR has no meaning within the scope of os-autoinst and having an incorrect PRODUCTDIR specified for os-autoinst is also not something that makes much sense. Keep in mind that isotovideo as standalone tool cares only about a single test with no history of "previous jobs" or anything. As the "check for a needle directory" is actually only a check if a directory with name "needles" exists I think here it would be better if you move the corresponding logic to openQA code. So it would be like the openQA worker code tries to "repair" the variable PRODUCTDIR in https://github.com/os-autoinst/openQA/pull/3578/files#diff-53098d895ef28f5c7db596e2dc505d7c8ee24b6a9a4cca23865fb225e10cd9fcR332 if otherwise no needle can be found.
There was a problem hiding this comment.
In general, we don't need INIT_PRODUCTDIR, but when the PRODUCTDIR is a relative path (for example the job is trigger by openqa-clone-custom-git-refspec), we need this to find the needles_dir.
I think here it would be better if you move the corresponding logic to openQA code.
if we move this logic to openQA code, when users specify NEEDLES_DIR as a GitHub or GitLab path, it will not work. Because the git check out happens in checkout_git_repo_and_branch on https://github.com/os-autoinst/os-autoinst/blob/master/OpenQA/Isotovideo/Utils.pm#L50
Amrysliu
changed the title
|
So what I am considering: As stated in #1569 (comment) this PR is trying to fix a real problem based on an already suboptimal design of the interface between os-autoinst and openQA. Hence I created https://progress.opensuse.org/issues/80372 to solve that. But I am tending to accept this PR as a "temporary workaround" – which by nature tend to stay forever – but at least we should do the following:
@Amrysliu can you do that? |
|
we already have #1577, so close this |
Related: os-autoinst/openQA#3578
Ticket: https://progress.opensuse.org/issues/67723