[POC] Trigger os-autoinst-distri-example tests from fresh openQA #5914
Conversation
This is supposed to trigger a isos post with the form data Signed-off-by: ybonatakis <[email protected]>
|
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
2024-09-09_11-04-48.mp4Due to the allowed size of upload the footage is as small as possible |
There was a problem hiding this comment.
I suggest you clean up this PR as mentioned in the inline-comments. Even though it is just a POC we probably want something another person could actually continue to work on. This is not even in a state where it is fun to look at for the sake of reviewing.
I added a few remarks about the modal. However, I suggest you remove it completely and show a flash message instead. That is much easier than making the modal actually work and the modal is not used/working in its current form anyway.
| ! run_example.js | ||
| < javascripts/run_example.js |
There was a problem hiding this comment.
This whole "feature" should probably go by the name "create test" or "schedule test" or "add test".
| ! run_example.js | |
| < javascripts/run_example.js | |
| ! create_test.js | |
| < javascripts/create_test.js |
("Run" is the wrong verb because we really don't immediately start a test here. "Example" of course also doesn't make any sense.)
| @@ -0,0 +1,29 @@ | |||
| function runExample(form) { | |||
There was a problem hiding this comment.
| function runExample(form) { | |
| function createTest(form) { |
| @@ -0,0 +1,29 @@ | |||
| function runExample(form) { | |||
There was a problem hiding this comment.
Please format the code in that function correctly and delete leftovers from debugging such as console.log calls and commented-out code.
assets/javascripts/run_example.js
Outdated
| let postUrl = form.dataset.postUrl; | ||
| $.ajax({ | ||
| url: postUrl, |
There was a problem hiding this comment.
| let postUrl = form.dataset.postUrl; | |
| $.ajax({ | |
| url: postUrl, | |
| $.ajax({ | |
| url: form.dataset.postUrl, |
assets/javascripts/run_example.js
Outdated
| console.log(ajaxOptions); | ||
| console.log(xhr); | ||
|
|
||
| if (xhr.responseJSON.error) { |
There was a problem hiding this comment.
xhr.responseJSON might be undefined and the code should be able to cope with that:
| if (xhr.responseJSON.error) { | |
| if (xhr.responseJSON?.error) { |
| @@ -34,7 +34,10 @@ | |||
| % } | |||
| % } | |||
| </ul> | |||
| </li> | |||
| </li> | |||
| <li class='nav-item' id="run_test"> | |||
There was a problem hiding this comment.
Be consistent with the quoting within the same line and the ID should also change (or be removed if not required):
| <li class='nav-item' id="run_test"> | |
| <li class="nav-item" id="create_test"> |
| @@ -0,0 +1,122 @@ | |||
| % layout 'bootstrap'; | |||
There was a problem hiding this comment.
We don't use camel-cased path elements here. So please move this file under test. (Is it really that hard to follow the established conventions of a projects. Please don't do everything your way. If everyone would do that this project would end up as a total mess of inconsistencies. And it becomes really annoying having to state this so often.)
There was a problem hiding this comment.
Now I noticed. believe me if I say I didnt try to do anything on my way here. rather trying to find my way. and what happened briefly I create this somewhere else and then I copied and remained as such because I didnt noticed
| <p><b>Hint:</b> Experimental form to trigger a test E.g. see <%= link_to 'this | ||
| link' => url_for('not_found') %>. |
There was a problem hiding this comment.
This way of wrapping it is weird:
| <p><b>Hint:</b> Experimental form to trigger a test E.g. see <%= link_to 'this | |
| link' => url_for('not_found') %>. | |
| <p><b>Hint:</b> Experimental form to trigger a test E.g. see <%= link_to 'this link' => url_for('not_found') %>. |
| link' => url_for('not_found') %>. | ||
| </p> | ||
| <div class="container-md"> | ||
| <form class="row g-3" onsubmit="runExample(this);" data-post-url="/api/v1/isos"> |
There was a problem hiding this comment.
The wrapping in the subsequent HTML code is completely inconsistent.
| link' => url_for('not_found') %>. | ||
| </p> | ||
| <div class="container-md"> | ||
| <form class="row g-3" onsubmit="runExample(this);" data-post-url="/api/v1/isos"> |
There was a problem hiding this comment.
We should not hardcode paths like /api/v1/isos. Use the Mojolicious helper url_for instead.
With this PR a logged in user can trigger a job via a form, providing settings, git repo and a scenarion definition. User doesnt need to have special role. Keep in mind that NEEDLES_DIR doesnt need for os-autoinst-distri-example. Things which stayed out of this PR are: - The UI design - Posibility to upload the yaml definition - Some way to return data to the webUI, like the job id - Input validation - Additional error handling (maybe) https://progress.opensuse.org/issues/162899 Signed-off-by: ybonatakis <[email protected]>
b10n1k
force-pushed
the
162899_distri_example
branch
from
cdbc40f
to
2032ec7
Compare
| <div class="md-12"> | ||
| <label for="scenario_definition" class="form-label col-sm-2 | ||
| control-label"> | ||
| </strong>Scenario Definition</strong></label> | ||
| <textarea type="text" class="form-control" id="scenario_definition" | ||
| rows="15" name="SCENARIO_DEFINITIONS_YAML"> | ||
| --- | ||
| products: | ||
| example: | ||
| distri: "example" | ||
| flavor: "DVD" | ||
| arch: "x86_64" | ||
| version: '0' | ||
| machines: | ||
| 64bit: | ||
| backend: "qemu" | ||
| job_templates: | ||
| simple_boot: | ||
| product: "example" | ||
| machine: "64bit" | ||
| </textarea> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
I would leave this whole block out for now. openQA should read the scenario definitions from the example test distribution itself.
There was a problem hiding this comment.
But having this in the PoC makes it easy to test
|
this has been implemented #5933 |
With this PR a logged in user can trigger a job via a form, providing
settings, git repo and a scenarion definition. User doesnt need to have
special role. Keep in mind that NEEDLES_DIR doesnt need for
os-autoinst-distri-example.
Things which stayed out of this PR are:
https://progress.opensuse.org/issues/162899