QA (public) » openQA Project (public)

If the needles subdirectory contains needles not present on the instance, then openQA will not find them.
Setting NEEDLES_DIR to needles/ on the other hand does not work.

We need to assume by default that needles are not part of the Git repository referenced by a Git URL specified via CASEDIR. That's simply because for our openSUSE test distribution we keep needles separately. Hence it is expected that specifying a custom CASEDIR still loads default needles and any relative paths are also referring to the default needles dir. I've been adding an explanatory comment to the relevant code: https://github.com/os-autoinst/openQA/blob/master/lib/OpenQA/Worker/Engines/isotovideo.pm#L369

If your custom CASEDIR repo already contains needles it should however work to load them if NEEDLES_DIR is empty/absent and needles are located under $PRODUCTDIR/needles.

Looks like the case mentioned in the last paragraph of my last comment is indeed broken as we set the NEEDLES_DIR here explicitly to needles which is a symlink to the default. However, as stated in the commit message of 1d8a079b731f5a88f9bd22fe03233ca6a77fc6e6 this is also required in some cases:

When only changing the behavior for PRODUCTDIR the needles could not be
loaded anymore (unless NEEDLES_DIR is specified explicitly) because the
checkout of CASEDIR does not necessarily contain needles. To fix this, the
symlinking of the needles directory is done in that case. See the verbose
comments within the code for further explanations.

So not setting NEEDLES_DIR here and assuming CASEDIR contains needles would break the case where it doesn't.

For the sake of not breaking other use-cases (and compatibility in general) it would make sense to add another variable to specify what NEEDLES_DIR is relative to, e.g. NEEDLES_DIR_BASE. If set to casedir it would work like you expect it to work and if set to default it would be relative within the default needles directory (like it behaves currently).

(Note that having the needles within the test distribution directory would likely be the better default. However, I suppose it is better not to change this in an incompatible way at this point.)

mkittler wrote:

Looks like the case mentioned in the last paragraph of my last comment is indeed broken as we set the NEEDLES_DIR here explicitly to needles which is a symlink to the default. However, as stated in the commit message of 1d8a079b731f5a88f9bd22fe03233ca6a77fc6e6 this is also required in some cases:

When only changing the behavior for PRODUCTDIR the needles could not be
loaded anymore (unless NEEDLES_DIR is specified explicitly) because the
checkout of CASEDIR does not necessarily contain needles. To fix this, the
symlinking of the needles directory is done in that case. See the verbose
comments within the code for further explanations.

So not setting NEEDLES_DIR here and assuming CASEDIR contains needles would break the case where it doesn't.

---

For the sake of not breaking other use-cases (and compatibility in general) it would make sense to add another variable to specify what NEEDLES_DIR is relative to, e.g. NEEDLES_DIR_BASE. If set to casedir it would work like you expect it to work and if set to default it would be relative within the default needles directory (like it behaves currently).

(Note that having the needles within the test distribution directory would likely be the better default. However, I suppose it is better not to change this in an incompatible way at this point.)

mkittler wrote:

Looks like the case mentioned in the last paragraph of my last comment is indeed broken as we set the NEEDLES_DIR here explicitly to needles which is a symlink to the default. However, as stated in the commit message of 1d8a079b731f5a88f9bd22fe03233ca6a77fc6e6 this is also required in some cases:

When only changing the behavior for PRODUCTDIR the needles could not be
loaded anymore (unless NEEDLES_DIR is specified explicitly) because the
checkout of CASEDIR does not necessarily contain needles. To fix this, the
symlinking of the needles directory is done in that case. See the verbose
comments within the code for further explanations.

So not setting NEEDLES_DIR here and assuming CASEDIR contains needles would break the case where it doesn't.

---

For the sake of not breaking other use-cases (and compatibility in general) it would make sense to add another variable to specify what NEEDLES_DIR is relative to, e.g. NEEDLES_DIR_BASE. If set to casedir it would work like you expect it to work and if set to default it would be relative within the default needles directory (like it behaves currently).

(Note that having the needles within the test distribution directory would likely be the better default. However, I suppose it is better not to change this in an incompatible way at this point.)

How about using PRODUCTDIR for this? If PRODUCTDIR is a git repo, then os-autoinst could assume that the needles are in it, but if CASEDIR is one, then it would not?

Let me think it though:

1. So far we don't allow setting a Git URL for PRODUCTDIR but that's actually good because it means we cannot break any existing behavior.
2. The referenced Git repo needed to contain main.pm on top-level but I suppose for your use-case (https://github.com/dcermak/kiwi-functional-tests) that's a given.
3. If PRODUCTDIR is a relative (or even absolute) path we should likely not change the behavior because a relative PRODUCTDIR (or absolute if ABSOLUTE_TEST_CONFIG_PATHS is set) is assigned by default for the job's DISTRI/VERSION and it would be very weird if assigning a PRODUCTDIR explicitly would cause a different behavior.
4. It is potentially a little bit confusing that the behavior of the needle lookup changes if PRODUCTDIR is set to be a Git URL. It is not intuitive to assume that a change to PRODUCTDIR changes the directory a relative NEEDLES_DIR is considered to be part of.
5. What CASEDIR should be set then? Without changing anything in that regard, $CASEDIR/lib would still be added to Perl's include path by os-autoinst so one needed to make sure to change CASEDIR as well in accordance.
6. This requires changes in os-autoinst and openQA:
   1. os-autoinst: Allow checking out Git repos when PRODUCTDIR is a Git URL.
   2. openQA: Adjust code in isotovideo.pm to avoid setting NEEDLES_DIR when PRODUCTDIR is a Git URL.
   3. openQA: Should we possibly just pass on the Git URL from PRODUCTDIR to os-autoinst as CASEDIR? This would maybe help with 5. and we wouldn't need 6.1.

Considering 3. to 6., the more explicit NEEDLES_DIR_BASE approach suggested in my previous comments seems better. It is less confusing and raises less questions. Of course it has the disadvantage that we need to introduce yet another variable.

mkittler wrote:

Considering 3. to 6., the more explicit NEEDLES_DIR_BASE approach suggested in my previous comments seems better. It is less confusing and raises less questions. Of course it has the disadvantage that we need to introduce yet another variable.

That makes sense, let's go with NEEDLES_DIR_BASE then.

I've started to implement by approach but it but it isn't really straight forward. That's also because it is a bit questionable how openQA currently discards a relative NEEDLES_DIR completely (in favor of the default one). Even specifying a path like foo/bar/baz for NEEDLES_DIR would simply mean to just read needles from the default needles directory without considering the sub directories.

(See https://github.com/Martchus/openQA/pull/new/needles-dir-base for my current attempt.)

Discussed with mkittler and cdywan. We propose to shift more logic into os-autoinst and implement an additional keyword so that os-autoinst looks up only a fallback needles repo in case needles are not found anywhere else. Also we assume that os-autoinst is easier to change and maintain. The current proposal is to use NEEDLES_DIR=[<keyword>]<dir>, e.g. NEEDLES_DIR=[fallback]my/productdir/needles for a relative fallback directory. The only suggested to be supported keyword should be "fallback" for now.

Suggested steps:

• In os-autoinst parse keyword from NEEDLES_DIR
• If "[fallback]" then try to find needles in all default paths, e.g. subdir of casedir, productdir, etc., only if not found look up in the fallback one
• Prepare setting fallback in openQA Worker/Engines/isotovideo.pm but do not merge yet
• Deploy new feature in os-autoinst
• Wait grace period
• Deploy openQA feature

I put the above ideas into a new ticket #97112. To conclude the current story quicker as we exceeded our due-date I suggest to just document the current best workaround as "keep in mind" in http://open.qa/docs/#_triggering_tests_based_on_an_any_remote_git_refspec_or_open_github_pull_request and resolve the ticket

PR for extending the documentation as stated in @okurz's comment: https://github.com/os-autoinst/openQA/pull/4130

So I will not follow the NEEDLES_DIR_BASE approach but the feature request added by #97112 will make the use case described in this ticket work by default without affecting other use cases.

https://github.com/os-autoinst/openQA/pull/4130 merged. Triggered https://app.circleci.com/pipelines/github/os-autoinst/openQA/7415/workflows/93acde46-2aad-45a4-b1ea-4cef970081d2/jobs/70002 to build updated documentation. I expect that this creates another doc update PR. As soon as that one deployed updated config I consider the ticket resolvable.

https://github.com/os-autoinst/openQA/pull/4132 is the according official documentation change