Allow scheduling a (example) product via the web UI #5933

Martchus · 2024-09-17T15:37:12Z

Related ticket: https://progress.opensuse.org/issues/166658

Still a draft because I need to add UI tests and move the so far hard-coded data into openqa.ini. It already reads scenario definitions and certain parameters directly from the example distribution, though. Of course this only works if there's a checkout of the example distribution. Maybe we should offer the user to create that checkout in case it is missing (which wouldn't be very hard to do; we'd just add a link on the web UI that'll internally spawn a checkout Minion job we recently implemented anyway).

This is how the menu, the form and success/error messages look like in one screenshot:

So the test won't fail if we would add other links (e.g. in the navbar). Related ticket: https://progress.opensuse.org/issues/166658

okurz · 2024-09-18T10:58:24Z

overall this looks quite good. I was thinking of alternatives where to place the action but your proposal so far looks like the best to me. Just with the "+" I was thinking of a bigger button, possibly blue, with a frame around to distinguish clearly from the caret

I mean https://fontawesome.com/icons/square-plus?f=classic&s=regular

Martchus · 2024-09-18T11:08:56Z

I'm pondering about configurability. Reading the structure that has currently the FIXME comment from openqa.ini doesn't really work because it needs nesting but we only use INI. I suppose I could simply read those values from an additional file (e.g. test_presets.ini). Otherwise I could check for sections like [test_presets/example] in openqa.ini but it'll be an additional loop.

EDIT: I guess I'll go for [test_presets/example] because I suppose the additional loop can be avoided.

Martchus · 2024-09-18T12:38:34Z

I now made it configurable. So in this screenshot I overrode the title by putting a [test_presets/example] section to my openqa.ini:

This screenshot also shows that it is now also possible to add additional settings. I think this is important if we want to offer this as a generic form (and the implementation is also quite simple).

This leaves tests and the possibility to clone the example distribution automatically (for the scenario definitions).

codecov · 2024-09-18T12:52:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.74%. Comparing base (e88bdd3) to head (3af5894).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5933      +/-   ##
==========================================
+ Coverage   98.73%   98.74%   +0.01%     
==========================================
  Files         395      396       +1     
  Lines       38764    38870     +106     
==========================================
+ Hits        38275    38384     +109     
+ Misses        489      486       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

perlpunk · 2024-09-18T12:56:29Z

I'm pondering about configurability. Reading the structure that has currently the FIXME comment from openqa.ini doesn't really work because it needs nesting but we only use INI. I suppose I could simply read those values from an additional file (e.g. test_presets.ini). Otherwise I could check for sections like [test_presets/example] in openqa.ini but it'll be an additional loop.

EDIT: I guess I'll go for [test_presets/example] because I suppose the additional loop can be avoided.

Config::IniFiles allows groups. You separate the group from the section with a space.
https://metacpan.org/pod/Config::IniFiles#Groups

Martchus · 2024-09-19T10:38:02Z

Config::IniFiles allows groups. You separate the group from the section with a space.
https://metacpan.org/pod/Config::IniFiles#Groups

I now changed it to use e.g. [test_preset foo]. This way we could use the grouping mechanism of the INI parser - even though this is not necessary for the PR in its current form. With this we could use https://metacpan.org/pod/Config::IniFiles#GroupMembers-($group) to show the list of available presets in e.g. the navbar menu. (Not implemented yet. For now I stick to what we have planned in the ticket. So cloning the example repo automatically is probably more important.)

kalikiana · 2024-09-19T10:49:08Z

Still a draft because I need to add UI tests and move the so far hard-coded data into openqa.ini. It already reads scenario definitions and certain parameters directly from the example distribution, though. Of course this only works if there's a checkout of the example distribution. Maybe we should offer the user to create that checkout in case it is missing (which wouldn't be very hard to do; we'd just add a link on the web UI that'll internally spawn a checkout Minion job we recently implemented anyway).

Is this obsolete? As this is not a draft anymore 😉

kalikiana

Looks really nice. Especially with the help texts. And I appreciate your making it extensible in the future.

Martchus · 2024-09-19T12:33:07Z

I crossed out the obsolete part.

Considering there's remaining work to do I'm still considering this as not ready. We don't want to show a new option prominently that'll not work out of the box.

perlpunk · 2024-09-19T13:19:49Z

lib/OpenQA/WebAPI/Controller/Test.pm

+    return undef unless my $ini_config = $config->{ini_config};
+    my $ini_key = "test_preset $preset_key";
+    return $presets{$preset_key}
+      = $ini_config->SectionExists($ini_key)


I still don't like that we are creating some extra code for this specific feature to handle nested data, while we could do this in a more generic way in Setup.pm with the group feature.
Also it moves any validation we might want to do to runtime instead of server startup.
Since it's something internal we can probably change the implementation later, but just saying.
Not sure what others think.

I tried to keep it simple but chose the section name so the group feature can be used in the future for more generic parsing. So if we needed to parse this in a more generic way at some point it'll be possible without a breaking change to the config file format.

okurz · 2024-09-19T13:47:59Z

I crossed out the obsolete part.

Considering there's remaining work to do I'm still considering this as not ready. We don't want to show a new option prominently that'll not work out of the box.

Consider a feature switch which should be off by default until we stabilized the feature

Martchus · 2024-09-19T15:47:32Z

I now implemented cloning the example test distribution. So when the user enters the page and the repo hasn't been cloned yet it says "You have to clone the example test distribution first." where the bold part is a link, When clicking on the link one gets a loading indication and after a few seconds (it really doesn't take long to checkout the example distribution) the form reloads with all values filled in. (And in any error case the error is shown and one can retry.) This didn't require adding much code because we already have Minion tasks for cloning from Git now.

perlpunk · 2024-09-19T16:20:38Z

Btw, we have to be careful to not do this on o3 or osd until fetchneedles is replaced. The example distri has a default branch main, and fetchneedles can't handle different default branches.

okurz

Some inline comments.

And have you considered #5933 (comment) ?

etc/openqa/openqa.ini

lib/OpenQA/WebAPI/Controller/Test.pm

t/ui/29-create_tests.t

Martchus · 2024-09-20T08:19:28Z

And have you considered #5933 (comment) ?

I only read that comment after I implemented the most recent changes I pushed to this PR yesterday. I don't think it is useful to merge this feature without enabling it by default because it is about the out of the box experience.

Related ticket: https://progress.opensuse.org/issues/166658

okurz · 2024-09-20T08:50:48Z

And have you considered #5933 (comment) ?

I only read that comment after I implemented the most recent changes I pushed to this PR yesterday. I don't think it is useful to merge this feature without enabling it by default because it is about the out of the box experience.

I know but do you trust it enough to be enabled on all production instances? After all the primary goal was to show that easy starting point on clean jobless instances

I guess we are not clear on what we want here afterall

Martchus · 2024-09-20T11:15:56Z

I know but do you trust it enough to be enabled on all production instances?

The feature itself I'd generally consider stable. It has full test coverage. The checkout of the example distribution relies on the Git cloning we implemented a while ago and I don't think it is generally problematic.

I suppose the only problem is fetchneedles cannot handle the use of main as main branch (as opposed to master). At least according to @perlpunk. So I'll have a look at the situation. Maybe that's easy to fix or we can ignore it until fetchneedles is replaced or I can do a manual checkout on our production instances that uses and alias for master.

Martchus · 2024-09-20T13:08:48Z

I added another commit to avoid problems with fetchneedles. See the commit message for details. The short story is that it'll work on our production instances and I also did local test runs of the entire script. There's only one caveat which I don't find problematic but I added a comment in the fetchneedles script documenting it.

script/fetchneedles

* Avoid running into an error when a Git repository like our example distribution that does not use the `master` branch anymore is present * Avoid hard-coding the default branch name for test and needle repositories * Use `git rev-parse --abbrev-ref refs/remotes/origin/HEAD` instead which works with all test and needle repositories we have in our two production instances (after I created `.git/refs/remotes/origin/HEAD` in the SLE needles checkout; see added comment for details) * Keep the possibility to specify `branch` and `needles_branch` in case one wants to fetch a special branch * Tested locally with repos using master and main including the code branch for repairing the needle repos * See https://progress.opensuse.org/issues/166658

okurz

Looks very nice now. Fix the code style issues and I am sure I can approve.

okurz · 2024-09-20T17:47:42Z

t/data/openqa.ini

@@ -1 +0,0 @@
-../../etc/openqa/openqa.ini


Because I had to add config values for testing instead of just testing with the default config. (We want to cover the part of the code that allows the users/admins to override values so we obviously need to supply this kind of configuration when running the test.)

okurz

Looks very nice now. Fix the code style issues and I am sure I can approve.

Make selectors in t/17-build_tagging.to more specific

db10f2d

So the test won't fail if we would add other links (e.g. in the navbar). Related ticket: https://progress.opensuse.org/issues/166658

Simplify code for assigning config values

4b4ed38

Martchus force-pushed the plus-button-example-tests branch from 6fdab55 to 3ba945a Compare September 18, 2024 12:34

Martchus force-pushed the plus-button-example-tests branch from 3ba945a to ac5e408 Compare September 18, 2024 15:40

Martchus force-pushed the plus-button-example-tests branch from ac5e408 to 17d8212 Compare September 19, 2024 10:38

Martchus marked this pull request as ready for review September 19, 2024 10:45

kalikiana previously approved these changes Sep 19, 2024

View reviewed changes

Martchus added the not-ready label Sep 19, 2024

perlpunk reviewed Sep 19, 2024

View reviewed changes

Martchus removed the not-ready label Sep 19, 2024

Martchus force-pushed the plus-button-example-tests branch from 1fd66cc to a15b42c Compare September 19, 2024 16:07

okurz requested changes Sep 20, 2024

View reviewed changes

etc/openqa/openqa.ini Outdated Show resolved Hide resolved

lib/OpenQA/WebAPI/Controller/Test.pm Show resolved Hide resolved

t/ui/29-create_tests.t Outdated Show resolved Hide resolved

Martchus added 2 commits September 20, 2024 10:31

Allow scheduling a (example) product via the web UI

8c3b829

Related ticket: https://progress.opensuse.org/issues/166658

Allow cloning example test distribution from UI for running example test

3af5894

Related ticket: https://progress.opensuse.org/issues/166658

Martchus force-pushed the plus-button-example-tests branch from a15b42c to 3af5894 Compare September 20, 2024 08:32

Ease development of fetchneedles by avoid hardcoding OPENQA_BASEDIR

7c5a476

Martchus force-pushed the plus-button-example-tests branch from 3e20b94 to 571eacc Compare September 20, 2024 13:10

kalikiana approved these changes Sep 20, 2024

View reviewed changes

kalikiana reviewed Sep 20, 2024

View reviewed changes

script/fetchneedles Outdated Show resolved Hide resolved

Martchus force-pushed the plus-button-example-tests branch from 571eacc to 1a61ffb Compare September 20, 2024 14:32

okurz approved these changes Sep 20, 2024

View reviewed changes

okurz requested changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow scheduling a (example) product via the web UI #5933

Allow scheduling a (example) product via the web UI #5933

Martchus commented Sep 17, 2024 •

edited

Loading

okurz commented Sep 18, 2024 •

edited

Loading

Martchus commented Sep 18, 2024 •

edited

Loading

Martchus commented Sep 18, 2024

codecov bot commented Sep 18, 2024 •

edited

Loading

perlpunk commented Sep 18, 2024

Martchus commented Sep 19, 2024

kalikiana commented Sep 19, 2024

kalikiana left a comment

Martchus commented Sep 19, 2024 •

edited

Loading

perlpunk Sep 19, 2024

Martchus Sep 19, 2024

okurz commented Sep 19, 2024

Martchus commented Sep 19, 2024

perlpunk commented Sep 19, 2024

okurz left a comment

Martchus commented Sep 20, 2024

okurz commented Sep 20, 2024

Martchus commented Sep 20, 2024

Martchus commented Sep 20, 2024

okurz left a comment

okurz Sep 20, 2024

Martchus Sep 20, 2024

okurz left a comment

		@@ -1 +0,0 @@
		../../etc/openqa/openqa.ini

Allow scheduling a (example) product via the web UI #5933

Are you sure you want to change the base?

Allow scheduling a (example) product via the web UI #5933

Conversation

Martchus commented Sep 17, 2024 • edited Loading

okurz commented Sep 18, 2024 • edited Loading

Martchus commented Sep 18, 2024 • edited Loading

Martchus commented Sep 18, 2024

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

perlpunk commented Sep 18, 2024

Martchus commented Sep 19, 2024

kalikiana commented Sep 19, 2024

kalikiana left a comment

Choose a reason for hiding this comment

Martchus commented Sep 19, 2024 • edited Loading

perlpunk Sep 19, 2024

Choose a reason for hiding this comment

Martchus Sep 19, 2024

Choose a reason for hiding this comment

okurz commented Sep 19, 2024

Martchus commented Sep 19, 2024

perlpunk commented Sep 19, 2024

okurz left a comment

Choose a reason for hiding this comment

Martchus commented Sep 20, 2024

okurz commented Sep 20, 2024

Martchus commented Sep 20, 2024

Martchus commented Sep 20, 2024

okurz left a comment

Choose a reason for hiding this comment

okurz Sep 20, 2024

Choose a reason for hiding this comment

Martchus Sep 20, 2024

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

Martchus commented Sep 17, 2024 •

edited

Loading

okurz commented Sep 18, 2024 •

edited

Loading

Martchus commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

Martchus commented Sep 19, 2024 •

edited

Loading