Project

General

Profile

Actions

action #162035

closed

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

coordination #88561: [epic] Extend needle version control handling

Support development of "Use needles from correct ref of NEEDLES_DIR" https://github.com/os-autoinst/openQA/pull/5175 size:M

Added by okurz 10 months ago. Updated about 1 month ago.

Status:
Resolved
Priority:
High
Assignee:
Category:
Feature requests
Target version:
Start date:
2024-06-10
Due date:
% Done:

0%

Estimated time:
Tags:

Description

Motivation

It can be confusing to not see candidate needles on the web UI that were not available at the time the test was being executed.

Acceptance criteria

  • AC1: The candidates view on the test details pages shows candidate needles of the version that was used when the test was executed.
  • AC2: Temporary needles are cleaned up (instantly or on a schedule)
  • AC3: The needle editor still shows the last candidates. One might optionally show there the candidates at the time the test ran as well.

Suggestions

  • Read exactly what the PR "Use needles from correct ref of NEEDLES_DIR" is about
  • Checkout #154783-10 and further comments for what's missing
  • Review the code and make suggestions or add changes yourself to get it merged
  • Make sure test coverage is appropriate (PR does not look like it atm.)
  • If any work becomes too much work then split out into separate explicit tickets and provide explicit feedback to the PR author about what is expected from them and what can be expected from us
  • Saying no to a feature is always an option too
  • Consider a NEEDLES_DIR being a different repo/remote entirely (e.g. a pull request) (although this is not strictly required it would be good to ensure we could also support it)
  • Read #56789 again

Related issues 5 (3 open2 closed)

Related to openQA Project (public) - action #56789: New needles from git repository not working with openqa-clone-custom-git-refspecBlockedokurz2019-09-11

Actions
Related to openQA Project (public) - action #157159: Show version of candidate needles the test ran on in the "diff view" on the test details pageNew

Actions
Copied to openQA Project (public) - action #177504: Conduct lessons learned "Five Why" analysis for "Use needles from correct ref of NEEDLES_DIR" size:SResolvedlivdywan

Actions
Copied to openQA Project (public) - action #177564: Ensure test coverage of needle editor size:SResolvedmkittler

Actions
Copied to openQA Project (public) - action #177817: Add a way to show needles with correct version from remote forges e.g. github by hotlinking them from githubusercontent or equivalent configurable URL mappings size:SWorkabletinita2024-06-10

Actions
Actions #1

Updated by okurz 10 months ago

  • Tracker changed from coordination to action
  • Target version changed from Tools - Next to Ready
Actions #2

Updated by okurz 10 months ago

  • Subject changed from Support development of https://github.com/os-autoinst/openQA/pull/5175 to Support development of "Use needles from correct ref of NEEDLES_DIR" https://github.com/os-autoinst/openQA/pull/5175 size:M
  • Description updated (diff)
  • Status changed from New to Workable
Actions #3

Updated by livdywan 9 months ago

I would guess there were more urgent tickets preventing someone from picking this up within our usual 30 days. If you took a look and had open questions, might be worth discussing again, though.

Actions #4

Updated by okurz 8 months ago

  • Priority changed from Normal to High
Actions #5

Updated by tinita 8 months ago

  • Assignee set to tinita
Actions #6

Updated by tinita 8 months ago

  • Status changed from Workable to In Progress
Actions #7

Updated by openqa_review 8 months ago

  • Due date set to 2024-09-03

Setting due date based on mean cycle time of SUSE QE Tools

Actions #8

Updated by tinita 7 months ago

  • Status changed from In Progress to Feedback

I tried out the pull request locally and added some notes: https://github.com/os-autoinst/openQA/pull/5175
Still need to understand all of it...

Actions #9

Updated by livdywan 7 months ago

  • Related to action #56789: New needles from git repository not working with openqa-clone-custom-git-refspec added
Actions #10

Updated by livdywan 7 months ago

  • Description updated (diff)
Actions #11

Updated by tinita 7 months ago

  • Related to action #157159: Show version of candidate needles the test ran on in the "diff view" on the test details page added
Actions #12

Updated by tinita 7 months ago

  • Status changed from Feedback to Workable

It looks like I can't work on it in the very near future, so putting it back to Workable for now

Actions #13

Updated by tinita 7 months ago

  • Assignee deleted (tinita)
Actions #14

Updated by livdywan 7 months ago

  • Due date deleted (2024-09-03)
Actions #15

Updated by livdywan 7 months ago

  • Status changed from Workable to In Progress
  • Assignee set to livdywan

Let's see if I can drive this. To start out I'm going to check if the OP had a chance to review the acceptance criteria we came up with.

Actions #16

Updated by livdywan 7 months ago

  • Status changed from In Progress to Blocked

Waiting for a response for now

Actions #17

Updated by livdywan 7 months ago

livdywan wrote in #note-16:

Waiting for a response for now

No response so far. Might just need to wait for Scott to make time for it.

Actions #18

Updated by livdywan 6 months ago

Waiting for a response for now

No response so far. Might just need to wait for Scott to make time for it.

Asked once more.

Actions #19

Updated by livdywan 6 months ago

  • Status changed from Blocked to Workable

Scott confirmed the AC's we came up with but seems to have no time to work on this for now. The PR has some conflicts. Maybe this can be considered unblocked.

Actions #20

Updated by ybonatakis 6 months ago

Let's wait for Liv to return from vacation next week!?!

Actions #21

Updated by okurz 6 months ago

  • Priority changed from High to Normal
Actions #22

Updated by livdywan 5 months ago

  • Status changed from Workable to In Progress

Re-reading the current code and rebasing the PR against the latest upstream

Actions #23

Updated by openqa_review 5 months ago

  • Due date set to 2024-11-05

Setting due date based on mean cycle time of SUSE QE Tools

Actions #24

Updated by livdywan 5 months ago

Discussed in the unblock

  • https://github.com/os-autoinst/openQA/pull/5175 rebased
  • I dropped the use of File::Touch which we don't depend on (I was confused by the CI results but the fix was straightforward)
  • Looking into coverage gaps which are now also visible on the PR
    • Most importantly the candidates view
    • Needle editing looks to be using the new code, too, but is not required
    • Check how to reproduce the new feature manually e.g. using NEEDLES_DIR/other test vars (same in unit tests)
Actions #25

Updated by livdywan 5 months ago

  • Status changed from In Progress to Workable

Putting this aside for a moment.

Actions #26

Updated by livdywan 5 months ago

  • Due date deleted (2024-11-05)

I won't be able to have this wrapped up and deployed by the due date. Unless somebody else wants to take over in my absence and make that happen.

Actions #27

Updated by livdywan 4 months ago

  • Assignee deleted (livdywan)
Actions #28

Updated by gpuliti 3 months ago

I've created a rebased version of the pr opened by @livdywan https://github.com/os-autoinst/openQA/pull/6097

Actions #29

Updated by okurz 2 months ago

  • Priority changed from Normal to High
Actions #30

Updated by livdywan about 2 months ago · Edited

  • Status changed from Workable to In Progress
  • Assignee set to livdywan

Looking to get @gpuliti's changes into the original branch and checking coverage since that was incomplete before.

Actions #31

Updated by livdywan about 2 months ago

  • Assignee changed from livdywan to gpuliti
Actions #32

Updated by openqa_review about 2 months ago

  • Due date set to 2025-02-18

Setting due date based on mean cycle time of SUSE QE Tools

Actions #33

Updated by livdywan about 2 months ago

1/3 done, tests currently being added and reviewed

Actions #34

Updated by gpuliti about 2 months ago

[ ] Ensure that we have 100% coverage
[x] finalize the wording used in the config file "correct version" vs branch/commit/sha
[x] Clarify if @mkittler's comment was addressed regarding "fetched by the web UI"

With the commit fd4e2469 there are only 3 missing lines to cover that can be seen in codecov. I've already created the test to cover them here, but I think I'm missing the correct endpoint api.

Actions #35

Updated by livdywan about 2 months ago

  • Status changed from In Progress to Workable
  • Assignee deleted (gpuliti)

As discussed before as Gabriele can't continue with this right now it would be super cool if somebody else can pick it up.

Actions #36

Updated by dheidler about 2 months ago

  • Status changed from Workable to In Progress
  • Assignee set to dheidler
Actions #37

Updated by okurz about 1 month ago

  • Priority changed from High to Urgent
Actions #38

Updated by livdywan about 1 month ago

  • Copied to action #177504: Conduct lessons learned "Five Why" analysis for "Use needles from correct ref of NEEDLES_DIR" size:S added
Actions #39

Updated by livdywan about 1 month ago

Reviewing the current state of the PR

  • _create_tmpdir_for_needles_refspec performs network access every time it is called
    • git fetch
    • vars.json is read but never saved
  • there is currently no feature flag for this
  • consider "our" ticket #157159 about candidate needles in the diff view
  • maybe this PR should exclude the needle editor
    • revert changes to the edit function
  • review the current state of tests for the needle editor that would cover the case here
Actions #40

Updated by dheidler about 1 month ago · Edited

livdywan wrote in #note-39:

Reviewing the current state of the PR

  • _create_tmpdir_for_needles_refspec performs network access every time it is called
    • git fetch

That is already adressed but I haven't pushed it yet

  • vars.json is read but never saved

Apart from the worker saving it.
It would be much more efficient if we could write that fields back to the database on job finish,
so that we don't need to parse a json file for each needle.
That should be done on job finish, though - maybe with a whitelist of job settings that
should be written from vars.json back to the database.

  • there is currently no feature flag for this

What is [scm git]->checkout_needles_sha in the config then?

Actions #42

Updated by livdywan about 1 month ago

  • Copied to action #177564: Ensure test coverage of needle editor size:S added
Actions #43

Updated by livdywan about 1 month ago

  • Due date deleted (2025-02-18)
  • Status changed from In Progress to Blocked
  • Priority changed from Urgent to High

Blocking on #177564 as dicussed.

Actions #44

Updated by livdywan about 1 month ago

  • Status changed from Blocked to In Progress

Blocker was resolved

Actions #45

Updated by livdywan about 1 month ago

  • Tags set to expert
Actions #46

Updated by livdywan about 1 month ago

Since this came up in the 5 Whys, can we clarify if we want #157159 on the backlog now (or next) and how it relates to this feature? Maybe we should at least estimate the ticket to be clear on what we need to know?

Actions #47

Updated by tinita about 1 month ago

Now that I see that there is a PR on only showing the correct ref for the matched needle in the step https://github.com/os-autoinst/openQA/pull/6190
I would like to reconsider some points:

  1. We are passing the complete file path to the json file in the needle url. If I look into the code, it's checking already if it is a subdir of share/tests anyway, so we can just leave off that path. I don't consider full paths in urls a good thing
  2. For just showing the test step with the matched needle, we don't need a local cache, I believe. We can just do a redirect to https://raw.githubusercontent.com/user/repo/sha/file.png in OpenQA::WebAPI::Controller::File::needle(), saving local space and traffic. I got it working locally with a hardcoded sha. Also other git providers provide a raw image view.

This way we could easily enable the feature without having to fear a lot of traffic.

For the needle editor: For that we would probably need the json as well, so we need to fetch content from git here. But this is used rarely compared to showing module steps, and only by logged in users, so we probably can enable this as well.

For the candidates list: We need a full needle directory to get all the candidates. But maybe this could be implemented as a feature on request, e.g. having a button on a test saying "This test is based on an older git sha. Show needle candidates based on this sha instead". This way by default we would also not have to fear any additional traffic.

Actions #48

Updated by openqa_review about 1 month ago

  • Due date set to 2025-03-11

Setting due date based on mean cycle time of SUSE QE Tools

Actions #49

Updated by dheidler about 1 month ago

  • Status changed from In Progress to Feedback

Regarding 1.:

Sure - but I would handle that in a different ticket to not make this more complicated than it already is. Otherwise we will never get this merged.
I would especially not fix this in this PR as this was a technical debt that was introduced way before.

Regarding 2.:

The githubusercontent hotlinking is a nice addition but of course github specific so it would be different for github vs. gitlab vs. forgejo (and no easy way of distinguishing between the last two of them apart from public instances like codeberg.org and gitlab.com).
We would also still need the code from the current PR as not every git forge has a webui at all.
And even with github a manual override might be required as the repo might be set to private.

So that should be handled in a different ticket as well to not overcomplicate things here.

Actions #50

Updated by tinita about 1 month ago

dheidler wrote in #note-49:

Regarding 2.:

The githubusercontent hotlinking is a nice addition but of course github specific so it would be different for github vs. gitlab vs. forgejo (and no easy way of distinguishing between the last two of them apart from public instances like codeberg.org and gitlab.com).

That can be configured with a simple mapping in the openqa.ini. No big effort.

We would also still need the code from the current PR as not every git forge has a webui at all.
And even with github a manual override might be required as the repo might be set to private.

The question is if we really need this. Using githubusercontent would be relatively easy to implement and it would be easy to enable without having to fear traffic, and we might never need to support private repos.

So in the end we would have easier code here.

Actions #51

Updated by dheidler about 1 month ago · Edited

But the code for plain git is already here in the PR.
All the work is done.
And it is optional to enable it.
Why throw it away?
For the sake of having less code? I don't think so.

The githubusercontent hotlinking sounds like a good idea that will be propably what we will be using.
But I would like to have both options as merging the existing PR is now zero effort.

Actions #52

Updated by dheidler about 1 month ago

  • Copied to action #177817: Add a way to show needles with correct version from remote forges e.g. github by hotlinking them from githubusercontent or equivalent configurable URL mappings size:S added
Actions #53

Updated by livdywan about 1 month ago

As per discussions yesterday I assume you're wrapping up https://github.com/os-autoinst/openQA/pull/6190 as planned, right? And the proof of concept mentioned in #177817 will be an optimization that's not making this obsolete.

Actions #54

Updated by dheidler about 1 month ago

I wasn't sure what the teams opition was on that but sure.

Actions #55

Updated by okurz about 1 month ago

  • Status changed from Feedback to In Progress
Actions #56

Updated by tinita about 1 month ago

tinita wrote in #note-47:

For the candidates list: We need a full needle directory to get all the candidates. [...]

I just tried out the code again and realized this works already fine for the candidates list.
I remember a conversation where Marius said that for the needle candidates we would need to check out that ref to be able to determine all the possible needle candidates, but all the necessary information is stored in details-modulename.json already. Maybe I misunderstood? Why was nobody correcting my above assumption? :)

Actions #57

Updated by dheidler about 1 month ago

  • Status changed from In Progress to Resolved
Actions #58

Updated by tinita about 1 month ago · Edited

  • Status changed from Resolved to Feedback

Introduced unhandled output in tests:
https://app.circleci.com/pipelines/github/os-autoinst/openQA/15972/workflows/6bcf88f6-aa49-47ec-9e5b-8535595df548/jobs/153720
https://app.circleci.com/pipelines/github/os-autoinst/openQA/15972/workflows/6bcf88f6-aa49-47ec-9e5b-8535595df548/jobs/153720

[20:29:33] t/16-utils-job-templates.t ................................ ok      803 ms ( 0.00 usr  0.00 sys +  0.67 cusr  0.13 csys =  0.80 CPU)
[20:29:34] t/16-utils-runcmd.t ....................................... 1/? hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint: 
hint:   git config --global init.defaultBranch <name>
hint: 
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint: 
hint:   git branch -m <name>
[20:29:34] t/16-utils-runcmd.t ....................................... ok     9811 ms ( 0.01 usr  0.00 sys +  

6.54 cusr  0.28 csys =  6.83 CPU)
[20:30:30] t/21-needles.t ............................................ 4/? hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint: 
hint:   git config --global init.defaultBranch <name>
hint: 
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint: 
hint:   git branch -m <name>
hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint: 
hint:   git config --global init.defaultBranch <name>
hint: 
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint: 
hint:   git branch -m <name>
[20:30:30] t/21-needles.t ............................................ ok    10873 ms ( 0.02 usr  0.00 sys + 10.24 cusr  0.49 csys = 10.75 CPU)
[20:30:41] t/22-dashboard.t .......................................... ok    16152 ms ( 0.06 usr  0.00 sys + 1
Actions #60

Updated by dheidler about 1 month ago

  • Status changed from Feedback to Resolved
Actions #61

Updated by okurz about 1 month ago

  • Due date deleted (2025-03-11)
Actions

Also available in: Atom PDF