Merge ~andersson123/autopkgtest-cloud:all-proposed-patch into autopkgtest-cloud:master

[Tim Andersson]

[Tim Andersson (andersson123)]

Needs testing in staging.

[Skia (hyask)]

I've added a few inline comments. If you want to discuss the approach I'm proposing, feel free to ping me for a call.

review: Needs Fixing

[Tim Andersson (andersson123)]

I've tested this this morning with cowboys + charms etc. Now that I've rebased and cleaned up the commits, I'm going to build the charms one last time and re-test, then request review.

[Tim Andersson (andersson123)]

per Skia's review in a call we're having right now, I'll rename all the variables that use 'env' or something similar, to like additional_params

rabbitmq key will stay as "env"
database column will stay as "env"

e.g.
VARS_FOR_ENV
->
KEYS_FOR_ADDITIONAL_PARAMS

[Tim Andersson (andersson123)]

I'll also add comments to all my changes to explain why I made them.

[Tim Andersson (andersson123)]

(and what the variables are for etc)

[Tim Andersson (andersson123)]

I also should amend the way all-proposed is preserved in the retry link. Use a boolean value and have the jinja template handle the value, and incorporate "&all-proposed=1" in the retry url.

[Tim Andersson (andersson123)]

https://stackoverflow.com/questions/8433450/why-doesnt-my-condition-logic-work-as-expected-in-jinja2-cherrypy

resource for me incase I lose it

[Tim Andersson (andersson123)]

amended, I'm just going to test in staging now

[Tim Andersson (andersson123)]

Tested in staging, ready for re-review

[Skia (hyask)]

Last round of small fixes, nothing big

review: Needs Fixing

[Tim Andersson (andersson123)]

I kept the sameas for the reason described here:
https://stackoverflow.com/a/49331424

in the case the code gets changed and accidentally passes all_proposed as an int with value either 0 or 1, sameas will cause a traceback - which is intended since all_proposed should only be a boolean.

[Tim Andersson (andersson123)]

I'll amend

[Tim Andersson (andersson123)]

amended, please re-review

[Skia (hyask)]

Great, thanks!

review: Approve

[Tim Andersson (andersson123)]

Awesome, thank you. I'll wait for CI then I'll merge this.

[Skia (hyask)]

Lint issues

[Tim Andersson (andersson123)]

I amended the linting issues, just waiting for CI now

[Tim Andersson (andersson123)]

CI is tempfail here, I'm merging this

diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
index ea31096..3744f06 100755
--- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
+++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker
@@ -79,6 +79,8 @@ ARCH_RELEASE_ALLOW_MAPPING = {
 
 FAIL_CODES = (4, 6, 12, 14, 20)
 
+KEYS_FOR_ADDITIONAL_PARAMS = ["all-proposed"]
+
 # In the case of a tmpfail, look for these strings in the log and if they're
 # found, consider this a real failure instead. This is useful if the test
 # breaks the testbed so much that autopkgtest can't report a failure properly -
@@ -319,7 +321,9 @@ def request_matches_per_package(package, arch, release, s):
     )
 
 
-def process_output_dir(dir, pkgname, code, triggers, all_proposed):
+def process_output_dir(
+    dir, pkgname, code, triggers, additional_params: dict = None
+):
     """Post-process output directory"""
 
     files = set(os.listdir(dir))
@@ -349,12 +353,12 @@ def process_output_dir(dir, pkgname, code, triggers, all_proposed):
                 json.dump(d, testinfo, indent=True)
             files.add("testinfo.json")
 
-    # add all-proposed to testinfo.json if test was triggered with all-proposed=1
-    if all_proposed:
+    if additional_params not in [None, {}]:
         d = {}
         with open(os.path.join(dir, "testinfo.json"), "r") as testinfo:
             d = json.load(testinfo)
-        d["all-proposed"] = 1
+        for key, val in additional_params.items():
+            d[key] = val
         with open(os.path.join(dir, "testinfo.json"), "w") as testinfo:
             json.dump(d, testinfo, indent=True)
 
@@ -1243,12 +1247,23 @@ def request(msg):
                 else:
                     f.write("%s\n" % params["readable-by"])
 
+        # List of key=value strings to be sent as a rabbitmq msg
+        # These variables are optional additional parameters users
+        # can employ when requesting a test which alters the behaviour
+        additional_parameters = []
+        # We also create it as a dict to easily insert into a json file
+        additional_parameters_for_testinfo = {}
+        for key in KEYS_FOR_ADDITIONAL_PARAMS:
+            if params.get(key, None) is not None:
+                additional_parameters.append("%s=%s" % (key, params[key]))
+                additional_parameters_for_testinfo[key] = params[key]
+
         (testpkg_version, duration, requester) = process_output_dir(
             out_dir,
             pkgname,
             code,
             params.get("triggers", []),
-            True if params.get("all-proposed", 0) == 1 else False,
+            additional_parameters_for_testinfo,  # embed additional parameters in testinfo.json
         )
 
         # If two tests for the same package with different triggers finish at the
@@ -1371,7 +1386,7 @@ def request(msg):
             "requester": requester,
             "swift_dir": swift_dir,
             "triggers": triggers,
-            "all-proposed": 1 if "all-proposed" in params else 0,
+            "env": ",".join(additional_parameters),
         }
     )
     complete_amqp.basic_publish(
diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
index 9a86a66..4772be2 100755
--- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi
+++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi
@@ -305,7 +305,7 @@ def package_release_arch(package, release, arch, _=None):
         code = human_exitcode(row[4])
         version = row[1]
         triggers = row[2]
-        env = row[
+        additional_params = row[
             6
         ]  # string of comma separated env variables e.g. all-proposed=1,test-name=mytest
 
@@ -323,18 +323,22 @@ def package_release_arch(package, release, arch, _=None):
             package,
             row[0],
         )
+        all_proposed = (
+            additional_params is not None
+            and "all-proposed=1" in additional_params
+        )
         results.append(
             (
                 version,
                 triggers,
-                env,
+                additional_params,
                 human_date(row[0]),
-                human_sec(row[3]),
+                human_sec(int(row[3])),
                 requester,
                 code,
                 url,
                 show_retry,
-                "&all-proposed=1" if "all-proposed=1" in env else "",
+                all_proposed,
             )
         )
 
diff --git a/charms/focal/autopkgtest-web/webcontrol/download-results b/charms/focal/autopkgtest-web/webcontrol/download-results
index ca0de53..c36b5ab 100755
--- a/charms/focal/autopkgtest-web/webcontrol/download-results
+++ b/charms/focal/autopkgtest-web/webcontrol/download-results
@@ -79,14 +79,6 @@ def process_message(msg, db_con):
         msg.channel.basic_ack(msg.delivery_tag)
         return
 
-    env_vars = []
-    env_spec = {
-        "all-proposed": "all-proposed=1",
-    }
-    for env, spec in env_spec.items():
-        if env in info:
-            env_vars.append(spec)
-
     test_id = get_test_id(db_con, release, arch, package)
 
     try:
@@ -101,7 +93,7 @@ def process_message(msg, db_con):
                 duration,
                 exitcode,
                 requester,
-                ",".join(env_vars),
+                info.get("env", ""),
             ),
         )
         db_con.commit()
diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
index 6ae5ab8..3da1e24 100644
--- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
+++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html
@@ -20,21 +20,25 @@
       <td>{{row[1]}}</td>
       <td>{{row[2]}}</td>
       <td>{{row[3]}}</td>
+      <td class="{{row[4]}}">{{row[4]}}</td>
       <td>
-        {% if row[4] != "-" %}
+        {% if row[5] != "-" %}
           <a href="https://launchpad.net/~{{row[4]}}">{{row[4]}}</a>
         {% else %}
-          {{row[4]}}
+          {{row[5]}}
         {% endif %}
       </td>
-      <td class="{{row[5]}}">{{row[5]}}</td>
       <td class="nowrap">
         <a href="{{row[7]}}/log.gz">log</a> &emsp;
         <a href="{{row[7]}}/artifacts.tar.gz">artifacts</a> &emsp;
         {% if row[8] %}
           {% set ts = row[1].split()|map('urlencode')|join("&trigger=")|safe %}
           {% set all_proposed = row[9] %}
-          <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}{{all_proposed}}">&#9851;</a>
+          {% if all_proposed %}
+            <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}&all-proposed=1">&#9851;</a>
+          {% else %}
+            <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}">&#9851;</a>
+          {% endif %}
         {% endif %}
       </td>
     </tr>