Merge ~andersson123/autopkgtest-cloud:all-proposed-patch into autopkgtest-cloud:master
- Git
- lp:~andersson123/autopkgtest-cloud
- all-proposed-patch
- Merge into master
Status: | Merged |
---|---|
Merged at revision: | 64f492d3b7d4f6cb215296986d5599d686ba952d |
Proposed branch: | ~andersson123/autopkgtest-cloud:all-proposed-patch |
Merge into: | autopkgtest-cloud:master |
Diff against target: |
175 lines (+38/-23) 4 files modified
charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker (+21/-6) charms/focal/autopkgtest-web/webcontrol/browse.cgi (+8/-4) charms/focal/autopkgtest-web/webcontrol/download-results (+1/-9) charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html (+8/-4) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Skia | Approve | ||
Review via email: mp+459627@code.launchpad.net |
Commit message
Description of the change
Tim Andersson (andersson123) wrote : | # |
Skia (hyask) wrote : | # |
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.
Tim Andersson (andersson123) : | # |
Tim Andersson (andersson123) wrote : | # |
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) wrote : | # |
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_
Tim Andersson (andersson123) wrote : | # |
I'll also add comments to all my changes to explain why I made them.
Tim Andersson (andersson123) wrote : | # |
(and what the variables are for etc)
Tim Andersson (andersson123) wrote : | # |
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) wrote : | # |
resource for me incase I lose it
Tim Andersson (andersson123) wrote : | # |
amended, I'm just going to test in staging now
Tim Andersson (andersson123) wrote : | # |
Tested in staging, ready for re-review
Skia (hyask) wrote : | # |
Last round of small fixes, nothing big
Tim Andersson (andersson123) : | # |
Tim Andersson (andersson123) wrote : | # |
I kept the sameas for the reason described here:
https:/
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.
Skia (hyask) : | # |
Tim Andersson (andersson123) wrote : | # |
I'll amend
Tim Andersson (andersson123) wrote : | # |
amended, please re-review
Tim Andersson (andersson123) wrote : | # |
Awesome, thank you. I'll wait for CI then I'll merge this.
Skia (hyask) wrote : | # |
Lint issues
Tim Andersson (andersson123) wrote : | # |
I amended the linting issues, just waiting for CI now
Tim Andersson (andersson123) wrote : | # |
CI is tempfail here, I'm merging this
Preview Diff
1 | diff --git a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker | |||
2 | index ea31096..3744f06 100755 | |||
3 | --- a/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker | |||
4 | +++ b/charms/focal/autopkgtest-cloud-worker/autopkgtest-cloud/worker/worker | |||
5 | @@ -79,6 +79,8 @@ ARCH_RELEASE_ALLOW_MAPPING = { | |||
6 | 79 | 79 | ||
7 | 80 | FAIL_CODES = (4, 6, 12, 14, 20) | 80 | FAIL_CODES = (4, 6, 12, 14, 20) |
8 | 81 | 81 | ||
9 | 82 | KEYS_FOR_ADDITIONAL_PARAMS = ["all-proposed"] | ||
10 | 83 | |||
11 | 82 | # In the case of a tmpfail, look for these strings in the log and if they're | 84 | # In the case of a tmpfail, look for these strings in the log and if they're |
12 | 83 | # found, consider this a real failure instead. This is useful if the test | 85 | # found, consider this a real failure instead. This is useful if the test |
13 | 84 | # breaks the testbed so much that autopkgtest can't report a failure properly - | 86 | # breaks the testbed so much that autopkgtest can't report a failure properly - |
14 | @@ -319,7 +321,9 @@ def request_matches_per_package(package, arch, release, s): | |||
15 | 319 | ) | 321 | ) |
16 | 320 | 322 | ||
17 | 321 | 323 | ||
19 | 322 | def process_output_dir(dir, pkgname, code, triggers, all_proposed): | 324 | def process_output_dir( |
20 | 325 | dir, pkgname, code, triggers, additional_params: dict = None | ||
21 | 326 | ): | ||
22 | 323 | """Post-process output directory""" | 327 | """Post-process output directory""" |
23 | 324 | 328 | ||
24 | 325 | files = set(os.listdir(dir)) | 329 | files = set(os.listdir(dir)) |
25 | @@ -349,12 +353,12 @@ def process_output_dir(dir, pkgname, code, triggers, all_proposed): | |||
26 | 349 | json.dump(d, testinfo, indent=True) | 353 | json.dump(d, testinfo, indent=True) |
27 | 350 | files.add("testinfo.json") | 354 | files.add("testinfo.json") |
28 | 351 | 355 | ||
31 | 352 | # add all-proposed to testinfo.json if test was triggered with all-proposed=1 | 356 | if additional_params not in [None, {}]: |
30 | 353 | if all_proposed: | ||
32 | 354 | d = {} | 357 | d = {} |
33 | 355 | with open(os.path.join(dir, "testinfo.json"), "r") as testinfo: | 358 | with open(os.path.join(dir, "testinfo.json"), "r") as testinfo: |
34 | 356 | d = json.load(testinfo) | 359 | d = json.load(testinfo) |
36 | 357 | d["all-proposed"] = 1 | 360 | for key, val in additional_params.items(): |
37 | 361 | d[key] = val | ||
38 | 358 | with open(os.path.join(dir, "testinfo.json"), "w") as testinfo: | 362 | with open(os.path.join(dir, "testinfo.json"), "w") as testinfo: |
39 | 359 | json.dump(d, testinfo, indent=True) | 363 | json.dump(d, testinfo, indent=True) |
40 | 360 | 364 | ||
41 | @@ -1243,12 +1247,23 @@ def request(msg): | |||
42 | 1243 | else: | 1247 | else: |
43 | 1244 | f.write("%s\n" % params["readable-by"]) | 1248 | f.write("%s\n" % params["readable-by"]) |
44 | 1245 | 1249 | ||
45 | 1250 | # List of key=value strings to be sent as a rabbitmq msg | ||
46 | 1251 | # These variables are optional additional parameters users | ||
47 | 1252 | # can employ when requesting a test which alters the behaviour | ||
48 | 1253 | additional_parameters = [] | ||
49 | 1254 | # We also create it as a dict to easily insert into a json file | ||
50 | 1255 | additional_parameters_for_testinfo = {} | ||
51 | 1256 | for key in KEYS_FOR_ADDITIONAL_PARAMS: | ||
52 | 1257 | if params.get(key, None) is not None: | ||
53 | 1258 | additional_parameters.append("%s=%s" % (key, params[key])) | ||
54 | 1259 | additional_parameters_for_testinfo[key] = params[key] | ||
55 | 1260 | |||
56 | 1246 | (testpkg_version, duration, requester) = process_output_dir( | 1261 | (testpkg_version, duration, requester) = process_output_dir( |
57 | 1247 | out_dir, | 1262 | out_dir, |
58 | 1248 | pkgname, | 1263 | pkgname, |
59 | 1249 | code, | 1264 | code, |
60 | 1250 | params.get("triggers", []), | 1265 | params.get("triggers", []), |
62 | 1251 | True if params.get("all-proposed", 0) == 1 else False, | 1266 | additional_parameters_for_testinfo, # embed additional parameters in testinfo.json |
63 | 1252 | ) | 1267 | ) |
64 | 1253 | 1268 | ||
65 | 1254 | # If two tests for the same package with different triggers finish at the | 1269 | # If two tests for the same package with different triggers finish at the |
66 | @@ -1371,7 +1386,7 @@ def request(msg): | |||
67 | 1371 | "requester": requester, | 1386 | "requester": requester, |
68 | 1372 | "swift_dir": swift_dir, | 1387 | "swift_dir": swift_dir, |
69 | 1373 | "triggers": triggers, | 1388 | "triggers": triggers, |
71 | 1374 | "all-proposed": 1 if "all-proposed" in params else 0, | 1389 | "env": ",".join(additional_parameters), |
72 | 1375 | } | 1390 | } |
73 | 1376 | ) | 1391 | ) |
74 | 1377 | complete_amqp.basic_publish( | 1392 | complete_amqp.basic_publish( |
75 | diff --git a/charms/focal/autopkgtest-web/webcontrol/browse.cgi b/charms/focal/autopkgtest-web/webcontrol/browse.cgi | |||
76 | index 9a86a66..4772be2 100755 | |||
77 | --- a/charms/focal/autopkgtest-web/webcontrol/browse.cgi | |||
78 | +++ b/charms/focal/autopkgtest-web/webcontrol/browse.cgi | |||
79 | @@ -305,7 +305,7 @@ def package_release_arch(package, release, arch, _=None): | |||
80 | 305 | code = human_exitcode(row[4]) | 305 | code = human_exitcode(row[4]) |
81 | 306 | version = row[1] | 306 | version = row[1] |
82 | 307 | triggers = row[2] | 307 | triggers = row[2] |
84 | 308 | env = row[ | 308 | additional_params = row[ |
85 | 309 | 6 | 309 | 6 |
86 | 310 | ] # string of comma separated env variables e.g. all-proposed=1,test-name=mytest | 310 | ] # string of comma separated env variables e.g. all-proposed=1,test-name=mytest |
87 | 311 | 311 | ||
88 | @@ -323,18 +323,22 @@ def package_release_arch(package, release, arch, _=None): | |||
89 | 323 | package, | 323 | package, |
90 | 324 | row[0], | 324 | row[0], |
91 | 325 | ) | 325 | ) |
92 | 326 | all_proposed = ( | ||
93 | 327 | additional_params is not None | ||
94 | 328 | and "all-proposed=1" in additional_params | ||
95 | 329 | ) | ||
96 | 326 | results.append( | 330 | results.append( |
97 | 327 | ( | 331 | ( |
98 | 328 | version, | 332 | version, |
99 | 329 | triggers, | 333 | triggers, |
101 | 330 | env, | 334 | additional_params, |
102 | 331 | human_date(row[0]), | 335 | human_date(row[0]), |
104 | 332 | human_sec(row[3]), | 336 | human_sec(int(row[3])), |
105 | 333 | requester, | 337 | requester, |
106 | 334 | code, | 338 | code, |
107 | 335 | url, | 339 | url, |
108 | 336 | show_retry, | 340 | show_retry, |
110 | 337 | "&all-proposed=1" if "all-proposed=1" in env else "", | 341 | all_proposed, |
111 | 338 | ) | 342 | ) |
112 | 339 | ) | 343 | ) |
113 | 340 | 344 | ||
114 | diff --git a/charms/focal/autopkgtest-web/webcontrol/download-results b/charms/focal/autopkgtest-web/webcontrol/download-results | |||
115 | index ca0de53..c36b5ab 100755 | |||
116 | --- a/charms/focal/autopkgtest-web/webcontrol/download-results | |||
117 | +++ b/charms/focal/autopkgtest-web/webcontrol/download-results | |||
118 | @@ -79,14 +79,6 @@ def process_message(msg, db_con): | |||
119 | 79 | msg.channel.basic_ack(msg.delivery_tag) | 79 | msg.channel.basic_ack(msg.delivery_tag) |
120 | 80 | return | 80 | return |
121 | 81 | 81 | ||
122 | 82 | env_vars = [] | ||
123 | 83 | env_spec = { | ||
124 | 84 | "all-proposed": "all-proposed=1", | ||
125 | 85 | } | ||
126 | 86 | for env, spec in env_spec.items(): | ||
127 | 87 | if env in info: | ||
128 | 88 | env_vars.append(spec) | ||
129 | 89 | |||
130 | 90 | test_id = get_test_id(db_con, release, arch, package) | 82 | test_id = get_test_id(db_con, release, arch, package) |
131 | 91 | 83 | ||
132 | 92 | try: | 84 | try: |
133 | @@ -101,7 +93,7 @@ def process_message(msg, db_con): | |||
134 | 101 | duration, | 93 | duration, |
135 | 102 | exitcode, | 94 | exitcode, |
136 | 103 | requester, | 95 | requester, |
138 | 104 | ",".join(env_vars), | 96 | info.get("env", ""), |
139 | 105 | ), | 97 | ), |
140 | 106 | ) | 98 | ) |
141 | 107 | db_con.commit() | 99 | db_con.commit() |
142 | diff --git a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html | |||
143 | index 6ae5ab8..3da1e24 100644 | |||
144 | --- a/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html | |||
145 | +++ b/charms/focal/autopkgtest-web/webcontrol/templates/browse-results.html | |||
146 | @@ -20,21 +20,25 @@ | |||
147 | 20 | <td>{{row[1]}}</td> | 20 | <td>{{row[1]}}</td> |
148 | 21 | <td>{{row[2]}}</td> | 21 | <td>{{row[2]}}</td> |
149 | 22 | <td>{{row[3]}}</td> | 22 | <td>{{row[3]}}</td> |
150 | 23 | <td class="{{row[4]}}">{{row[4]}}</td> | ||
151 | 23 | <td> | 24 | <td> |
153 | 24 | {% if row[4] != "-" %} | 25 | {% if row[5] != "-" %} |
154 | 25 | <a href="https://launchpad.net/~{{row[4]}}">{{row[4]}}</a> | 26 | <a href="https://launchpad.net/~{{row[4]}}">{{row[4]}}</a> |
155 | 26 | {% else %} | 27 | {% else %} |
157 | 27 | {{row[4]}} | 28 | {{row[5]}} |
158 | 28 | {% endif %} | 29 | {% endif %} |
159 | 29 | </td> | 30 | </td> |
160 | 30 | <td class="{{row[5]}}">{{row[5]}}</td> | ||
161 | 31 | <td class="nowrap"> | 31 | <td class="nowrap"> |
162 | 32 | <a href="{{row[7]}}/log.gz">log</a>   | 32 | <a href="{{row[7]}}/log.gz">log</a>   |
163 | 33 | <a href="{{row[7]}}/artifacts.tar.gz">artifacts</a>   | 33 | <a href="{{row[7]}}/artifacts.tar.gz">artifacts</a>   |
164 | 34 | {% if row[8] %} | 34 | {% if row[8] %} |
165 | 35 | {% set ts = row[1].split()|map('urlencode')|join("&trigger=")|safe %} | 35 | {% set ts = row[1].split()|map('urlencode')|join("&trigger=")|safe %} |
166 | 36 | {% set all_proposed = row[9] %} | 36 | {% set all_proposed = row[9] %} |
168 | 37 | <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}{{all_proposed}}">♻</a> | 37 | {% if all_proposed %} |
169 | 38 | <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}&all-proposed=1">♻</a> | ||
170 | 39 | {% else %} | ||
171 | 40 | <a href="{{base_url}}request.cgi?release={{release}}&arch={{arch}}&package={{package|urlencode}}&trigger={{ts}}">♻</a> | ||
172 | 41 | {% endif %} | ||
173 | 38 | {% endif %} | 42 | {% endif %} |
174 | 39 | </td> | 43 | </td> |
175 | 40 | </tr> | 44 | </tr> |
Needs testing in staging.