Project

General

Profile

Actions

action #94735

closed

coordination #58184: [saga][epic][use case] full version control awareness within openQA

coordination #94750: [epic] Support needles from git-cloned or separate repo if casedir points to a git-repo

needles not found in `needles` subdirectory when CASEDIR is a git repository

Added by dancermak over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Feature requests
Target version:
Start date:
2021-06-25
Due date:
% Done:

0%

Estimated time:

Description

Observation

openQA does not appear to take the needles subdirectory into account for loading needles when CASEDIR is a git repository.

Steps to reproduce

  • Clone a job and set CASEDIR to a git repository with a needles subdirectory in that git repository
  • If the needles subdirectory contains needles not present on the instance, then openQA will not find them.

Workaround

Setting NEEDLES_DIR to CASEDIR works around that problem. Setting NEEDLES_DIR to needles/ on the other hand does not work.

Example failure: https://openqa.opensuse.org/tests/1806254 (-> failed)

cloned with NEEDLES_DIR='needles/': https://openqa.opensuse.org/tests/1806460 (-> failed)

cloned with NEEDLES_DIR='https://github.com/dcermak/kiwi-functional-tests#basic_functional_test': https://openqa.opensuse.org/tests/1806588 (-> passed)

Suggestions


Related issues 1 (1 open0 closed)

Copied to openQA Project - action #97112: Support relative needle directories together with tests checked out from gitNew

Actions
Actions #1

Updated by okurz over 2 years ago

  • Category set to Regressions/Crashes
  • Target version set to Ready
  • Parent task set to #94750
Actions #2

Updated by mkittler over 2 years ago

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.

Actions #3

Updated by mkittler over 2 years ago

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.)

Actions #4

Updated by ilausuch over 2 years ago

  • Description updated (diff)
  • Category changed from Regressions/Crashes to Feature requests

We considered during the daily that this if a feature because:

Actions #5

Updated by mkittler over 2 years ago

  • Category changed from Feature requests to Regressions/Crashes

@dancermak To me it seems important not to break other use-cases (where changing CASEDIR shouldn't affect NEEDLES_DIR). It would be good to know if my suggestion would work for you.

Maybe someone also has a better idea for making the behavior configurable.

Actions #6

Updated by mkittler over 2 years ago

  • Category changed from Regressions/Crashes to Feature requests
Actions #7

Updated by livdywan over 2 years ago

  • Due date set to 2021-08-06
  • Status changed from New to Feedback
  • Assignee set to mkittler

Marius agreed to take the ticket while we're waiting for clarification

Actions #8

Updated by dancermak over 2 years ago

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?

Actions #9

Updated by mkittler over 2 years ago

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.

Actions #10

Updated by dancermak over 2 years ago

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.

Actions #11

Updated by livdywan over 2 years ago

  • Due date changed from 2021-08-06 to 2021-08-13

It's probably fair to say that more urgent issues had priority, and this was pending open questions as well. Bumping the due date.

Actions #12

Updated by mkittler over 2 years ago

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.)

Actions #13

Updated by okurz over 2 years ago

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

Actions #14

Updated by okurz over 2 years ago

  • Copied to action #97112: Support relative needle directories together with tests checked out from git added
Actions #15

Updated by mkittler over 2 years ago

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.

Actions #16

Updated by okurz over 2 years ago

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.

Actions #17

Updated by okurz over 2 years ago

  • Status changed from Feedback to Resolved
Actions #18

Updated by okurz over 2 years ago

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

Actions #19

Updated by okurz over 2 years ago

  • Due date deleted (2021-08-13)
Actions

Also available in: Atom PDF