Fix the issue _URL in jobs post is different from that in isos post

[Amrysliu]

Related: https://progress.opensuse.org/issues/66268

[Amrysliu]

Here is a verify run: http://10.67.19.103/tests/60#settings.
I triggered this job using

liuxiaojing@linux-90dc:~/code/repos/openQA/script> ./openqa-cli api --host http://10.67.19.103 -X post jobs TEST=test DISTRI=sle VERSION=15-SP2 ARCH=x86_64 FLAVOR=Online MACHINE=64bit HDD_1_URL=http://10.67.19.103/tests/56/asset/hdd/sle-15-SP2-x86_64-189.1-textmode@64bit.qcow2 BUILD=189.1
{"id":60}

@AdamWill I wonder how do you use the _URL in your test when using isos post? such as isos post HDD_1_URL=http://.../foo.qcow2?

[AdamWill]

Pretty much, yeah. We set ISO_URL, HDD_2_DECOMPRESS_URL, HDD_1_URL, KERNEL_URL and/or INITRD_URL depending on the asset type. Sometimes we also specify the non-_URL parameter as well to give a custom name, sometimes we don't. The URLs are full URLs to images from Fedora nightly composes, so a typical job might have something like ISO_URL=https://kojipkgs.fedoraproject.org/compose/rawhide/Fedora-Rawhide-20200505.n.0/compose/Everything/x86_64/iso/Fedora-Everything-netinst-x86_64-Rawhide-20200505.n.0.iso .

Is that what you were asking?

[AdamWill]

So I'm not sure if it matters, but I do still see some ordering difference with the ISO path here. On the ISO path we do create_downloads_list before we call $self->_generate_jobs, which is what calls OpenQA::JobSettings::generate_settings on that path; even with this change, on this path we are still calling OpenQA::JobSettings::generate_settings before we do create_downloads_list. That means we're processing vars prefixed with + and doing the placeholder replacement before we call create_downloads_list on this path, but after we call create_downloads_list on the ISO path...

[codecov]

Codecov Report

Merging #3049 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3049   +/-   ##
=======================================
  Coverage   92.03%   92.03%           
=======================================
  Files         211      211           
  Lines       12846    12846           
=======================================
  Hits        11823    11823           
  Misses       1023     1023           

Impacted Files	Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/Job.pm	90.85% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f46d46c...8b254f6. Read the comment docs.

[okurz]

Approved but not merging to give the opportunity to discuss the ordering diffs that Adam mentioned.

Side-note: t/api/04-jobs.t is by far the longest running test module within the API tests and also the file size is above a sane maintainable level. Please consider to work on this, e.g. either strip less relevant tests without loosing test coverage or split the test module or move tests into other test modules.

[Amrysliu]

Thanks for replying.

Is that what you were asking?

yes, that is what I wonder.

So I'm not sure if it matters, but I do still see some ordering difference with the ISO path here. On the ISO path we do create_downloads_list before we call $self->_generate_jobs, which is what calls OpenQA::JobSettings::generate_settings on that path; even with this change, on this path we are still calling OpenQA::JobSettings::generate_settings before we do create_downloads_list. That means we're processing vars prefixed with + and doing the placeholder replacement before we call create_downloads_list on this path, but after we call create_downloads_list on the ISO path...

The document says If you set both ISO_1 and ISO_1_URL, the file pointed to by ISO_1_URL will be downloaded and renamed to the name set as ISO_1 . In jobs post if the ISO_1 is set in test suite settings, the download's file will be renamed by this value (You could see that through the unit test code I added) . But in isos post, if the ISO_1 is set in test suite settings or other places, the download file will not be renamed to it. So I put the create_downloads_list after _generate_jobs. jobs post is different from isos post, it only create one job at one time, so I think this way does not matter. This is why I wondered about your workflow :)

[AdamWill]

Ah, I see :)

Yeah, I don't think there is any case where we set a _URL arg and the matching non-_URL arg is set in the templates. We have some asset args in the templates but only for fixed assets. We only use the _URL stuff in scheduler code, so the relevant args are always coming in with the request.

[Amrysliu]

@AdamWill Do you have any suggestion about this pr? or any question?

@okurz @Martchus May I merge this pr after confirming that AdamWill agrees on this change?

[Martchus]

Sounds good. Considering this PR only influenced posting a single job (and not scheduling an ISO) he'll be likely not influenced by the change anyways.

[okurz]

There had been no "please don't merge" so I guess we can merge :)

[AdamWill]

My power has been out all morning :)

I think this was fine to merge, it's certainly better than before.