NEEDLES_DIR documentation and/or behaviour could be improved

[phil-hands]

I have needles in the same repo as the tests for the Debian tests (using git-lfs to deal with the bloat, which seems to work well BTW).

If I set the CASEDIR to be a branch or commit, it fails with a complaint about not finding the thing it's looking for in the repo.

In that case, it creates a symlink in the pool/N directory for the needles, to the default location for the needles repo, which of course does not match the needles in the CASEDIR repo.

If I set NEEDLES_DIR=products/debian/needles then it works.

The Documentation says:

Note that customizing CASEDIR does not mean needles will be loaded from there, even if the repository specified as CASEDIR contains needles. To load needles from that repository, it needs to be specified as NEEDLES_DIR as well.

which suggests that one should copy the git URL into both variables, but that doesn't work (and strikes me as a bit pointless TBH).

Given this comment about NEEDLES_DIR, CASEDIR & PRODUCTDIR it seems like the current behaviour is intended, and that one should just set the relative path for NEEDLES_DIR, in which case I think that should be made more explicit in the docs.

Alternatively, it strikes me that since the thing that one is setting NEEDLES_DIR to is just $PRODUCTDIR/needles there really ought to be a way of making it do that automatically, without having to specify the PRODUCTDIR value twice, but that probably needs to be dependent upon whether the normal repo which the CASEDIR is almost certainly forked is also an all-in-one or a needles_separate repo.

[okurz]

Some related issues:

• Support relative needle directories together with tests checked out from git
• [epic] Support needles from git-cloned or separate repo if casedir points to a git-repo
• [epic] Extend needle version control handling
• New needles from git repository not working with openqa-clone-custom-git-refspec
• Improve handling external Git repositories (for needles)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[okurz]

Now tracked additionally as https://progress.opensuse.org/issues/117133 . Also I created https://progress.opensuse.org/issues/117136 to prevent the stale bot to close issues which are still valid.

[Martchus]

The docs just say that one is supposed to set NEEDLES_DIR but not to what in which use case. That should certainly be improved.

What is your use case? Custom test dir + default needles dir? Then the combination of CASEDIR=giturl + NEEDLES_DIR=relative/path/of/needles/within/default/test/dir should work.

However, I guess your case is custom test dir + custom needles dir. Then you need to set NEEDLES_DIR=giturl as you've actually inferred from the docs. However, this makes only sense if that Git repo contains needles directly (not within a sub directory). So I suppose that's only going to work if you have a separate repo for needles and therefore it wasn't working in your case. So I guess your use-case isn't covered. I'm wondering why NEEDLES_DIR=products/debian/needles helped because I'd assumed it would then use the default needles repo.

Alternatively, it strikes me that since the thing that one is setting NEEDLES_DIR to is just $PRODUCTDIR/needles there really ought to be a way of making it do that automatically, …

If you do not set NEEDLES_DIR then the default one should actually be assumed (${OPENQA_BASEDIR:-/var/lib}/openqa/tests/$distri/products/$distri/needles). Specifying a custom CASEDIR should not change that.

---

So the behavior is tailored for having needles in a separate repository. If we can clarify your use case we can of course think of a solution to support it. However, we must not break existing behavior. So maybe we need to introduce an additional variable to tell the openQA worker to treat a relative NEEDLES_DIR to be relative to CASEDIR (and not the default needles dir).

[phil-hands]

My use case is that I want to allow people to fork and/or branch from the test repo used for openqa.debian.net, where the needles are to be found in products/debian/needles within that repo.

They should then be able to set up a new test, and create needles to match, push the result, and then refer to their commit to see if things work.

(I really want that to just happen via the gitlab CI setup, so that people don't need to know what's going on to write new tests)

If I said that I'd got that working, I think I was confused, since my recollection now is that at one point I thought I had it working, but in fact I'd just pushed the needles in question to the main branch as well, and the test case was picking them up from there.

I think that in reality, there was no combination of settings that managed to get hold of a needle that only existed in the commit set via CASEDIR.

If you suspect there's a combination of settings that ought to make this work, I am of course very happy to try again.

BTW I don't think it should make any difference, but the .png files in that repository are stored via git-lfs, so are not really in the same repo, although they look like they are.

[Martchus]

If I said that I'd got that working, I think I was confused, since my recollection now is that at one point I thought I had it working, but in fact I'd just pushed the needles in question to the main branch as well, and the test case was picking them up from there.

That makes more sense.

I think that in reality, there was no combination of settings that managed to get hold of a needle that only existed in the commit set via CASEDIR.

Right, as I said, that use case is not supported at this point.

If you suspect there's a combination of settings that ought to make this work, I am of course very happy to try again.

No. As said before, we'd likely even need to introduce a new variable (instead of just changing the behavior to avoid breaking other use cases).

[Martchus]

Note that last time I was working in that area introducing another variable was deemed to complicated so I resorted to dumping down the cases we can handle. So before implementing anything it would be good if other members of the @tools-team would respond. Otherwise it'll be only frustrating to do changes that are then rejected after all.

[okurz]

Note that last time I was working in that area introducing another variable was deemed to complicated so I resorted to dumping down the cases we can handle

could you provide a bit more context about that, e.g. the commit and/or ticket where you did that? Might help to refresh my memory there :)

[Martchus]

I think I ditched my commits when you've saw them. Not sure whether there's a record (because I mainly remember discussing it in a jitisi session).

[okurz]

The original proposal goes in the direction of https://progress.opensuse.org/issues/58184 , a bigger feature complex we had planned for the future. My vision is that openQA would be able to care about a complete test definition including needles within one git repository with a secondary use case being layered git repos as presented by the OP. So eventually the plan is to come to that but this will likely take long.

An alternative solution might be to use variable expansion within variables so that one could specify something like NEEDLES_DIR=%CASEDIR%/needles. This will likely help the OP's original usecase but might make our life harder for implementing https://progress.opensuse.org/issues/58184

[Martchus]

The PR has been merged so we should be good now. (You can of course re-open if there's something missing.)

[phil-hands]

Hi Again,

I just gave this a try, and it's not working quite as I envisaged.

In order to get it to work, I needed to set NEEDLES_DIR=%CASEDIR%openqa-tests-debian/products/debian/needles as you can see here: https://openqa.debian.net/tests/97768

It strikes me that there are a couple of things wrong with this. The first is that the case dir repo gets cloned into a directory named after the remote repo, so in this case we have CASEDIR=https://salsa.debian.org/philh/openqa-tests-debian.git/#start_button which gives rise to a pool directory on the worker that has the cloned repo in openqa-tests-debian and a symlink called needles pointing at openqa-tests-debian/products/debian/needles.

It seems wrong to be using the name of the repo that the user specifies here. Wouldn't it be better to always use e.g. casedir to avoid the possibility of malicious repo names?

Secondly, whatever the directory is called, shouldn't the relative path specified in NEEDLES_DIR be from within that direcotory? (so, in this case I'd expect to specify NEEDLES_DIR=products/debian/needles to get things working.

In fact, I've forgotten why I'm using the products/.../needles variant of the needles dir, but having to specify the distro in that place strikes me as a duplication of effort, so perhaps I should try moving everything up to the top level.

It would be nice if one could specify something like NEEDLES_DIR=%RELATIVE_MAGIC% and have it do the normal hunt of both needles and productts/$DISTRI/needles within the repo specified in CASEDIR, since then one could have a fork of the repo using the top-level needles approach, while another had the products directories, and still only need to specify the same thing when launching the jobs.

BTW I am of course happy to come up with patches to do this, once we've worked out what the preferred approach is.

[Martchus]

It seems wrong to be using the name of the repo that the user specifies here. Wouldn't it be better to always use e.g. casedir to avoid the possibility of malicious repo names?

Yes. I only implemented it that way because that's consistent with what's done on os-autoinst level when the Git repo is actually cloned. To change that we needed to change the code that does the Git cloning in os-autoinst and then also adapt the newly added openQA worker code accordingly.

Secondly, whatever the directory is called, shouldn't the relative path specified in NEEDLES_DIR be from within that direcotory?

No, NEEDLES_DIR is (if a relative path has been specified) relative to isotovideo's working directory (see https://github.com/os-autoinst/os-autoinst/blob/master/doc/backend_vars.asciidoc). That also makes generally sense because the needles directory is not necessarily a sub directory of CASEDIR (which I presume you expected to be the relative base). So consequently the worker must set NEEDLES_DIR to the full relative path from the pool directory (which is isotovideo's working directory).

In fact, I've forgotten why I'm using the products/.../needles variant of the needles dir, but having to specify the distro in that place strikes me as a duplication of effort, so perhaps I should try moving everything up to the top level.

Maybe you've copied the structure from openSUSE's test repository (https://github.com/os-autoinst/os-autoinst-distri-opensuse/tree/master/products). It is used for multiple products so there the needles directory is in a distribution-specific path as needles for those different distributions are actually provided by different other repositories. This way of structuring it is supported by openQA as it sets the default product/needles directory to fit that structure. When specifying the needles directory yourself you'll need to take care of that yourself.

do the normal hunt …

I don't think it is normally hunting that way. Normally the openQA worker default-initializes PRODUCT_DIR unconditionally as explained in the previous paragraph. If I read the code in https://github.com/os-autoinst/os-autoinst/blob/5a76fb8e636ccc4fdf22b7738caae6aa40895920/needle.pm#L276 correctly, then isotovideo looks for a directory needles within that PRODUCT_DIR (and PRODUCT_DIR itself is relative to CASEDIR). If NEEDLES_DIR is set explicitly (normally not done by default by the worker) it is expected to be relative to the current working directory although due to the PRODUCT_DIR handling (which as mentioned it relative to CASEDIR) there's actually also a fallback for it being treated relative to CASEDIR. Only if PRODUCT_DIR was empty it would simply look under needles in CASEDIR.

[phil-hands]

I don't think it is normally hunting that way. ...

Yes, you're right -- I somehow had the impression that I'd originally had things not all under the ./products/$DISTRI directory, but looking again I guess one of the examples I was looking at early on must have had PRODUCT_DIR defined without me noticing, or some such.

[phil-hands]

so, getting back to the %CASEDIR% substitution aspect of this, the way I read the documentation as it now stands describes the way I was imagining that it was going to work, rather than the way it actually works (which is hardly a surprise, given that I think I suggested that wording ;-) )

In particular:

So specifying e.g. NEEDLES_DIR=%CASEDIR%/the-needles
... results in needles found in https://github.com/…/tree/…/the-needles to be used

Which is not correct AFAICS. If you have a repo https://github.com/.../my-repo.git that includes the-needles as the directory with needles in it, then you need to specify NEEDLES_DIR as %CASEDIR%my-repo/the-needles (with the repo name directly after %CASEDIR%)

[phil-hands]

Another little wrinkle is that if I specify this:

NEEDLES_DIR=%CASEDIR%openqa-tests-debian/products/debian/needles

then the test works, whereas if I specify it like this (with a / after the %CASEDIR%):

NEEDLES_DIR=%CASEDIR%/openqa-tests-debian/products/debian/needles

if fails, producing this error:

Can't init needles from needles; If one doesn't specify NEEDLES_DIR, the needles will be loaded from $PRODUCTDIR/needles firstly or /var/lib/openqa/pool/11/openqa-tests-debian/needles ($CASEDIR + needles), check vars.json at /usr/lib/os-autoinst/needle.pm line 281.

As seen here: https://openqa.debian.net/tests/98227#details ( vs. the working version here: https://openqa.debian.net/tests/98226 )

I guess that's because the latter is being treated as non-relative somehow, but it's not exactly obvious that one should expect that to matter, so I think it should either be explicitly documented, or the code should be fixed to make it work regardless, as deemed appropriate (I've not worked out why the with-slash version would be useful in a distinct way, so would currently assume that they should be treated as equivalent).