Multiple bug fixes and improvements for template dump/load

[AdamWill]

This series fixes a ton of issues I noticed in template dumping/loading while investigating https://progress.opensuse.org/issues/181895 . To summarize:

• Loading JobTemplates is fixed by reverting to sending the request with form data, not as JSON
• Two logic bugs in openqa-dump-templates which meant it inadvertently almost never dumped JobTemplates are fixed
• Behavior of openqa-load-templates when a job group already exists is brought into line with other forms of data (modify the group if --update or --clean are set, ignore it and move on if not), and we rely on the API to tell us whether the group already exists instead of explicitly checking
• We make openqa-dump-templates by default dump job groups / job templates in the format corresponding with how they exist on the server: legacy groups/templates are dumped to JobTemplates, modern YAML groups/templates are dumped to JobGroups; however, we add a --convert arg that will always dump to JobGroups, effectively "converting" legacy groups to modern (this is how it behaved before the whole patch series, but by default and more or less by accident)
• The tests are substantially improved and extended to more clearly cover all the expected behaviors

[AdamWill]

OK, I found a way to finesse the patch series: I combined the three major fixes to dump-templates into one. So the fix for accidentally defining $options{group}, the broken product filter line, and the change to dumping as the data exist on the server all go together.

As well as meaning the tests pass at each point, this actually makes sense. If we fix the two bugs before we change the dumping behavior, that makes us dump in both formats by default, which really isn't a good idea. And vice versa, if we do the dump-in-native-format-by-default fix first, it doesn't really work as intended because we'll actually fail to dump 'legacy' data entirely due to the logic bugs. So I think this isn't just convenient, it's also right.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.08%. Comparing base (98c95ea) to head (c8c91b6).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6446      +/-   ##
==========================================
+ Coverage   99.06%   99.08%   +0.02%     
==========================================
  Files         399      399              
  Lines       40327    40401      +74     
==========================================
+ Hits        39949    40031      +82     
+ Misses        378      370       -8     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[AdamWill]

commit messages rewritten by J. Jonah Jameson, wall-of-text factor reduced by 43.786%. todo: the first commit doesn't quite do everything the message claims yet, I'll fix that in the morning. and add the missing coverage lines.

[Martchus]

The change looks generally good but I have a few nitpicks.

Unfortunately not everything is covered according to codecov so it would be nice to add tests for those cases as well. Otherwise I'd also be fine with marking lines as uncoverable for these scripts as we don't care much about them anymore from our side. (But maybe with a remark that the code is not really uncoverable and we just avoiding the effort here.)

[AdamWill]

I'm going to add a test for the uncovered bit, it's in the todo note above. It's not too hard to do. There's a couple of other things I still need to add to the tests too.

[AdamWill]

wow, tidy is a psychopath. it flat out refuses to allow braces on the next line. I have no words.

[adamw@toolbx openQA (template-dump-load-bugs %)]$ ./tools/tidyall -a
[tidied]  script/openqa-dump-templates
[adamw@toolbx openQA (template-dump-load-bugs *%)]$ git diff
diff --git a/script/openqa-dump-templates b/script/openqa-dump-templates
index af926520d..54adb28b5 100755
--- a/script/openqa-dump-templates
+++ b/script/openqa-dump-templates
@@ -275,8 +275,7 @@ sub jt_key ($jt) {
         $jt->{group_name},
         $jt->{test_suite}->{name},
         $jt->{machine}->{name},
-        $prod->{arch}, $prod->{distri}, $prod->{flavor}, $prod->{version}
-    );
+        $prod->{arch}, $prod->{distri}, $prod->{flavor}, $prod->{version});
 }
 
 sub handle_table ($table) {

I'm calling the UN.

[AdamWill]

to preserve a note on the whole "our test fixtures are logically impossible" thing: this situation developed during the big YAML template introduction, specifically in e458cda (added settings to a job template in the 'opensuse' group) and 819374a (added a description to another one). e458cda was the 'original sin' - it should either have made the 'opensuse' job group in the fixtures into a YAML group (by giving it a YAML template string and dropping explicit creation of the job templates in 04-products.pl) or used a separate YAML job group.

Unfortunately, fixing this isn't trivial - if you turn the opensuse group into a YAML group now, several other tests start failing, including some of the YAML tests themselves. Adding a new YAML group fixture and adjusting appropriate tests to use it instead also isn't trivial. I didn't feel like putting the effort into fixing this mess, since I didn't create it and this PR was already taking forever. I might file a progress ticket pointing the problem out, though, for anybody else to fix if they feel so inclined.

[AdamWill]

@Martchus @okurz let me know if you want me to change the things I left outstanding; I'm happy with the state of this now.

[okurz]

I suggest to also use signatures in newly added code but the current state is ok. Actually really really good so approved :)

[AdamWill]

OK, I did the signatures. I also snuck in a new check for --clean and --update with YAML templates: with the changes in "job groups: simplify, don't error on group exists" we actually get almost correct behavior for free, since the only sensible thing for --update or --clean to do for a JobGroup is to re-POST the YAML string. The only thing that's wrong is that (IMO) --clean should wipe the YAML strings for any groups that exist on the server but are not in the data being loaded, but it does not. I added this check in the final commit rather than the "job groups: simplify, don't error on group exists" commit because it's convenient to use the 40-templates-jgs.pl that I added in that commit, but I suppose I could move the addition of that file up into the earlier commit if we're really being obsessive.

[Martchus]

Looks good now and thanks for the detailed explanations.

---

wow, tidy is a psychopath.

Yes, it sometimes often overshoots the goal.