| mnaser | clarkb: yeah, this landed on limestone... so i'm trying to recreate the playbook locally | 00:00 |
|---|---|---|
| *** shanemcd has joined #zuul | 00:04 | |
| *** dangtrinhnt has joined #zuul | 00:21 | |
| mnaser | interesting, it looks like it worked just fine here locally when i replicated it. i'm re-running with verbose.. | 00:29 |
| *** rlandy has quit IRC | 00:36 | |
| *** dangtrinhnt has quit IRC | 00:37 | |
| *** dangtrinhnt has joined #zuul | 00:44 | |
| *** saneax has joined #zuul | 00:50 | |
| *** dangtrinhnt has quit IRC | 00:58 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 01:02 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 01:11 |
| *** dangtrinhnt has joined #zuul | 01:17 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 01:23 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 01:34 |
| *** swest has quit IRC | 01:35 | |
| *** dangtrinhnt has quit IRC | 01:37 | |
| *** dangtrinhnt_ has joined #zuul | 01:37 | |
| *** dangtrinhnt_ has quit IRC | 01:45 | |
| *** rfolco has quit IRC | 01:47 | |
| *** dangtrinhnt has joined #zuul | 01:47 | |
| *** swest has joined #zuul | 01:51 | |
| *** dangtrinhnt has quit IRC | 01:52 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 01:59 |
| *** dangtrinhnt has joined #zuul | 02:01 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 02:10 |
| *** ysandeep|away is now known as ysandeep|rover | 02:31 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 02:33 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 02:33 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 02:47 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 02:58 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 03:05 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 03:12 |
| *** dangtrinhnt has quit IRC | 03:25 | |
| *** bolg has quit IRC | 03:34 | |
| *** bhavikdbavishi has joined #zuul | 03:42 | |
| *** bhavikdbavishi1 has joined #zuul | 03:45 | |
| *** bhavikdbavishi has quit IRC | 03:46 | |
| *** bhavikdbavishi1 is now known as bhavikdbavishi | 03:46 | |
| *** dangtrinhnt has joined #zuul | 04:00 | |
| *** evrardjp has quit IRC | 04:36 | |
| *** evrardjp has joined #zuul | 04:36 | |
| *** sgw has quit IRC | 05:07 | |
| *** sgw has joined #zuul | 05:24 | |
| AJaeger | mnaser, tobiash, please check the Zuul output in https://review.opendev.org/#/c/716334/ - lots of "Job openstack-tox-py37: unable to map line for file comments: ", can we avoid that? | 05:34 |
| *** bolg has joined #zuul | 05:39 | |
| AJaeger | same failure on https://review.opendev.org/716377 | 05:42 |
| AJaeger | and again on recheck of 716334 | 05:49 |
| *** bhavikdbavishi has quit IRC | 05:51 | |
| *** bhavikdbavishi has joined #zuul | 05:56 | |
| *** bhavikdbavishi has quit IRC | 06:16 | |
| *** bhavikdbavishi has joined #zuul | 06:37 | |
| tobiash | Zuul emits this warning if unchanged files are part of the returned line comments | 06:38 |
| tobiash | corvus: shall we disable those warnings? | 06:39 |
| tobiash | AJaeger: also looks like it did emit some false positives | 06:40 |
| AJaeger | tobiash: false positives? Not good. I suggest to disable those warnings - or show one at most | 06:45 |
| tobiash | That seems to report line comments for stdin which seems like it matched a line that's not intended as line comment | 06:46 |
| *** irclogbot_1 has quit IRC | 06:49 | |
| *** smyers has quit IRC | 06:51 | |
| *** tobiash has quit IRC | 06:51 | |
| *** jhesketh has quit IRC | 06:52 | |
| *** smyers has joined #zuul | 06:53 | |
| *** jhesketh has joined #zuul | 06:53 | |
| *** tobiash has joined #zuul | 06:53 | |
| *** fbo has quit IRC | 06:54 | |
| *** irclogbot_1 has joined #zuul | 06:55 | |
| *** fbo has joined #zuul | 07:00 | |
| *** jcapitao has joined #zuul | 07:02 | |
| *** ysandeep|rover is now known as ysandeep|brb | 07:03 | |
| *** avass is now known as Guest4471 | 07:11 | |
| *** dangtrinhnt has quit IRC | 07:11 | |
| *** avass has joined #zuul | 07:11 | |
| *** dangtrinhnt_ has joined #zuul | 07:12 | |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 07:22 |
| *** bhavikdbavishi has quit IRC | 07:25 | |
| *** tosky has joined #zuul | 07:32 | |
| *** ofosos has joined #zuul | 07:37 | |
| *** ysandeep|brb is now known as ysandeep | 07:52 | |
| *** jpena|off is now known as jpena | 07:56 | |
| *** ysandeep is now known as ysandeep|rover | 07:57 | |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 08:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 08:19 |
| *** bhavikdbavishi has joined #zuul | 08:22 | |
| bolg | zuul-maint: there are several changes adapting tests to prepare them for multi-scheduler. Most of them are reviewed with 2* +2, missing due to recent changes: https://review.opendev.org/c/708812/13, https://review.opendev.org/c/709542/9, https://review.opendev.org/c/709735/8, https://review.opendev.org/c/712958/7 they do not introduce any braking changes and touch only the tests. Could someone please have a look? | 08:40 |
| *** dangtrinhnt has joined #zuul | 08:44 | |
| *** dangtrinhnt_ has quit IRC | 08:48 | |
| *** kmalloc has quit IRC | 08:52 | |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds variable to toggle whether to revoke sudo https://review.opendev.org/706248 | 09:05 |
| openstackgerrit | Sorin Sbarnea proposed zuul/zuul master: WIP: Enable ANSI rendering on stdout/stderr https://review.opendev.org/716251 | 09:12 |
| *** dangtrinhnt has quit IRC | 09:16 | |
| *** dangtrinhnt_ has joined #zuul | 09:17 | |
| *** dangtrinhnt_ has quit IRC | 10:26 | |
| *** jcapitao is now known as jcapitao_lunch | 10:37 | |
| *** ysandeep|rover is now known as ysandeep|afk | 10:56 | |
| *** harrymichal has quit IRC | 11:01 | |
| *** harrymichal has joined #zuul | 11:02 | |
| zbr | I still see CORS header 'Access-Control-Allow-Origin' on job-output.json.gz. do we have a CR to address it? | 11:09 |
| *** harrymichal has quit IRC | 11:21 | |
| *** harrymichal has joined #zuul | 11:22 | |
| *** ysandeep|afk is now known as ysandeep|rover | 11:22 | |
| *** dangtrinhnt has joined #zuul | 11:30 | |
| *** bhavikdbavishi has quit IRC | 11:36 | |
| AJaeger | tobiash, mnaser, another problem, please check https://zuul.opendev.org/t/openstack/build/f3f1712820c54140b5badf9c64ee6f37 | 11:42 |
| *** jpena is now known as jpena|lunch | 11:42 | |
| AJaeger | this passed py27 and failed in the parser for pep8 | 11:42 |
| tobiash | AJaeger: another revert? | 11:43 |
| AJaeger | tobiash: if we can't fix it - yes :( | 11:44 |
| *** cdearborn has joined #zuul | 11:44 | |
| AJaeger | let's first see whether anybody has a fix for it | 11:45 |
| tobiash | Depends on the urgency, I'm sure we can fix it | 11:45 |
| tobiash | But right now I don't have time to work on it | 11:45 |
| cdearborn | FYI, the py27 gate job is broken for python-dracclient for both stable/queens and stable/stein | 11:46 |
| cdearborn | see: https://review.opendev.org/#/c/716100/ and https://review.opendev.org/#/c/716101/ | 11:46 |
| cdearborn | error is: UnicodeEncodeError: 'utf-8' codec can't encode character '\udcc0' in position 17520: surrogates not allowed | 11:47 |
| cdearborn | any ideas? | 11:47 |
| AJaeger | cdearborn: we know what it is - a job change. Question is how to fix | 11:48 |
| AJaeger | cdearborn: please give us some time to investigate - if we cannot fix it, we'll revert the change | 11:48 |
| tobiash | AJaeger: I think that unicode failure is easy to fix | 11:49 |
| tobiash | just a sec | 11:49 |
| cdearborn | thx! | 11:50 |
| arxcruz | Hi guys, i'm trying to add my role into one job in openstack, and i'm getting this error: | 11:53 |
| arxcruz | ERROR Ansible plugin dir /var/lib/zuul/builds/fb47578b4bec4df09f82d3e417e5e4b6/ansible/pre_playbook_1/role_1/ansible-role-collect-logs/filter_plugins found adjacent to playbook /var/lib/zuul/builds/fb47578b4bec4df09f82d3e417e5e4b6/ansible/pre_playbook_1/role_1/ansible-role-collect-logs in non-trusted repo | 11:53 |
| arxcruz | ansible-role-collect-logs is a trusted repo | 11:53 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: WIP: Try to fix unicode issue when parsing tox https://review.opendev.org/716560 | 11:53 |
| tobiash | cdearborn: can you test that with a depends-on? ^ | 11:53 |
| arxcruz | the review is https://review.opendev.org/#/c/702676/ | 11:54 |
| cdearborn | @tobiash: sure | 11:54 |
| tobiash | arxcruz: inline filter plugins are not allowed for untrusted playbooks due to security reasons in zuul | 11:55 |
| arxcruz | tobiash: i might be wrong, but ansible-role-collect-logs was supposed to be a trusted one | 11:55 |
| *** dangtrinhnt has quit IRC | 11:55 | |
| arxcruz | tobiash: it's under openstack umbrella | 11:55 |
| AJaeger | tobiash: a trusted jobs are defined in project-config | 11:56 |
| AJaeger | arxcruz: ^ | 11:56 |
| *** dangtrinhnt_ has joined #zuul | 11:56 | |
| AJaeger | arxcruz: so, for Zuul it's not trusted | 11:56 |
| arxcruz | AJaeger: is it possible to add it to project-config ? | 11:56 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Ignore errors when parsing tox output https://review.opendev.org/716561 | 11:59 |
| AJaeger | arxcruz: I don't understand what you're doing here at all with your debugging. And I can't help you further myself. you might need to wait for later and discuss with others. | 11:59 |
| tobiash | AJaeger, cdearborn: and this should permanently ignore errors in that task ^ | 11:59 |
| AJaeger | tobiash: Thanks! | 11:59 |
| arxcruz | AJaeger: I'm trying to replace openstack-ansible log script with the ansible-role-collect-logs | 12:00 |
| arxcruz | AJaeger: but thanks, your information were valuable I'll check on my side now :) | 12:00 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 12:02 |
| cdearborn | tobiash: got the same error after adding 'depends-on': https://review.opendev.org/#/c/716100/ - is it not working or did i make a mistake? | 12:08 |
| tobiash | cdearborn: that was a shot in the dark, could you try a depends-on to https://review.opendev.org/716561 ? | 12:09 |
| tobiash | that should just ignore that error | 12:09 |
| cdearborn | tobiash, sure! | 12:09 |
| tobiash | the fix might be a little bit more difficult as I though as it happens inside of ansible | 12:09 |
| *** rlandy has joined #zuul | 12:11 | |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 12:11 |
| openstackgerrit | Tobias Henkel proposed zuul/nodepool master: Add debug info on json parse error of image upload https://review.opendev.org/716566 | 12:15 |
| *** dmellado has quit IRC | 12:15 | |
| *** Goneri has joined #zuul | 12:16 | |
| *** jcapitao_lunch is now known as jcapitao | 12:16 | |
| cdearborn | tobiash: nope, that didn't do it. Now there's a 2nd error about a possible syntax error: https://review.opendev.org/#/c/716100/ | 12:23 |
| *** rfolco has joined #zuul | 12:23 | |
| *** dmellado has joined #zuul | 12:23 | |
| AJaeger | tobiash: https://zuul.opendev.org/t/openstack/build/d3eabd8f9cc94db6b80c9a3c2eb2a060 is the error | 12:23 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Ignore errors when parsing tox output https://review.opendev.org/716561 | 12:28 |
| tobiash | AJaeger: updated ^ | 12:28 |
| AJaeger | tobiash: left a question for you | 12:31 |
| *** rpittau has joined #zuul | 12:32 | |
| mnaser | morning | 12:33 |
| mnaser | okay, looks like i'm here just in time.. | 12:33 |
| AJaeger | morning, mnaser ! Yeah, a bit of fun in scrollback ;9 | 12:33 |
| mnaser | k | 12:35 |
| * mnaser looks | 12:35 | |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Ignore errors when parsing tox output https://review.opendev.org/716561 | 12:35 |
| tobiash | AJaeger: ^ | 12:35 |
| mnaser | ok i think i have a solution | 12:37 |
| mnaser | do we maybe wanna give it a try or lets merge this for now? | 12:37 |
| tobiash | mnaser: I think we should do both | 12:38 |
| tobiash | ignore errors and fix the error :) | 12:38 |
| tobiash | we'll still see the error anyway so ignoring them won't hinder us from fixing the underlying issue | 12:39 |
| mnaser | tobiash: agreed | 12:40 |
| mnaser | tobiash: also it's not as straight forward as it seems to be in the ansible layer | 12:40 |
| tobiash | mnaser: yes, I noticed that too ;) | 12:40 |
| mnaser | AJaeger, tobiash: +3 for now.. | 12:42 |
| tobiash | \o/ | 12:42 |
| AJaeger | mnaser: want to dig into https://review.opendev.org/#/c/716334/ or https://review.opendev.org/716377 - we should get rid of these warnings | 12:44 |
| AJaeger | python-dracclient is passing the py27 jbos with depends-on now! | 12:45 |
| AJaeger | cdearborn, tobiash, mnaser , looks like we're green! ^ | 12:45 |
| tobiash | :) | 12:46 |
| *** jpena|lunch is now known as jpena | 12:46 | |
| mnaser | tobiash: i might have a really silly workaround | 12:46 |
| mnaser | tobiash: this whole thing happens because ansible tries to log things into syslog it's actions and that big blob of string data probably makes it unhappy as it tries to encode it and log to syslog | 12:46 |
| openstackgerrit | Sorin Sbarnea proposed zuul/zuul-jobs master: Add support for RedHat platforms on install-podman https://review.opendev.org/716578 | 12:47 |
| mnaser | tobiash: if we set that task to 'no_log' .. we avoid that... https://github.com/ansible/ansible/blob/stable-2.9/lib/ansible/module_utils/basic.py#L1945-L1957 | 12:47 |
| tobiash | that is a really ugly workaround and makes it impossible to debug that script | 12:47 |
| mnaser | ah, no_log doesn't log errors anymore too in the console? | 12:48 |
| mnaser | by errors i mean tracebacks | 12:48 |
| tobiash | no_log will cause nothing to log at all not even errors | 12:49 |
| tobiash | that's intended to protect secrets normallyu | 12:49 |
| mnaser | tobiash: wait, what if we marked the argument in the module as a secret, it looks like this fucntion loops over all the params and skips the no logged one? | 12:49 |
| mnaser | that will keep it still running but it will not log the whole wall of text into syslog | 12:49 |
| tobiash | is that possible? | 12:50 |
| mnaser | the code that i linked makes it seem like it's looping over values | 12:50 |
| mnaser | oh yes it is | 12:50 |
| mnaser | see this test https://github.com/ansible/ansible/blob/308723c3ca1122363e419070f1fa1d76ff5611a9/test/units/module_utils/common/parameters/test_list_no_log_values.py#L18 | 12:50 |
| tobiash | cool, I don't think we'll want to log the stdout of the tox output again anyway | 12:51 |
| mnaser | https://github.com/ansible/ansible/blob/308723c3ca1122363e419070f1fa1d76ff5611a9/test/units/module_utils/basic/test__log_invocation.py also is specific to our failures | 12:51 |
| mnaser | yeah exactly | 12:51 |
| mnaser | okay let me try | 12:51 |
| zbr | older improvement on ensure-tox: https://review.opendev.org/#/c/690057/ | 12:53 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: tox_parse_output: add no_log to tox_output https://review.opendev.org/716579 | 12:54 |
| mnaser | tobiash, AJaeger ^ something like this | 12:55 |
| tobiash | if that works that would be a damn simple fix :) | 12:55 |
| openstackgerrit | Merged zuul/zuul-jobs master: Ignore errors when parsing tox output https://review.opendev.org/716561 | 12:55 |
| mnaser | cdearborn: would it be cool to try your patch again with that briefly | 12:55 |
| mnaser | with a "Depends-On: | 12:55 |
| mnaser | Depends-On: https://review.opendev.org/716579 | 12:56 |
| mnaser | (it could even be another unrelated patch) | 12:56 |
| zbr | mnaser: i think i missed the issue with tox output, what happened? the idea of hiding output worries me from UX point of view. | 12:56 |
| AJaeger | mnaser: we merged 716561 too fast, so you want to merge with 716561 reverted | 12:56 |
| mnaser | AJaeger: i think we would still catch the failure | 12:56 |
| AJaeger | zbr: check 716579, it's not tox_output, it's tox_outpu as input to a parser ;) | 12:56 |
| mnaser | zbr: we're not hiding tox output, we're hiding tox output from being registered as a param for the module that scans the output for inline comments | 12:56 |
| mnaser | AJaeger: going back to your point, i think the job will still pass, but we'll just have to review logs by hand after to make sure it didn't raise any exceptions | 12:57 |
| zbr | mnaser: huh, happy to hear that. | 12:57 |
| AJaeger | mnaser: you can create your own change for dracclient on top of cdearborn's with depends-on | 13:01 |
| *** hashar has joined #zuul | 13:05 | |
| cdearborn | mnaser, sure thing - sorry had to step away for a bit | 13:05 |
| *** bhavikdbavishi has joined #zuul | 13:09 | |
| mnaser | cdearborn: cool, no worries, ill wait for you | 13:14 |
| mnaser | AJaeger: juggling through a million things this morning :p | 13:14 |
| AJaeger | mnaser: no worries | 13:17 |
| zbr | i want to prose a change on how tox role runs: when it gets a (command separated) list of envirnments to split it and to run tox each time with a single environment | 13:18 |
| openstackgerrit | Merged zuul/zuul-jobs master: tox_parse_output: add no_log to tox_output https://review.opendev.org/716579 | 13:18 |
| zbr | this will improve UX because user may be able to see the error on exactly the failed environment instead of having the error "lost" from that default tail. | 13:19 |
| zbr | if this sounds ok, i can propose a PR for implementing it. | 13:19 |
| cdearborn | mnaser, AJaeger, tobiash: All green! https://review.opendev.org/#/c/716100/ | 13:19 |
| *** dangtrinhnt_ has quit IRC | 13:20 | |
| tobiash | yes, that seems to have fixed it | 13:21 |
| mnaser | yay! | 13:21 |
| mnaser | i'll use that as a "learning experience" for the golangci-lint one im doing :p | 13:21 |
| *** dangtrinhnt has joined #zuul | 13:21 | |
| cdearborn | mnaser, AJaeger, tobiash: thank you all so much for all your help and for so quickly fixing this. I really appreciate it!!! | 13:22 |
| *** dangtrinhnt has quit IRC | 13:23 | |
| *** dangtrinhnt_ has joined #zuul | 13:23 | |
| *** dangtrinhnt_ has quit IRC | 13:24 | |
| AJaeger | cdearborn: sorry for breaking your change - and thanks for the help getting it fixed! | 13:26 |
| cdearborn | Ajaeger: no problem - glad to help | 13:27 |
| mordred | mnaser: one more followup - we might want to strip ansi chars from the message in the pep8 | 13:52 |
| *** smcginnis has joined #zuul | 13:52 | |
| mordred | mnaser: see https://review.opendev.org/#/c/697636/10/cinder/tests/unit/volume/drivers/vmware/test_fcd.py@567 as an example :) | 13:52 |
| * mordred goes to look | 13:52 | |
| AJaeger | yeah, https://review.opendev.org/#/c/697636/10 has 432 comments ;( | 13:52 |
| AJaeger | smcginnis: ^ | 13:52 |
| AJaeger | mnaser, tobiash, clarkb, mordred : Did either of you volunteer to send an announcement about this change? | 13:53 |
| AJaeger | mordred: but those are wrong errors, it should never have left those 432 at all! | 13:54 |
| AJaeger | mordred: oh, they come from pylint... | 13:54 |
| *** bhavikdbavishi has quit IRC | 13:55 | |
| AJaeger | tobiash, mnaser, what about prefixing the comments with the job,, so ssay "openstack-tox-pylint : E1120: No value for argument 'get_disk_type' in method call ([no-value-for-parameter) | 13:56 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Strip ansi codes from pep8 message https://review.opendev.org/716598 | 13:56 |
| AJaeger | mordred: you speak about pep8 messages - but it could be pylint or sphinx as well ;) | 13:57 |
| smcginnis | Is there a reason we have this now? We have plenty of pylint errors. We don't want them pointed out on every patch. | 13:57 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 13:57 |
| avass | ^^^ anyone want to take a look at that | 13:57 |
| avass | actually I should add a test for the environment variables, one sec | 13:58 |
| mordred | smcginnis: this is mostly intended as an improvement for the pep8 job - I don't think we considered people running non-voting linters that they didn't care about ... let's see what we can do about that | 13:58 |
| AJaeger | smcginnis: https://review.opendev.org/#/c/715727/ is the change - and we really had pep8 only in mind | 13:59 |
| AJaeger | mordred: tobiash wants to extend to sphinx as well, see https://review.opendev.org/#/c/716264/1 | 13:59 |
| tobiash | mordred: we could check the voting flag and skip if non-voting? | 14:00 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Strip ansi codes from pep8 message https://review.opendev.org/716598 | 14:01 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Add flag for toggling inline comments for linters https://review.opendev.org/716599 | 14:01 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 14:01 |
| mordred | AJaeger: cool | 14:01 |
| avass | that should do it | 14:01 |
| mordred | smcginnis: I mean - also - maybe stop running that pylint job since it's clearly a waste of time and energy? if there are 432 errors and the job is nonvoting, I'm pretty sure nobody ever looks at it and it's just burning vms | 14:02 |
| mordred | smcginnis: (we'll clearly fix this - but it's a good indication to me additionally that that job is a waste of time) | 14:02 |
| mordred | tobiash: there's a patch adding a flag to toggle if off | 14:02 |
| smcginnis | mordred: We want to find new ones that are introduced when files are touched, but pylint has so many false positives that it's useless to gate on it. | 14:02 |
| smcginnis | So it's a balance of trying to get the benefit out of it, versus limiting noise. | 14:03 |
| smcginnis | And this just brought a whole lotta noise. | 14:03 |
| mordred | smcginnis: I've been arguing that pylint is useless for like 8 years now - but shrug | 14:03 |
| mordred | totally - we'll fix that part - and sorry about that | 14:03 |
| tobiash | mordred: should we change that flag to opt-in rather than opt out? | 14:05 |
| smcginnis | If we just need to set a flag in the cinder .zuul.yaml, I think that's a fair approach. | 14:05 |
| mordred | smcginnis: remote: https://review.opendev.org/716600 Turn off inline comments on pylint | 14:05 |
| smcginnis | ++ | 14:06 |
| mordred | tobiash: no - I mean, I think it's a good feature that should normally work for people - the normal case is to run jobs you care about and the info from them should be valualbe, so I think the "we run this, normally mostly ignore it, but it's sometimes valuable for reasons" is the exceptional case | 14:06 |
| tobiash | ++ | 14:07 |
| mordred | smcginnis: my hunch is that it's noisy for most people running that job - so let's do it globally instead of just in cinder ... people can always opt-in on a pylint job if they want to in their repo - but I'm betting _most_ people runnig pylint are not gatig on it and are probably in a similar situation | 14:07 |
| smcginnis | Yeah, that makes sense. | 14:08 |
| smcginnis | And I would assume locally setting that to true would override that? Then if a project does really care, they can flip it on for their repos. | 14:08 |
| tristanC | could the inline comments be restricted to voting job that failed? | 14:10 |
| mordred | smcginnis: yes, that's right | 14:10 |
| AJaeger | mordred: and can we add a note on which job reports? So, you want to know whether it's sphinx, pep8, pylint or whatever reports... | 14:11 |
| mordred | AJaeger: yeah - I think that's a good idea | 14:11 |
| mordred | tristanC: I think we could - but let's wait on doing that for a bit and operate under that assumption that people are running jobs whose output they care about - and disable things when that isn't the case, rather than starting from an assumption that jobs are noise and signal is the exception | 14:12 |
| mnaser | i agree with mordred -- i think running linters that are non-voting is the exception to teh rule imho | 14:14 |
| mnaser | also as an infra donor, it's a little bit of a bummer to see resources go to waste like that running a non-voting job that no one cares to fix | 14:14 |
| AJaeger | mnaser: could you +2A https://review.opendev.org/#/c/716600/ , please? | 14:14 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Add tox_envlist to the inline comment https://review.opendev.org/716601 | 14:16 |
| mordred | AJaeger, mnaser : ^^ | 14:16 |
| mordred | what do you think about that? | 14:17 |
| AJaeger | mordred: can we test that? | 14:18 |
| mordred | AJaeger: I think that's a great idea :) | 14:19 |
| avass | what python version are we using in zuul-jobs? | 14:19 |
| mordred | mnaser, corvus : https://review.opendev.org/#/c/715157/ this is a recent openstack-tox-pep8 failure - note that it says something about comments being left for invalid file | 14:19 |
| mnaser | i really signed myself up for a can of worms :) | 14:20 |
| mordred | I think it's ok - the change produced a change in arguments for a function which caused an error in a file that isn't in the change | 14:20 |
| mnaser | it'll be super awesome once this all is done | 14:20 |
| mordred | so - I think it's fine and is just life :) | 14:21 |
| avass | mordred: was going to comment that I very much prefer f-strings but had to check and they weren't introduced until python3.6 :( | 14:22 |
| mnaser | AJaeger: +3'd | 14:22 |
| openstackgerrit | Merged zuul/zuul-jobs master: Add flag for toggling inline comments for linters https://review.opendev.org/716599 | 14:23 |
| AJaeger | thanks, mnaser | 14:23 |
| mordred | avass: yeah - tell me about it - I also very much prefer f-strings but 3.5 still abounds | 14:23 |
| *** avass has quit IRC | 14:27 | |
| *** avass has joined #zuul | 14:27 | |
| mnaser | today on weird things | 14:27 |
| mnaser | the work i was doing on golangci-lint was working great | 14:27 |
| mordred | AJaeger: remote: https://review.opendev.org/716604 DNM Testing zuul job change | 14:28 |
| mnaser | until i moved the install-go to pre.yaml and it's the golangci-lint has been oom-ing since... | 14:28 |
| mordred | mnaser: o_O | 14:28 |
| mnaser | i mean, it could be a concidence | 14:28 |
| mnaser | https://review.opendev.org/#/c/716464/3 is what im testing it against | 14:28 |
| mordred | mnaser: it certainly don't seem like a thing that should oom because of that | 14:28 |
| AJaeger | mordred: that change hasa broken .zuul.yaml ;( | 14:28 |
| mordred | honestlky, it doesn't seem like a thing that should oom | 14:28 |
| mordred | AJaeger: fixed (too much delete) | 14:29 |
| mnaser | mordred: i could swear the only change i did that made the ooms start happening was move to pre.yaml | 14:29 |
| mnaser | oh and btw, our install-go role is broken :) | 14:29 |
| avass | mnaser: it's broken? | 14:29 |
| mnaser | it returns meta: end_host if it detects go is installed already | 14:29 |
| mnaser | so if go is already installed and you run it twice | 14:29 |
| avass | ah yeah, that was a mistake I made | 14:30 |
| mnaser | it happily exits the whole playbook | 14:30 |
| mnaser | no worries : | 14:30 |
| mnaser | i would say more a "bug" than "broken" :) | 14:30 |
| avass | I found it but haven't taken the time to fix it yet | 14:30 |
| mnaser | i will try to find time to figure out some clean alternative way of doing it | 14:30 |
| avass | give me a second and I'll do it | 14:30 |
| mordred | mnaser: we shold makea. go-stow element and add support for consuming that to | 14:31 |
| mordred | too | 14:31 |
| mordred | :) | 14:31 |
| mnaser | mordred: true, i think we're waiting for a dib release to hack on that, too many things at once | 14:31 |
| mordred | yah | 14:31 |
| mordred | mnaser: btw - dib was released with python-stow | 14:31 |
| avass | just got off work anyway, and since I'm working from home I can do it while watching TV anyway :) | 14:31 |
| mnaser | ooooo | 14:31 |
| mnaser | mordred: step #2 is getting a nodepool image with a newer dib | 14:32 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 14:33 |
| avass | mnaser: how about this? | 14:34 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Do not end host if correct go version is installed https://review.opendev.org/716607 | 14:34 |
| mordred | mnaser: you should have a new nodepool image with new dib: https://review.opendev.org/#/c/716104/ | 14:34 |
| mordred | avass: yes | 14:35 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Add tox_envlist to the inline comment https://review.opendev.org/716601 | 14:38 |
| mnaser | ok so golangci-lint latest seemes to have memory issues, v1.23.8 copes better | 14:38 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 14:42 |
| corvus | mordred, mnaser: was the ansi strip patch tested with a depends on? https://review.opendev.org/716598 | 14:50 |
| *** ysandeep|rover is now known as ysandeep|away | 14:50 | |
| mnaser | corvus: i believe mordred has a change in the works that they're depended-on-ing | 14:50 |
| mordred | corvus: https://review.opendev.org/#/c/716604/ should test it | 14:50 |
| mnaser | which also makes me feel we really should start building unit tests to this thing | 14:50 |
| corvus | mnaser: ++ | 14:50 |
| mnaser | as it looks like it's picked up usage | 14:50 |
| mordred | corvus: (the pylint output has the ansi) | 14:51 |
| mnaser | and it has a huge impact if we do something wrong | 14:51 |
| corvus | mordred: what about the envlist change, is that also tested? | 14:51 |
| mnaser | i'm a little deep in other things right now so i cant really write the tests :< | 14:51 |
| AJaeger | we could also change the logic: Enable it for now only on the pep8 jobs. | 14:51 |
| mordred | corvus: actually - both are tested with the same depends-on | 14:51 |
| AJaeger | (instead of disabling pylint) | 14:51 |
| corvus | k | 14:51 |
| mnaser | i dont like disabling it for a bunch of stuff | 14:54 |
| *** hashar has quit IRC | 14:55 | |
| AJaeger | mnaser: where do we want it? On py37? On pylint? On py36? There're many tox based jobs... | 14:55 |
| AJaeger | mnaser: I'm thinking of enabling it on a few until we're sure that it works fine and then enhance.. Maybe we don't need it | 14:56 |
| mnaser | AJaeger: well, we can just have our regex matchers that ignore it | 14:56 |
| mnaser | also | 14:57 |
| AJaeger | mordred: https://review.opendev.org/#/c/716604/ looks good - but since pylint is passing, we don't see the ansi code working | 14:57 |
| mordred | AJaeger, corvus : https://review.opendev.org/#/c/716604/2/cinder/context.py@20 | 14:57 |
| mordred | yeah - I wasn't expecting pylint to pass since it failed SO BADLY on the other patch :) | 14:57 |
| AJaeger | mordred: so, line 20 looks great! | 14:58 |
| mordred | the ansi stripping at least didn't break the normal outpuot | 14:58 |
| mordred | oh - they only run pylint on files that changed in the change | 14:58 |
| * mordred makes another version of that patch real quick | 14:59 | |
| AJaeger | mordred: will the changes fix also "Job openstack-tox-cover: unable to map line for file comments: | 15:00 |
| AJaeger | stderr: 'fatal: There is no path stdin in the commit' | 15:00 |
| AJaeger | See https://review.opendev.org/#/c/716573/ | 15:00 |
| corvus | mordred, mnaser: would it be possible for you to collect samples of all the failed lines as you do this? | 15:01 |
| corvus | just stick them in a file or a comment in the role for now | 15:01 |
| corvus | mordred, mnaser: but that will help us build up a library of content for the future unit test for this | 15:01 |
| corvus | if we lose it now, we'll never find out what sort of things broke it in the past, and if we refactor it, we'll discover this all again | 15:02 |
| mordred | AJaeger: I resubmitted the cinder DNM with a change that shoudl trigger a pylint error too | 15:02 |
| AJaeger | mordred: and did you enable the pyling reporting? | 15:02 |
| mordred | AJaeger: yes | 15:02 |
| AJaeger | great! | 15:03 |
| AJaeger | bbl | 15:04 |
| mnaser | corvus: i have actually been doing my failure collection inside the review itself that merged + been using teh same topic for a collection of DNM tests across projects so we can look there | 15:04 |
| mnaser | so at least we can go back and look at those reviews and see what warnings they have raised | 15:04 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Add tox_envlist to the inline comment https://review.opendev.org/716601 | 15:05 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: WIP Add testing for inline tox comments https://review.opendev.org/716623 | 15:05 |
| mordred | mnaser: ^^ there's a change that grabbed soe of the cinder text | 15:05 |
| corvus | mnaser: i'm worried if we don't collect them in a file, we'll lose that. as we've seen, it's really hard to go back and pick up the context from 6 month old changes with expired logs :/ | 15:09 |
| *** jcapitao is now known as jcapitao_afk | 15:09 | |
| corvus | mordred: that's perfect, thanks! | 15:09 |
| mnaser | corvus: fair, is an etherpad cool? | 15:10 |
| corvus | mnaser: i think mordred's file in https://review.opendev.org/716623 is the place | 15:10 |
| mordred | mnaser: point me at some things and I can go grab it | 15:10 |
| mordred | mnaser: https://review.opendev.org/#/q/topic:pep8-inline-comments yeah? | 15:11 |
| mnaser | mordred: https://review.opendev.org/#/q/topic:pep8-inline-comments | 15:11 |
| mnaser | yeah | 15:11 |
| mnaser | mordred: https://review.opendev.org/#/c/610744/ was the biggest thing i was usnig to try mostly | 15:13 |
| mnaser | so you'll catch some warnings there | 15:13 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: WIP Add testing for inline tox comments https://review.opendev.org/716623 | 15:16 |
| avass | corvus: regarding adding callbacks to the executor, how do we want to enable that? Whitelist any callbacks found in the callback directory or configure it in zuul.conf? | 15:16 |
| corvus | avass: oh that's a good question. i was assuming zuul.conf, but that first idea is interesting. though, are there a bunch of built-in callbacks we'd get if we did that? | 15:21 |
| corvus | mordred, tobiash: ^ | 15:21 |
| mordred | WELL | 15:21 |
| *** hashar has joined #zuul | 15:21 | |
| mordred | I think we might want to configure this - because the move is towards callback plugins being provided in collections and they'll have namespaces (like ansible.posix.debug instead of debug) - so just scanning a dir isn't likely to be the most forward-compatible thing | 15:22 |
| corvus | oh good point | 15:23 |
| *** zxiiro has joined #zuul | 15:23 | |
| avass | ah | 15:23 |
| avass | it's something we're going to want pretty soon so I'm looking into how we can do it at the moment | 15:26 |
| openstackgerrit | Sorin Sbarnea proposed zuul/zuul-jobs master: Add support for RedHat platforms on install-podman https://review.opendev.org/716578 | 15:26 |
| tobiash | corvus: is that whitelisting or enabling? | 15:26 |
| *** jcapitao_afk is now known as jcapitao | 15:29 | |
| corvus | tobiash: my memory may be fuzzy, but i thought they were the same thing? that any whitelisted callback plugin gets called... | 15:30 |
| avass | tobiash, corvus: mostly whitelisting, configuration for the callbacks can be done with environment variables | 15:30 |
| avass | but I guess having the configuration for the callbacks themselves in a file somewhere would be good too | 15:30 |
| zbr | haha, bit funny to see zuul as reviewer, clearly easier to read. also it would work fine with ansible-lint too as it supports "parsable mode" (pep8 standard format) | 15:31 |
| zbr | i wonder if this would not put so much load on gerrit that it would collapse, as I suspect a *big* number of comments from zuul. | 15:32 |
| mordred | corvus, mnaser, clarkb, AJaeger : https://etherpad.openstack.org/p/N6V29rv4CA <-- how's that look for an email to openstack-discuss@ | 15:33 |
| avass | tobiash, corvus: I think whitelisting is the only blocker for callbacks at the moment | 15:34 |
| mordred | avass: most can also be configured via ansible.cfg ... so maybe $something that causes $something to wind up in ansible.cfg would be easiest? (given how ansible is run via bubblewrap via zuul-executor so plumbing the env vars all the way through might get mind-bendy) | 15:34 |
| avass | mordred: right... I mixed up the zuul-executor with ansible itself | 15:36 |
| mordred | avass: close enough ;) | 15:36 |
| avass | so since I heard you guys like toml, it would be perfect to nest an [extra_ansible_cfg] under [executor] ;) | 15:39 |
| mnaser | mordred: +1 -- ilike that, maybe point to teh reviews where we disabled the pylint output in case someone wants to enable it? | 15:39 |
| mnaser | btw, while i am in timeout for losing all pipelines for the vexxhost tenant | 15:40 |
| mnaser | how do we feel about renaming a role? something like the install-go one to ensure-go | 15:40 |
| mnaser | i think we can maybe use a symbolic link that will make it still continue to work on both sides? | 15:41 |
| clarkb | mordred: email seems ok. Idea I just had: could use a project level var to toggle inline comments on and off | 15:41 |
| clarkb | then people can opt into it at a project level without changing jobs | 15:41 |
| avass | mnaser: I was thinking about that earlier | 15:41 |
| *** bhavikdbavishi has joined #zuul | 15:42 | |
| avass | I've been wondering if there's any difference between the install-* vs ensure-* roles | 15:42 |
| avass | or if everything should be ensure-* for consistency | 15:43 |
| mnaser | avass: i believe historically our consistency story has been ensure-* | 15:44 |
| mnaser | at least, that's the pattern i've mostly followed, ensure-$foo and $foo as roles | 15:44 |
| avass | maybe copy the install-$foo roles to ensure-$foo and deprecate the install-$foo roles? | 15:46 |
| mnaser | i guess we could move it to ensure-*, keep a symlink and then announce the change in zuul-announce for 14 days | 15:46 |
| avass | haha | 15:46 |
| mordred | mnaser, clarkb : updated - how's that? | 15:46 |
| mnaser | well i think we just agreed to agree :) | 15:46 |
| corvus | mnaser, avass: ++ move for consistency | 15:46 |
| mnaser | mordred: +2 from me :) | 15:47 |
| clarkb | mordred: lgtm, covers the toggle idea | 15:48 |
| mnaser | this is a big leap but we should probably have some sort of 'zuul blog' so we can announce things like this and tweet them out to get excitement about them | 15:48 |
| corvus | mordred: looks good, bet we should probably do that in #opendev and send to service-discuss in the future? :) | 15:48 |
| mnaser | b/c this is _pretty_ cool | 15:48 |
| mordred | corvus: yeah - probably so :) | 15:48 |
| mordred | mnaser: and yeah | 15:48 |
| mordred | ok. bombs away for now | 15:49 |
| corvus | mnaser, mordred: i think the cms work is near completion, but stalled? we could do a simple static blog on the website | 15:50 |
| mnaser | mordred: i saw you had soem work done at some point about gatsby-ing the website | 15:50 |
| mordred | yeah - that's on my TDL | 15:50 |
| mordred | I should really get around to finishing that | 15:50 |
| corvus | oh yeah, we can still do the gatsby part before the full gerrit integration, right? | 15:50 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 15:50 |
| corvus | so we'd still have a static website, just generated by gatsby? | 15:50 |
| mnaser | if there's not much work left... i can have someone who's working on gatsby-ing our website wrap it up | 15:50 |
| corvus | i'm using too much shorthand.... | 15:51 |
| mnaser | and then i can use the roles in zuul-jobs to build and publish a site for our use too (as we're going to use gatsby to publish our site) | 15:51 |
| corvus | if i understand correctly, the long term plan is to convert the zuul website to a statically generated site using gatsby. then, later, to integrate that into netlify-cms with the gerrit plugin that lets the cms web app propose changes to gerrit. | 15:52 |
| corvus | but that first step is all we need right now in order to have a sensible static-generated blog | 15:52 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 15:52 |
| avass | hmm, I guess the documentation needs to be updated as well | 15:53 |
| mordred | corvus: yes. | 15:53 |
| mordred | mnaser: what I've got is pretty stale - but I'll see if I can't do another pass at it this week - then it might be reasonable to have your human do a pass and fix whatever I did poorly | 15:54 |
| AJaeger | mordred: just saw the cinder results - great! | 16:01 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: WIP: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 16:01 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: WIP: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 16:03 |
| avass | ^ something like that | 16:03 |
| avass | I guess the tests need to be updated as well | 16:03 |
| AJaeger | mordred: etherpad with mail draft LGTM | 16:03 |
| corvus | mordred: you cleverly made https://review.opendev.org/716601 self-testing :) | 16:04 |
| AJaeger | Lol! | 16:05 |
| openstackgerrit | Merged zuul/zuul-website master: Add redirect for /docs/zuul/user/ https://review.opendev.org/709195 | 16:07 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: WIP: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 16:14 |
| mordred | corvus: haha | 16:15 |
| *** rpittau is now known as rpittau|afk | 16:15 | |
| mordred | corvus: https://review.opendev.org/#/c/716604/ <-- that shows ansi stripping and envlist prepending | 16:15 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Add tox_envlist to the inline comment https://review.opendev.org/716601 | 16:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: WIP: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 16:17 |
| avass | I have a feeling this would have been easier to do in multiple changes | 16:17 |
| clarkb | avass: ya it might be easier to do a role at a time. Also you can have the odl role include the new role | 16:18 |
| clarkb | (to avoid needing to copy code between them | 16:19 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: WIP: Rename install- roles to ensure- for consistency https://review.opendev.org/716636 | 16:19 |
| avass | clarkb: ah I could do that instead | 16:22 |
| AJaeger | corvus: want to change your WIP to +A on https://review.opendev.org/#/c/716598/2 ? | 16:24 |
| corvus | AJaeger: yep, i went ahead and approved the followup too since it's just a whitespace change but has been shown to work | 16:25 |
| AJaeger | thanks! | 16:25 |
| AJaeger | mordred, corvus, tobiash needs to rebase https://review.opendev.org/#/c/716263/1 now - what do you think of that one and the followup? | 16:26 |
| corvus | i think mnaser's suggestion makes sense, i think it might improve long-term maintainability | 16:30 |
| tobiash | I agree | 16:30 |
| clarkb | cmurphy pointed out in #openstack-infra that unittest jobs are generating output that ends up in zuul comments too. I think mordred is investigating but thought I'd mention it here | 16:31 |
| clarkb | https://review.opendev.org/#/c/716334/ is an example change | 16:31 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: WIP Add testing for inline tox comments https://review.opendev.org/716623 | 16:32 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Require a / in a file path https://review.opendev.org/716655 | 16:32 |
| mordred | clarkb: ^^ there's a fix I think for the cmurphy report - and also I added those lines to the patch where we're collecting output samples for future tests | 16:32 |
| tobiash | it's matching "stdin:27:1: K333 'import oslo_i18n.log' must be used instead of 'import oslo.i18n.log'." | 16:32 |
| mordred | yeah | 16:33 |
| mordred | so - the patch above just adds a / to the regex | 16:33 |
| mordred | but maybe that's too simplistic and we shoudl do an os.path.exists instead? | 16:33 |
| clarkb | mordred: that will prevent doing top level .py files | 16:33 |
| mordred | clarkb: will theuy not show up at ./foo.py ? | 16:33 |
| clarkb | mordred: oh maybe | 16:33 |
| clarkb | setup.py is probably the most important one, we could whitelist it if necessary | 16:34 |
| tobiash | mordred: is 716623 just collecting examples for a yet to create test case or did I miss the actual test? | 16:34 |
| corvus | tobiash: it's the first thing, since everyone is too busy putting out fires to write the test right now :| | 16:35 |
| tobiash | os.path.exists can be difficult because we might not necessarily know the working dir | 16:35 |
| tobiash | ah ok | 16:35 |
| mordred | I think I've got path.exists ... | 16:36 |
| *** evrardjp has quit IRC | 16:36 | |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Use os.path.exists https://review.opendev.org/716657 | 16:36 |
| *** bhavikdbavishi has quit IRC | 16:36 | |
| *** evrardjp has joined #zuul | 16:36 | |
| mordred | tobiash: like that ^^? (if that looks ok. I'll squash it on the regex update) | 16:37 |
| tobiash | mordred: if that works I think the regex update is not needed | 16:37 |
| openstackgerrit | Merged zuul/zuul-jobs master: Strip ansi codes from pep8 message https://review.opendev.org/716598 | 16:37 |
| * mnaser apologizes for opening the flood gates | 16:37 | |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Check that a file exists for inline comments https://review.opendev.org/716655 | 16:38 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: WIP Add testing for inline tox comments https://review.opendev.org/716623 | 16:38 |
| mordred | tobiash: let me send up a keystoneauth dn | 16:38 |
| mordred | dnm | 16:38 |
| tobiash | mordred: does chdir not work with a module? | 16:38 |
| tobiash | that would simplify the path.exists check (and not break absolute paths) | 16:38 |
| mordred | remote: https://review.opendev.org/716659 DNM testing inline comment fix | 16:39 |
| mordred | tobiash: oh - it should work | 16:39 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Check that a file exists for inline comments https://review.opendev.org/716655 | 16:40 |
| mordred | ok. I rechecked the keystoneauth patch - hopefully that shows this working | 16:41 |
| tobiash | mordred: shall we use isfile instead of exists? | 16:42 |
| tobiash | that would also filter out comments that point to a device file | 16:43 |
| mordred | tobiash: it's probably more correct | 16:43 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Check that a file exists for inline comments https://review.opendev.org/716655 | 16:43 |
| tobiash | lgtm | 16:44 |
| openstackgerrit | Merged zuul/zuul-jobs master: Add tox_envlist to the inline comment https://review.opendev.org/716601 | 16:44 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 16:45 |
| mnaser | corvus: i have been thinking it would be pretty neat for us to add aarch64 tests to our 'all-platforms' | 16:46 |
| mnaser | it'd also make for a (kinda) trivial work (i think?) and also a really cool sort of announcement that zuul has native multiarch support and testing in it's roles | 16:47 |
| mnaser | (i'm working on some testing for the install-go -- soon to be called ensure-go and thought that would be a nice thing to add ...) | 16:48 |
| fungi | we do still have limited capacity, delays, and frequent node failures for those | 16:49 |
| fungi | i gather it's been improving, but not sure what our tolerance level is there | 16:50 |
| clarkb | the big recent change was going to ipv6 only on the test nodes in the new arm64 cloud | 16:53 |
| clarkb | I think that gives us significanlty more capacity, but its still relatively small and those jobs do take longer | 16:53 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-kubernetes to ensure-kubernetes for consistency https://review.opendev.org/716667 | 16:53 |
| avass | uuh, how do you retrigger the gate? I'm confused | 16:54 |
| avass | regate? reverify? | 16:54 |
| avass | ah nevermind it was just a bit slow | 16:55 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: go-jobs: improve testing https://review.opendev.org/716668 | 16:56 |
| mnaser | fungi, clarkb: i think we can use the approach ianw used for arm64 which is run them in a seperate pipeline | 16:57 |
| clarkb | mnaser: ya that is probably best to start. But we also need to be careful we don't consume all the arm resources testing zuul roles no one is using on arm yet | 16:58 |
| clarkb | I think my preference would be to identify roles that make sense and/or have known consumers on arm | 16:58 |
| openstackgerrit | Monty Taylor proposed zuul/zuul-jobs master: Check that a file exists for inline comments https://review.opendev.org/716655 | 16:58 |
| clarkb | start there and see how that works with current capacity | 16:58 |
| mnaser | clarkb: yeah i mean mostly i'm thinking it's for when we make a role change and we test across all of our platforms which only happens if we change a specific role | 16:59 |
| mnaser | clarkb: perhaps we can add another tag called "all-archs" and we are selective with what we test in arm, because obviously, ensure-tox is always going to work regardless of the architecture for example (...ideally) | 16:59 |
| mnaser | but something like (install|ensure)-go might be a different story | 17:00 |
| mordred | yeah. actually, I feel like ensure-go might be the first thing in our bucket where arm might matter as a specific test target | 17:01 |
| mnaser | speaking of which, it may be valuable for us to parse those tags somewhere else and document which roles/jobs are being tested against what platforms to have a nice presentable thing for users in our docs | 17:01 |
| mnaser | so there's no surprises when you try $foo on a combination of $arch/$os | 17:01 |
| clarkb | yup I think we can identify the roels that make the most sense and start with them | 17:01 |
| clarkb | the docker stuff would likely be good too since kolla is one of the current consumers of arm resources | 17:02 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-kubernetes to ensure-kubernetes for consistency https://review.opendev.org/716667 | 17:06 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-openshift to ensure-openshift for consistency https://review.opendev.org/716675 | 17:06 |
| AJaeger | mordred: https://review.opendev.org/716573 is another change with lots of comments " Job openstack-tox-py36: unable to map line for file comments: " if you need more ;) | 17:07 |
| tobiash | AJaeger: that looks like the same root cause and should be fixed by 716655 | 17:09 |
| AJaeger | great | 17:10 |
| *** jpena is now known as jpena|off | 17:10 | |
| AJaeger | tobiash, mordred, do we have a test change up for that one? I can do one quickly... | 17:11 |
| AJaeger | mordred: you're faster ;) | 17:11 |
| AJaeger | I'm approving https://review.opendev.org/#/c/716655/ now - the test in https://review.opendev.org/#/c/716659/2 looks fine | 17:13 |
| mordred | AJaeger: I agree - the test change has the lines in its log and appropriately did not try to leave them as comments | 17:17 |
| tobiash | mordred: we should now test a change where we expect line comments to make sure we don't filter all comments now ;) | 17:18 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 17:19 |
| tobiash | I'll push up a zuul change to test that | 17:19 |
| mnaser | tobiash: i can un-abandon one of mine | 17:20 |
| mnaser | i have one with a few | 17:20 |
| mnaser | tobiash: say like this one? https://review.opendev.org/#/c/610744/ | 17:20 |
| tobiash | mnaser: yes, thanks | 17:21 |
| mnaser | tobiash: restored :) | 17:22 |
| mordred | tobiash: good point :) | 17:23 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul master: DNM: Test doc file comments https://review.opendev.org/716680 | 17:24 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Generalize parse tox output https://review.opendev.org/716263 | 17:29 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 17:29 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 17:31 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-kubernetes to ensure-kubernetes for consistency https://review.opendev.org/716667 | 17:31 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-openshift to ensure-openshift for consistency https://review.opendev.org/716675 | 17:31 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-podman to ensure-podman for consistency https://review.opendev.org/716682 | 17:31 |
| mordred | tobiash: (left comment, but I think it might be cleaner to rebase that on top of the stdin patch - since we're already passing in zuul_work_dir there now | 17:31 |
| tobiash | mordred: zuul_work_dir is not necessarily the same if we execute a tox in a subdir of a repo | 17:33 |
| mordred | hrm | 17:33 |
| mordred | tobiash: that's complicated then - because zuul_work_dir also might not be the same as ~/{{ project.src_dir }} | 17:34 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Do not end host if correct go version is installed https://review.opendev.org/716607 | 17:34 |
| mordred | tobiash: (I don't think we normally run linters in a different project than the triggering one of course ...) | 17:34 |
| tobiash | mordred: we do | 17:34 |
| mordred | ah - nod | 17:35 |
| tobiash | (run in a subdir) | 17:35 |
| mordred | oh - yeah - subdir makes sense | 17:35 |
| tobiash | but not necessarily pep8 | 17:35 |
| mordred | yeah | 17:35 |
| tobiash | but I agree, running in a subdir will only work then if the output of the linter uses absolute paths | 17:35 |
| tobiash | but I think that's a limitation we could accept | 17:36 |
| mordred | yah | 17:36 |
| mordred | so - yeah - maybe project.src_dir is probably the right thing to use now | 17:36 |
| mordred | the main thing that might not work would be doing sphinx in the root of a project that is in required_projects but is not the project triggering the patch | 17:36 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Adds roles to install and run hashicorp packer https://review.opendev.org/709292 | 17:36 |
| mordred | since you'd be looking for the wrong src_dir ... but maybe let's let someone complain about that use case before we try to figure out how to solve it | 17:37 |
| tobiash | mordred: yeah that's right | 17:37 |
| mordred | we could expose the strip dir as a user-settable override so that people constructing such a job could just be explicit - but I don't think we need to do that today | 17:37 |
| tobiash | but actually for my specific use case we're running doc builds (which need stripping because of sphinx) from the root | 17:38 |
| *** bhavikdbavishi has joined #zuul | 17:38 | |
| openstackgerrit | Merged zuul/zuul-jobs master: Check that a file exists for inline comments https://review.opendev.org/716655 | 17:38 |
| mordred | good - so I think it'll work for both of us with that logic | 17:38 |
| tobiash | so I think let's use the work dir first and find a generic subdir solution if needed later | 17:39 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 17:39 |
| mordred | tobiash: sounds good to me | 17:40 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-devstack to ensure-devstack for consistency https://review.opendev.org/716685 | 17:41 |
| avass | mnaser: are you renaming install-go or should I do it? | 17:41 |
| mnaser | avass: i am not renaming anything, i'll leave that for you :) | 17:41 |
| avass | alright :) | 17:42 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-javascript-packages to ensure-javascript-packages for consistency https://review.opendev.org/716687 | 17:44 |
| mnaser | zuul-maint: i have someone who is going to work on rebuilding the zuul-website using gatsby (which is really mostly using react to build sites) -- so it'll be quite similar to our dashboard -- with the same exact "theme" -- and then once they're done with that, we'll add a "blog" components so we can add markdown files to add new blog posts. i think this will be really important to have a platform | 17:45 |
| mnaser | for all teh cool new things we're doing (and helpful for the OSF to market as well) | 17:45 |
| mnaser | any strong feelings against that? :> it will continue to be all managed via git/code-review | 17:45 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Generalize parse tox output https://review.opendev.org/716263 | 17:46 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 17:46 |
| clarkb | mnaser: do we want to drastically change the theme of the existing site? | 17:47 |
| clarkb | (If I read it properly you're suggesting we use the dashboard theme on the main site) | 17:47 |
| mnaser | clarkb: i don't think they have any more time than just converting the current _exact_ site into a react based one mostly | 17:47 |
| mnaser | but we can totally do something like that if we want to use patternfly eventually into it, but the goal is to really have a 1:1 mapping so we avoid having a whole discussion aroud "what looks good" | 17:48 |
| mnaser | mordred: can we leave comments on a file that was not part of a change? the error message i just got seems to imply "no" | 17:48 |
| mnaser | as i get a warning "Comments left for invalid file builders/pod_metrics_endpoint.go" | 17:48 |
| mordred | mnaser: nope, we can't | 17:48 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-go to ensure-go for consistency https://review.opendev.org/716689 | 17:48 |
| mnaser | mordred: is that a zuul thing or? because, gerrit does support that | 17:49 |
| mnaser | see https://review.opendev.org/#/c/716464/3/builders/pod_metrics_endpoint.go which has no changes | 17:49 |
| mnaser | (this is a story of "i added linters into a repo that didnt have linters before") | 17:49 |
| mordred | mnaser: yesh - I thnk it's a zuul thing then | 17:50 |
| clarkb | maybe I underestimate things but couldn't we continue to use the existing css then just have to generate html tags properly in gatsby? | 17:50 |
| mordred | clarkb: yeah - the gatsby conversion is just about converting how the website is made | 17:50 |
| mnaser | maybe we can revisit that? i can imagine being excited to add linters to your repo only to be greeted with no much inline comments | 17:50 |
| mnaser | clarkb: yeah pretty much that's what's going to end up happening | 17:50 |
| mordred | yeah | 17:50 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-javascript-packages to ensure-javascript-packages for consistency https://review.opendev.org/716687 | 17:50 |
| mordred | clarkb: we are _not_ talking about redesigning the website | 17:51 |
| mordred | just reworking how it's built | 17:51 |
| clarkb | mnaser: ok thats not what I read above. I read that we'd be applying the zuul dashboard css to the website instead | 17:51 |
| mordred | nope | 17:51 |
| mnaser | ahhhh, what i meant by similar to our dashboard was | 17:51 |
| mordred | like dashboard in that it's react under the hood | 17:51 |
| mnaser | "we use react in our dashboard so in terms of maintainership, it's not going to be something exotic to us" | 17:51 |
| clarkb | quite similar to our dashboard -- with the same exact "theme" <- I think theme caused the confusion there | 17:52 |
| clarkb | you mean theme of development not theme of rendering | 17:52 |
| mordred | ah | 17:52 |
| mordred | I read that as "exact same theme as current website" | 17:52 |
| mnaser | yeah that was the intention, but yeah, we're all o the same page now :p | 17:52 |
| clarkb | for the content side I think most of the current pages can probably be expressed as simple markdown type documents | 17:53 |
| * mordred should move his gatsby website to opendev | 17:53 | |
| mordred | yup | 17:53 |
| clarkb | the front page is probably an exception to htat, but everything else can be markdown I think | 17:53 |
| mordred | as we want to do more complex things - gatsby has some really nice build-time features to do data-driven things via graphql | 17:53 |
| mordred | clarkb: but luckily - for thigns that need to do be straight html (or even just straight react) - it's trivial to do so | 17:54 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-nodejs to ensure-nodejs for consistency https://review.opendev.org/716692 | 17:54 |
| * mordred has been very pleased with gatsby fwiw | 17:55 | |
| *** gmann is now known as gmann_lunch | 17:56 | |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-yarn to ensure-yarn for consistency https://review.opendev.org/716693 | 17:58 |
| mnaser | mordred: i gues sthere was a reasoning behind this.. https://review.opendev.org/#/c/613161/ | 17:58 |
| mnaser | there's probably not really a trivial way of knowing if that file does exist or not | 17:59 |
| mnaser | given the reporter code runs in the scheduler which does not have a copy of the codebase | 17:59 |
| mordred | yeah. it's an interesting thing - because on the one hand it protects against just trying to leave bogus comments | 18:00 |
| mordred | OTOH - for the use case you described, it can actually be useful to leave comments for valid files | 18:00 |
| mordred | even if they aren't part of the change | 18:00 |
| mnaser | yeah.. | 18:00 |
| mnaser | im not sure how we can detect that | 18:00 |
| *** jcapitao has quit IRC | 18:00 | |
| mnaser | invalid file vs file-not-part-of-change | 18:01 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-pdk-dependencies to ensure-pdk-dependencies for consistency https://review.opendev.org/716695 | 18:01 |
| mordred | mnaser: yeah | 18:01 |
| corvus | er, we added that change because gerrit rejects comments not part of the changes? | 18:01 |
| mnaser | thats weird. i can leave a -1 with a comment on a file that was not part of a change here: https://review.opendev.org/#/c/716464/3 | 18:02 |
| mnaser | maybe gerrit's behaviour changed? but i don't think we've updated since | 18:02 |
| mnaser | i guess it might reject it if it's comments for an actual file that doesn't exist | 18:02 |
| corvus | mnaser: how did you accomplish that? | 18:03 |
| mordred | same question | 18:03 |
| clarkb | if you think about it from the perspective of a gerrit user whose pushed a change the last thing I/you/we want is a comment saying "someones code over here is broken" | 18:03 |
| clarkb | even if gerrit allows it I'm not sure it is very helpful | 18:04 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-if-python to ensure-if-python for consistency https://review.opendev.org/716698 | 18:04 |
| avass | that should be all of them, now they just need to pass their tests | 18:04 |
| corvus | clarkb: right, it's not actionable in this change. it's probably most likely to happen in a change series, due to a previous change. | 18:04 |
| mordred | I disagree | 18:04 |
| mnaser | corvus: i manually edited the url when viewing a file, and then just left a comment | 18:05 |
| mordred | mnaser's use case was "I added a linter job and it broke a bunch of files" | 18:05 |
| mordred | you'd need to fix those files in order to land the linter change | 18:05 |
| mordred | so I think it's totally actionable | 18:05 |
| mordred | and relevant to the change | 18:05 |
| mnaser | so i just went straight to https://review.opendev.org/#/c/716464/3/builders/pod_metrics_endpoint.go and left a comment | 18:05 |
| clarkb | mordred: but we don't have that context in gerrit | 18:05 |
| clarkb | (or zuul) | 18:05 |
| mordred | (same with "I chagned a compiler flag") | 18:05 |
| mordred | clarkb: what? | 18:05 |
| clarkb | mordred: all gerrit knows is you've updated the tox.ini (or equivalent) | 18:06 |
| mnaser | you do because in the comment, you can still click and it will send you to the file | 18:06 |
| clarkb | it doesn't know that that effects all the .py files | 18:06 |
| mordred | clarkb: right - but the test job output _does_ know that | 18:06 |
| mnaser | the -1 i left myself here still shows up: https://review.opendev.org/#/c/716464/3 -- pointing at the exact file/location | 18:06 |
| mordred | so if we can leave comments on files that aren't in the change, it's still potentially valuable to a reviewer | 18:06 |
| *** reiterative has quit IRC | 18:06 | |
| clarkb | mordred: yes, but if you are a drive by committer trying to land a simple bugfix the wrong thing is for the automated system to tell them they have to be am aintainer to land a bugfix | 18:06 |
| mordred | mnaser: I have reproduced your behavior | 18:06 |
| fungi | can gatsby use restructuredtext too? just wondering if there's a way to avoid using different markup languages between our website and our docs | 18:06 |
| corvus | mordred: okay, but i think it's a bit of an edge case. normally you would see this in a patch series, and you don't want to repeat the same errors on every patch in the series. you want to see the errors for this change, and you want to go back to previous changes to see the errors there. the same is true for thu zuul config errors, which is why we went to all the effort to make sure they only report on | 18:06 |
| corvus | the relevant change. | 18:06 |
| clarkb | fungi: I think it is just markdown | 18:07 |
| *** reiterative has joined #zuul | 18:07 | |
| mordred | fungi: unfortunately no - restructuredtext hasn't gotten many non-python implementations | 18:07 |
| fungi | i suppose then it's at least no worse than editing our docs in rst and our site in raw sgml | 18:07 |
| mnaser | yep... | 18:07 |
| mordred | if there was a javascript impl, gatsby would be able to support it | 18:08 |
| fungi | and if someone really cares, there are probably rst->md converters we could use to preprocess | 18:08 |
| mnaser | corvus: i don't know if i agree on this being an edge case, i guess if you never had linters and you just added them, it would be nice to see the output going in your files of what broke | 18:08 |
| mnaser | esp if we're going to say "hey it's not possible to easily lint you golang project, just add golangci-lint and see the inline comments!" .. oh wait, none of it shows up | 18:09 |
| corvus | mnaser: well, i feel pretty strongly that ci systems shouldn't be black boxes and people should be able to run linters locally. | 18:09 |
| mordred | same with compiler flags, or dependency library updates which break a java build | 18:09 |
| corvus | mnaser: but moreover, surely adding a linter to a project for the first time is an edge case? i mean, that really is not the normal course of business for a project, right? | 18:10 |
| corvus | (i'll grant it's maybe an edge case you think is worth handling) | 18:11 |
| mnaser | corvus: that's fair, i'll agree with you on that i think it's worth handling but it's not a common occurance :) | 18:11 |
| mordred | corvus: I think adding a linter maybe be a bad specific example that also implies a potentially lower value - there are all sorts of dev activities which have impacts on code bases outside of the files touched. changing build settings in a makefile, updating a compiler setting, updating a dep in a strongly-typed/compiled language with a signature changing | 18:11 |
| corvus | and mordred is brainstorming some more common cases | 18:11 |
| mnaser | btw, sidenote -- i have now golangci-lint reporting here: https://review.opendev.org/#/c/716464/4 yay! | 18:11 |
| mordred | "this patch updates to the latest hyper.rs library" - now there are 70 files that are using the wrong object signature for a method | 18:12 |
| mordred | of course, we don't support rustc yet :) | 18:12 |
| corvus | mordred, mnaser: so here's the information i have: when we had zuul reporting on all the errors, people really wanted it scoped down to just what the change is touching. maybe this is different? | 18:12 |
| mordred | yeah - maybe? | 18:13 |
| mordred | because a zuul config change isn't going to have a knock-on effect (or shouldn't) like some of those other things | 18:13 |
| mnaser | ++ | 18:13 |
| corvus | mordred: sure it does. otherwise it woudn't generate all those errors :) | 18:13 |
| mordred | also - perhaps this a thing where there are valid arguments to be made in both direction | 18:13 |
| mordred | corvus: :) | 18:13 |
| clarkb | my real concern here is how do you differentiate that case from the case of I'm a drive by committer and the CI system wants me to maintain the software in order to land a simple bug fix | 18:13 |
| clarkb | because I think that is a really bad user experience | 18:14 |
| clarkb | (and I don't think we have enough context to differentiate) | 18:14 |
| mordred | clarkb: well - that's the case no mattrer what in this case | 18:14 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 18:14 |
| mordred | those changes aren't going to alnd without those other fixes | 18:14 |
| clarkb | mordred: it depends if you do like the pylint thing | 18:14 |
| mordred | the question is do we surface them in inline comments or only in the job console log | 18:14 |
| clarkb | which is apparently common enough | 18:14 |
| mordred | clarkb: let's ignore the nonvoting pylint case | 18:14 |
| mordred | we fixed that | 18:14 |
| clarkb | mordred: sort of you didn't address the use case aiui | 18:14 |
| mordred | and assume that jobs aren't just completely bong | 18:14 |
| clarkb | you just stopped doing it entirely | 18:14 |
| mordred | yes | 18:14 |
| mordred | because it'sa. bong job | 18:15 |
| clarkb | I don't think its bong | 18:15 |
| mordred | I do | 18:15 |
| clarkb | static checking on only the files that change is a very common thing | 18:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-kubernetes to ensure-kubernetes for consistency https://review.opendev.org/716667 | 18:15 |
| mordred | it is a job producing output people don't want to see | 18:15 |
| corvus | i'm with mordred: pylint has never produced useful output :) | 18:15 |
| mordred | so - I really think that's a red herring here - sure, it may be a valid use case but it's _certainly_ an edge case if it is | 18:16 |
| mnaser | realistically, the job was going to fail anyways, so reporting it in console log vs in-line is going to yield same results (if using depends-on) | 18:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-openshift to ensure-openshift for consistency https://review.opendev.org/716675 | 18:16 |
| mordred | let's start with "I run jobs that produce output I care about and I'd like to see it in the most efficient manner possible" | 18:16 |
| mordred | which si what I think inline comments from zuul aim to help with | 18:16 |
| mordred | people are _always_ going to do other weird things, and that's also ok | 18:16 |
| clarkb | its not just pylint fwiw | 18:17 |
| mordred | I know | 18:17 |
| clarkb | airship is apparently doing similar with coverage jobs now | 18:17 |
| mordred | I'm just saying - let's see if we can solve the actual case before going off and worrying about other things other people have decided they might want to do | 18:17 |
| clarkb | and I think kata does it to walk commits in PRs and accumulate commit specific things | 18:17 |
| mordred | I don't care what kata wants to do with anything | 18:17 |
| clarkb | mordred: my point is its a general class of thing people do here | 18:18 |
| clarkb | regardless of project or tool | 18:18 |
| mordred | 'I recognize that | 18:18 |
| corvus | mnaser: can you repeat your test with gerrit master, maybe on this change: https://gerrit-review.googlesource.com/c/zuul/ops/+/261032 | 18:18 |
| mordred | clarkb: but what I'm saying is let's not hamstring the straightforward use case because there is _another_ use case where it might not make sense and where inline comments themselves might just make more sense to be disabled altogether | 18:18 |
| mordred | we don't have a story for those cases at all right now | 18:18 |
| mordred | so solving them is completely orthogonal to this | 18:19 |
| mnaser | corvus: sorry, i don't think i follow, do you mean the whole "comment on a file that doesn't exist" thing ? | 18:19 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-devstack to ensure-devstack for consistency https://review.opendev.org/716685 | 18:19 |
| clarkb | mordred: ok | 18:19 |
| corvus | mnaser: yep, i want to find out if that will still work in the future | 18:19 |
| mnaser | corvus: good idea, i also wanted to think of github too as a case as well | 18:19 |
| clarkb | mordred: taking corvus' change stack example I think I'd personally prefer errors closest to the source | 18:19 |
| clarkb | we'll end up with very noisy comments and change histories otherwise | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-javascript-packages to ensure-javascript-packages for consistency https://review.opendev.org/716687 | 18:20 |
| mordred | corvus: done | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-nodejs to ensure-nodejs for consistency https://review.opendev.org/716692 | 18:20 |
| mordred | corvus: I left a comment on nodepool.yaml | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-yarn to ensure-yarn for consistency https://review.opendev.org/716693 | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-pdk-dependencies to ensure-pdk-dependencies for consistency https://review.opendev.org/716695 | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-if-python to ensure-if-python for consistency https://review.opendev.org/716698 | 18:20 |
| mordred | clarkb: yeah - this is where I thnk it might need to be configurable | 18:20 |
| mordred | clarkb: becuase I _strongly_ disagree with your preference there :) | 18:20 |
| mordred | on a project I had control over, I would want to see all of the errors on all of the files | 18:20 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 18:21 |
| mordred | but I can understand that you might not agree | 18:21 |
| mnaser | corvus: looks like mordred did it and i also did it as well | 18:21 |
| clarkb | mordred: the reason for me preference is I don't want to mistakenly fix it in a child change, then have to fix it in the parent | 18:21 |
| clarkb | mordred: because the comments will indicate that I need to fix it in the child | 18:21 |
| clarkb | mnaser: then I've got some ugly rebasing and the chagne history gets weird | 18:21 |
| mnaser | clarkb: but in that case, the job will also indicate you need to fix it in the child? | 18:21 |
| mordred | clarkb: sure - totally understand. I just have a different preference | 18:21 |
| clarkb | the end result will still be fine, but getting there will be a mess | 18:21 |
| mordred | and I think it will be cleaner and easier to see reported everywhere, at least personally | 18:21 |
| clarkb | mnaser: yes thats the problem. It shouldn't be fixed in the child it should be fixed in the parent where the issue originates | 18:21 |
| mnaser | clarkb: assuming we can identify that everywhere, which we cannot | 18:22 |
| mordred | for me what I want is to present the information that the build has produced in a streamlined way | 18:22 |
| mnaser | so at least having it inline can be easy for someone to reply and say "i didn't touch this, i didn't break this" | 18:22 |
| corvus | yes, it's true that the log output for the job will say that the file needs to be changed. however, by adding the comment to the change, you're going to make the file appear in the gerrit UI when it did not before. that may, for some people, give them a moment of confusion since they may not understand why a file they don't remember editing is suddenly appearing in the changeset. | 18:22 |
| clarkb | mnaser: yes, I'm willing to deal with corner cases liek changing tox.ini or compiler flags directly to avoid that problem | 18:22 |
| mordred | if the build of eafch patch is showing all of the failures, then all of the failure are valid for each patch | 18:22 |
| mnaser | i think we have to remember that for the typical zuul user, they would be far more confused as to "why did i not get any comments, is something broken?" | 18:23 |
| mnaser | we can discuss this for a long time but if it's not feasible in a very simple way, i worry that we cant go far :( | 18:23 |
| mordred | this is a good point :) | 18:23 |
| mnaser | esp if that filtering happens in the zuul-scheduler layer | 18:23 |
| corvus | i think i'm sold on the idea that "a compiler flag change should show you all the compiler errors". i'm less sold on "it's intuitive for files not changed to appear in the gerrit ui list of changed files" | 18:23 |
| mnaser | i think the only way that can even be possible is if the executor did the filtering of the zuul comments | 18:24 |
| mnaser | and the scheduler passed it directly through | 18:24 |
| fungi | zuul will still report the job failure | 18:24 |
| mnaser | if that's never going to be an acceptable approach, we might have to all be sad that things would have to stay as is | 18:24 |
| corvus | i guess i'm not following the conversation; i'm not sure how we got to "implementation may not be possible" | 18:25 |
| corvus | i'm still at "is the ux desirable?" | 18:25 |
| mordred | corvus: I wonder if it might be a difference in how we use gerrit? for me, a comment about where a failure is on a line in the review is so much quicker for me to see and understand what I need to go do it's worth it to me | 18:25 |
| fungi | i imagine up-reving flake8/pycodestyle and having gerrit suddenly display hundreds of files in its browser for that change because they all have the wrong flavor of whitespace | 18:25 |
| mordred | but I can totally imagine that people with other interactive workflows might not have that experience | 18:26 |
| fungi | i'd much rather just look at the job log in that case | 18:26 |
| corvus | mordred: let's just say i accept your argument there. | 18:26 |
| corvus | mordred: can you evaluate my supposition that files not in the change appearing in the gerrit ui as if they were changed files may not be intuitive? | 18:26 |
| mordred | corvus: totaly - I can totally imagine that that could be the case for someone | 18:27 |
| mordred | this is why I think this might be one of those project-workflow differences choices areas (ignoring implementatino) ... because I could see a given zuul user breaking either way on the cost/benefit of those two things | 18:27 |
| *** gmann_lunch is now known as gmann | 18:28 | |
| *** sugaar has quit IRC | 18:28 | |
| mordred | (or really a given project manager, not really possible on a per-developer basis to have such an opinion) | 18:28 |
| corvus | mordred: the tricky thing is that i think this crosses over a bit from "preference of the project" to "preference of the individual reviewer" | 18:28 |
| corvus | like, if the project manager decides that errors on untouched files should be reported as inline comments (a sensible workflow decision), they have also decided that every reviewer will lose the ability to quickly identify what files are touched by a change | 18:29 |
| mordred | yeah, that is totally true | 18:30 |
| corvus | maybe that's an okay trade-off? maybe users will get used to it and it won't bother them? but it may be putting us in a place where gerrit does not behave in the way it "normally" behaves. | 18:30 |
| mordred | otoh - a project manager that decides to not allow the inline comments on untouched files may have reviewers/developers fix the things reported and then still get a failued on the next patch and have to go digging through logs wondering what happened | 18:31 |
| mordred | yah - it's not a straightforward call | 18:32 |
| corvus | mordred: yep. and perhaps considering that *this* is also an edge case may be useful -- again, normally you're not going to trigger a comment-storm in a typical patch. only new or updated linters/compiler settings/libraries are going to do that. usually. | 18:32 |
| mordred | yeah | 18:33 |
| mordred | at least, one hopes it's not a normal thing :) | 18:33 |
| corvus | (discounting pylint, of course) | 18:33 |
| mordred | corvus: maybe we should suggest to Alice a robot-comments feature to allow a user to set a preference of whether to display robot comments for files not in a change | 18:33 |
| clarkb | setting them apart in the files diff view might be a good idea | 18:34 |
| corvus | mordred: yes, and if disabled, it would not expand the list of files | 18:34 |
| mordred | corvus: ++ | 18:34 |
| mordred | clarkb: robot comments are a new structure in newer gerrit apart from human comments | 18:34 |
| *** hashar is now known as hasharBreak | 18:34 | |
| mordred | which is cool | 18:34 |
| clarkb | they are still inline though right? | 18:35 |
| corvus | mnaser: i'm pretty sure we'd have everything needed to set this as a job option and adjust behavior in the reporter in the scheduler. | 18:35 |
| clarkb | and files diff is one page now | 18:35 |
| clarkb | I think you want to separate the files as in commit and out of commit there | 18:35 |
| mordred | I disagree :) | 18:35 |
| mordred | I want to see it all in as few clicks as humanly possible | 18:35 |
| clarkb | one page is fine | 18:35 |
| clarkb | but do two stacks | 18:35 |
| corvus | yeah, you could separate without clicking | 18:36 |
| clarkb | no extra clicking | 18:36 |
| mordred | ah - yes! that would be great | 18:36 |
| mordred | totally agree | 18:36 |
| mordred | I think that would be a nice improvement to RC | 18:36 |
| corvus | mnaser, mordred: this is why we added the filtering: http://eavesdrop.openstack.org/irclogs/%23openstack-infra/%23openstack-infra.2018-10-18.log.html#t2018-10-18T11:12:38 | 18:37 |
| corvus | so it seems that the way that zuul is posting the comments does result in a gerrit error (notwithstanding that doing something similar through the webui does not) | 18:38 |
| mordred | corvus: fascinating | 18:38 |
| corvus | so step 1 would be to solve that api issue | 18:38 |
| mordred | yeah | 18:38 |
| mordred | corvus: turns out removing the filtering without fixing the API issue would not be an improvement | 18:38 |
| corvus | oh... it *could* be that gerrit might accept a comment on a file if the file exists in the repo in that commit? but in this case we were sending garbage filenames and that's an error? | 18:39 |
| corvus | still, we'd want to confirm the exect behavior and validate the idea | 18:39 |
| mordred | corvus: that would make sense | 18:39 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 18:40 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul master: DNM: Test doc file comments https://review.opendev.org/716680 | 18:40 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-docker to ensure-docker for consistency https://review.opendev.org/716663 | 18:42 |
| corvus | mordred, mnaser: to summarize my position: i agree that there are valid use cases for 'inline all the comments'. i'm suspicious that doing that by default would be a good UX in gerrit, but i'm not convinced it's bad and would be willing to experiment with it in opendev. i'd be okay adding it as an option (probably off by default to start?) to jobs, but we need to perform some testing against the gerrit api | 18:45 |
| corvus | to validate it first. | 18:45 |
| corvus | clarkb: ^ | 18:45 |
| clarkb | that seems reasonable | 18:46 |
| clarkb | maybe dog food in zuul for a bit before exposing it broadly | 18:46 |
| mordred | ++ | 18:46 |
| mordred | corvus: I agree with all of those words | 18:46 |
| fungi | i have at times hit new releases of style checkers which suddenly start complaining about nearly every file in your project, so i can see where that could sometimes blow up on you | 18:47 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul master: DNM: Test doc file comments https://review.opendev.org/716680 | 18:49 |
| corvus | fungi: yep. i think there's still disagreement on whether inlining all the comments in that case is "good" or "bad". i'm comfortable with differing opinions on that and having zuul support those differing opinions. there is a very nuanced distinction between showing those comments inline and the thing i'm concerned about (the list of files). | 18:49 |
| corvus | if we (optionally) support it, we'll get feedback on both parts of that (whether having the comments inline is useful or confusing, and whether having the file list expand is confusing or irrelevant) | 18:50 |
| mordred | ++ | 18:51 |
| corvus | mnaser: we also can write superuser blog posts | 18:57 |
| corvus | (i feel like that's orthogonal to "zuul-ci.org should have a blog"; i don't mean to suggest that as an alternative, just something to keep in mind as an additional channel) | 18:57 |
| mordred | ++ | 18:59 |
| *** harrymichal has quit IRC | 19:07 | |
| *** harrymichal has joined #zuul | 19:07 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 19:08 |
| mnaser | corvus: yes i agree in also "syndicating" that content as well | 19:12 |
| mnaser | corvus, mordred: i'm in agreement with the state of things right now, but yeah, we'd have to figure out the gerrit bits (i suspect it has to do with commenting on a non-existing file) and then all the code in place | 19:12 |
| mnaser | until then i am in revision six million for inline stuff for golang | 19:13 |
| mnaser | this ain't easy | 19:13 |
| mnaser | which also means i am about to give up and just write unit tests for all of our sakes | 19:13 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-kubernetes to ensure-kubernetes for consistency https://review.opendev.org/716667 | 19:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-openshift to ensure-openshift for consistency https://review.opendev.org/716675 | 19:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-podman to ensure-podman for consistency https://review.opendev.org/716682 | 19:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-devstack to ensure-devstack for consistency https://review.opendev.org/716685 | 19:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-javascript-packages to ensure-javascript-packages for consistency https://review.opendev.org/716687 | 19:15 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-nodejs to ensure-nodejs for consistency https://review.opendev.org/716692 | 19:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-yarn to ensure-yarn for consistency https://review.opendev.org/716693 | 19:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-pdk-dependencies to ensure-pdk-dependencies for consistency https://review.opendev.org/716695 | 19:16 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-if-python to ensure-if-python for consistency https://review.opendev.org/716698 | 19:16 |
| avass | I think most of it should work now | 19:17 |
| avass | not sure why this change complains about invalid configuration though: https://review.opendev.org/#/c/716675/ | 19:18 |
| AJaeger | mordred: previously, if tox fails, I could open the Console tab and it would open at "tox: Run tox". Now "Tox: Return tox status" shows failed. and "Run tox" shows changed. can we change that back? | 19:20 |
| AJaeger | Example: https://zuul.opendev.org/t/openstack/build/96de28817cd84bd5aa183015b51ca855/console | 19:20 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Support multiple matchers when parsing tox output https://review.opendev.org/716263 | 19:27 |
| *** bhavikdbavishi has quit IRC | 19:27 | |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 19:27 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 19:27 |
| mordred | AJaeger: oh hrm. that is a bother | 19:27 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-nodejs to ensure-nodejs for consistency https://review.opendev.org/716692 | 19:28 |
| mordred | AJaeger: it's related to how the tox output processing is working ... I agree, that's nonideal - we'll need to ponder on it for just a bit to find a proper solution | 19:28 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-yarn to ensure-yarn for consistency https://review.opendev.org/716693 | 19:29 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Keep error status in tox run https://review.opendev.org/716736 | 19:33 |
| tobiash | mordred, AJaeger: this might make that console issue better ^ | 19:33 |
| AJaeger | thanks! | 19:33 |
| mordred | tobiash: that is much smarter than where my brain was taking me | 19:38 |
| corvus | tobiash: +2 with comment, what do you think? | 19:39 |
| tobiash | mordred: any idea why this misses the debug task after "look for output": https://0f32d14684fb053eca08-e68f967f18214a0d1e6eb98f4cfddc91.ssl.cf5.rackcdn.com/716680/3/check/zuul-tox-docs/f708016/job-output.txt ? | 19:39 |
| tobiash | mordred: the depends on adds such a debug task directly afterwards | 19:39 |
| tobiash | mordred: yeah, I'll add a comment | 19:40 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Keep error status in tox run https://review.opendev.org/716736 | 19:42 |
| tobiash | mordred: found it: - '<Job zuul-tox-docs branches: None source: zuul/project-config/zuul.d/jobs.yaml@master#1>' | 19:45 |
| tobiash | mordred: do you know why zuul-tox-docs is defined in project-config? | 19:46 |
| tobiash | that makes that job non-speculative | 19:46 |
| tobiash | which explains why my sphinx line comment changes don't work | 19:46 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul master: DNM: Test doc file comments https://review.opendev.org/716680 | 19:48 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 19:51 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 19:51 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-pdk-dependencies to ensure-pdk-dependencies for consistency https://review.opendev.org/716695 | 19:52 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Rename install-if-python to ensure-if-python for consistency https://review.opendev.org/716698 | 19:52 |
| openstackgerrit | Merged zuul/zuul-jobs master: Keep error status in tox run https://review.opendev.org/716736 | 19:56 |
| tobiash | AJaeger: fyi ^ | 19:56 |
| *** hasharBreak is now known as hashar | 19:57 | |
| openstackgerrit | James E. Blair proposed zuul/zuul-registry master: Add debug/verification for uploads https://review.opendev.org/716444 | 19:57 |
| avass | mordred, corvus: I cannot figure out why these changes "depends on a change with invalid confiuguration": https://review.opendev.org/#/c/716675/4 https://review.opendev.org/#/c/716695/ https://review.opendev.org/#/c/716692/ | 19:58 |
| corvus | avass: hrm, i'll see if there's anything in the logs | 19:58 |
| corvus | zuul-maint: is anyone else interested in reviewing a not-too-complicated zuul-registry change? i'd like to get https://review.opendev.org/716444 merged so it's in place the next time the problem shows up | 19:59 |
| avass | corvus: yeah that error could have been a bit more specific | 19:59 |
| corvus | avass: yep, it's usually more obvious :/ | 19:59 |
| *** weshay is now known as weshay|ruck | 20:00 | |
| corvus | avass: it's a bug in zuul: AttributeError: 'NoneType' object has no attribute 'loading_errors' | 20:00 |
| corvus | avass: i think there's a patch; lemme see if i can find it and if it's landed | 20:01 |
| tobiash | corvus: that sounds familiar | 20:01 |
| corvus | https://review.opendev.org/712544 | 20:01 |
| corvus | clarkb: ^ | 20:01 |
| corvus | yep that's the right error | 20:01 |
| tobiash | corvus: shall we land that or first try to find out why the parent layout is None? | 20:02 |
| corvus | tobiash: i don't understand the fix yet, so i don't want to land it | 20:03 |
| tobiash | me too | 20:03 |
| clarkb | I thik we decided my change wasnt right in earlier conversation | 20:03 |
| clarkb | ya | 20:03 |
| corvus | avass: i'm pretty sure a recheck once the queue is clear will work | 20:03 |
| corvus | clarkb: oh, maybe we should dig up that convo and paste it into gerrit :) | 20:03 |
| clarkb | I'm hesitant too and that change was mostly to leave a breadcrumb with stab at problem | 20:03 |
| tobiash | that would help :) | 20:03 |
| avass | corvus: alright I'll wait a bit then :) | 20:04 |
| corvus | clarkb, tobiash, avass: http://eavesdrop.openstack.org/irclogs/%23zuul/%23zuul.2020-03-17.log.html#t2020-03-17T23:14:34 | 20:05 |
| AJaeger | tobiash: cool | 20:05 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul master: DNM: Test doc file comments https://review.opendev.org/716680 | 20:05 |
| corvus | there's no gate pipeline involved here, so that may reduce the search space for the problem somewhat | 20:06 |
| openstackgerrit | Albin Vass proposed zuul/zuul-jobs master: Update registry test to use ensure-podman and ensure-docker https://review.opendev.org/716752 | 20:06 |
| fungi | zuul-maint: in case it slipped past you like it did me, the osf "community meeting" conference call tomorrow is intended to feature project updates, which means zuul: http://lists.zuul-ci.org/pipermail/zuul-discuss/2020-April/001202.html | 20:07 |
| fungi | it turns out the meeting organizers are looking for ~5 minutes of updates on what's been going on with zuul in the 5 months since corvus's project update in shanghai | 20:07 |
| fungi | i started a brainstorming etherpad here and clarkb and jamesmacarthur have also suggested some content for a slide, but nothing's carved in stone: https://etherpad.openstack.org/p/zuul-2020q1-update | 20:07 |
| fungi | if anybody knows of things which should be included, or wants to volunteer to talk for 5 minutes on the call about whatever winds up in the slide, that's awesome | 20:08 |
| corvus | fungi: looks good | 20:09 |
| tobiash | corvus, fungi: shall we add github checks api support? | 20:10 |
| fungi | i guess that landed since november? | 20:11 |
| tobiash | yes, that landed recently | 20:11 |
| fungi | gerrit checks api too, right? | 20:11 |
| fungi | or i guess that's still in flux? | 20:11 |
| corvus | gerrit checks is 'experimental' | 20:12 |
| corvus | github checks is more solid | 20:12 |
| tobiash | but not yet feature complete as well | 20:12 |
| mnaser | great | 20:13 |
| mnaser | i have functioning unit tests | 20:13 |
| mnaser | but now i get to have the fun of long strings and pep8. | 20:13 |
| *** sgw has quit IRC | 20:14 | |
| *** harrymichal has quit IRC | 20:17 | |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 20:18 |
| corvus | avass: it might be worth a quick note to the zuul-discuss list to lest folks know about the install -> ensure effort to make sure we've got consensus (i'd hate to merge all those and then have folks say "maybe we should have done ... instead") | 20:18 |
| mnaser | mordred, corvus: these are unit tests for the golangci-lint job, maybe it can serve as a good example for doing the tox ones... i am at a bit of a loss of whats a clean way of keeping it under 80 characters without making the string-y bits super confusing | 20:19 |
| mnaser | tobiash: ^ | 20:19 |
| fungi | corvus: tobiash: thanks, so "support for the github checks api" is how we'd refer to it i guess? | 20:20 |
| corvus | fungi: ++ or "support for github checks" | 20:21 |
| tobiash | btw, this includes file comments :) | 20:21 |
| tobiash | which is why this recently became much more interesting for me ;) | 20:22 |
| corvus | mnaser: one crazy idea: save the strings in a $(yaml|json|csv|whatever) file and have each unit test call a function to return the correct string from the fixture file | 20:22 |
| fungi | corvus: got it, i tried to infer terminology from the connection driver docs but it uses "check" and "checks" in fairly disconnected ways | 20:22 |
| corvus | fungi: yeah, "checks" is the common name of the feature in github | 20:22 |
| fungi | trying to figure out a way to not make it sound like we only just added support for github itself, since checks is such a generic term | 20:23 |
| fungi | support for github's checks feature? | 20:23 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 20:23 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 20:23 |
| corvus | mnaser: getting the weirdly formatted text out of the functions might make them easier to read (at the risk of having to look elsewhere for the data) | 20:23 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Don't silently ignore exceptions when parsing tox output https://review.opendev.org/716766 | 20:23 |
| corvus | fungi: ++ | 20:23 |
| avass | corvus: yeah I agree, just wanted those changes to be ready | 20:24 |
| mnaser | corvus: you know i was mulling over that and now that you said it out loud, that makes me feel better about that too | 20:24 |
| corvus | mnaser: great minds | 20:24 |
| *** igordc has joined #zuul | 20:27 | |
| avass | It's getting pretty late here, I'll check in tomorrow | 20:27 |
| * tobiash totally overlooked the clock | 20:28 | |
| avass | haha, yeah same here | 20:28 |
| *** avass has quit IRC | 20:29 | |
| *** y2kenny has joined #zuul | 20:29 | |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Support multiple matchers when parsing tox output https://review.opendev.org/716263 | 20:32 |
| y2kenny | Hi, I ran into some exception about MergeJob object has no attribute updated on scheduler loadconfig: | 20:32 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Don't silently ignore exceptions when parsing tox output https://review.opendev.org/716766 | 20:32 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 20:32 |
| y2kenny | http://paste.openstack.org/show/791487/ | 20:32 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 20:32 |
| *** sgw has joined #zuul | 20:33 | |
| tobiash | y2kenny: that can occur if there is no merger in the system or the merger hits a timeout when cloning a large repo | 20:34 |
| corvus | y2kenny: that's obviously not a great error and we should improve that; but it may be caused by a problem on an executor or merger which handled the job; you might be able to find more info if you look at their logs | 20:34 |
| y2kenny | ok so that in itself shouldn't prevent the config from loading right? | 20:34 |
| y2kenny | your description sounds right because I didn't start any executor or merger for a long time | 20:35 |
| corvus | y2kenny: that's how zuul loads its config (by asking the executors/mergers to do it for it), so an error there (espcially at server start) is unrecoverable | 20:35 |
| fungi | is this related to the situation mnaser encountered where starting the scheduler before the executors/mergers causes it to not load its configuration? | 20:36 |
| y2kenny | um... so I should start an executor before scheduler | 20:36 |
| y2kenny | that explains it | 20:36 |
| tobiash | fungi: yes | 20:36 |
| corvus | y2kenny: it should be enough to start them at around the same time. but yeah, not long before, or the scheduler will timeout waiting. | 20:36 |
| fungi | and the suggestion for the kubernetes operator is to orchestrate the start order, right? | 20:37 |
| y2kenny | corvus: I am still trying to do things the hard way :). | 20:37 |
| y2kenny | I was just doing the last bit of moving the scheduler onto k8s w/o using the operator or helm | 20:37 |
| tobiash | fungi: yes, I think tristanC already has a patch up for that | 20:37 |
| fungi | oh, cool | 20:38 |
| corvus | fungi: i think the k8s operator takes the "start them all around the same time" approach; *restarting* is a slightly different question which tristanC has a patch up for | 20:38 |
| y2kenny | I got executor, nodepool, web on my k8s cluster... just missing the scheduler | 20:38 |
| fungi | ahh, i can see where restart order can be an issue | 20:38 |
| y2kenny | right now I am doing all of them in a bunch of yaml files | 20:38 |
| corvus | fungi: (specifically in the case of a restart with an updated config, the mergers/executors need to start with the new config before the scheduler does) | 20:39 |
| fungi | yep, makes sense | 20:39 |
| y2kenny | corvus: that's good to know... I was wondering what I needed to do when I change the config | 20:39 |
| corvus | y2kenny: that's the case for a change to zuul.conf, like to add a connection | 20:39 |
| corvus | y2kenny: a main.yaml (tenant config) change doesn't need any restart, just a "smart-reconfiguration". | 20:40 |
| y2kenny | so far I have been restarting pretty much everything... with docker compose | 20:40 |
| y2kenny | ah ok. That's good to know about the tenant config | 20:41 |
| *** avass has joined #zuul | 20:41 | |
| y2kenny | does this mean the docker compose for the quick start is a bit misleading? | 20:44 |
| y2kenny | the executor container has a depends_on scheduler instead of the other way around | 20:44 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: Strip source dir from file comments https://review.opendev.org/716264 | 20:45 |
| openstackgerrit | Tobias Henkel proposed zuul/zuul-jobs master: DNM: Debug sphinx message https://review.opendev.org/716722 | 20:46 |
| tobiash | corvus, mordred: first sphinx line comment: https://review.opendev.org/#/c/716680/5/doc/source/howtos/gerrit_setup.rst | 20:54 |
| tobiash | :) | 20:54 |
| corvus | tobiash: nice! | 20:55 |
| mordred | tobiash: that is awesome | 20:58 |
| corvus | mordred, mnaser: thinking further on the line comments for invalid file thing: jhesketh suggested a change to that warning message to include the comment. we could look at that as a way to get the info to the user without altering the gerrit ui. it wouldn't be inline comments, but they'd appear in the overall change message, which, with most gerrit uis displaying the inline comments with the change | 20:58 |
| corvus | message, is a very likely place for people to read them anyway). maybe that's a half-step we could take that doesn't open up as many ui questions? | 20:58 |
| *** avass has quit IRC | 20:59 | |
| mordred | corvus: seems like a decent step forward - especially since the warning message isn't as friendly to the user (it's a base job sending the data, there's nothing they can do about it) | 20:59 |
| mordred | AJaeger: check out https://review.opendev.org/#/c/716680/5/doc/source/howtos/gerrit_setup.rst from tobiash above | 21:00 |
| corvus | mordred, mnaser: here's a gertty sketch of what i'm talking about might look like (should be about the same in gerrit web ui): http://paste.openstack.org/show/791491/ | 21:00 |
| mordred | corvus: maybe instead of Warning: more like "Issues in files not in this change:" or something? | 21:02 |
| mordred | but not that - those are terrible words | 21:02 |
| mordred | largely I like the idea | 21:02 |
| corvus | mordred: yeah, the warning is a generic header, but we can probably do something about that | 21:04 |
| corvus | (like, the current implementation adds an entry to the list of warnings for the item) | 21:04 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 21:05 |
| mnaser | corvus: yeah i like that | 21:06 |
| mnaser | corvus, mordred, tobiash: i havecome up with "somewhat" of a framework to testing our "outputs" in the golangci-lint stuff | 21:06 |
| mnaser | we can probably reuse that, and all we really need to do is create a yaml file with data, workdir and expected output comments | 21:07 |
| clarkb | somewhat orthogonal but I thought golang had a built in linter | 21:07 |
| clarkb | liek you didn't need to do anything it just forced it on everyone | 21:07 |
| tobiash | mnaser: cool a file comments parser test framework :) | 21:08 |
| y2kenny | clarkb: are you thinking about go fmt? | 21:08 |
| mnaser | clarkb: it does, but golangci-lint is kindof say.. similar to pylint where it combines a bunch of the most popular ones like static checks etc etc all into one package | 21:08 |
| mnaser | tobiash: yep, just need to figure out how to put it somewhere more 'central' in the repo as a next step and i think that will make testing the tox one very trivial :D | 21:08 |
| clarkb | mnaser: gotcha so its more extensive than the default | 21:09 |
| mnaser | clarkb: yep :) | 21:09 |
| tobiash | mnaser: zuul-jobs has a tests dir in its root. I think that might be a good central place for central parts | 21:10 |
| mnaser | tobiash: can we import that from the role library tests somehow? | 21:11 |
| tobiash | mnaser: that's the question, but we have __init__.py files in the whole chain from root to test file in the module | 21:11 |
| tobiash | so it might be as easy as 'import tests.foo' | 21:12 |
| tobiash | stestr runs from the root appearently so I think it should just work | 21:13 |
| mnaser | let me try locally quickly | 21:13 |
| *** y2kenny has quit IRC | 21:13 | |
| mnaser | oh yeah i got it | 21:17 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 21:18 |
| tobiash | :) | 21:18 |
| mnaser | tobiash: ^ that worked :) thanks! this is shaping up nicely | 21:18 |
| openstackgerrit | Merged zuul/zuul-registry master: Add debug/verification for uploads https://review.opendev.org/716444 | 21:22 |
| tobiash | mnaser: added a question | 21:22 |
| mnaser | tobiash: ah yes, good catch | 21:23 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 21:27 |
| mnaser | tobiash: finally. that should be it. i also cleaned up the yaml load warnings too | 21:27 |
| tobiash | mnaser: one comment, lgtm apart from that | 21:29 |
| openstackgerrit | Mohammed Naser proposed zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 21:31 |
| mnaser | tobiash: thanks, addressed :) | 21:31 |
| tobiash | lgtm | 21:31 |
| mnaser | woot, i'm excited, that should make it easy to write tests for the tox based one now without worrying that we broke things | 21:32 |
| tobiash | yeah, I'll look into that tomorrow probably if no one else gets bored ;) | 21:32 |
| corvus | mnaser: i like that, you keep the input/expected output in the same place (the yaml file) | 21:32 |
| mnaser | corvus: yep, and the generator code means no "hey you forgot to create a test but created a file" or lots of repeated stuff | 21:33 |
| mnaser | one place to look when it breaks | 21:33 |
| mnaser | tobiash: cool! look forward for that :) i'll get back to dealing with other stuff for now :) | 21:34 |
| *** y2kenny has joined #zuul | 21:49 | |
| *** frickler_ has joined #zuul | 22:03 | |
| corvus | mnaser: want any more review on that or should i push it through? | 22:04 |
| *** aspiers has quit IRC | 22:08 | |
| *** frickler has quit IRC | 22:08 | |
| *** rlandy is now known as rlandy|brb | 22:12 | |
| *** aspiers has joined #zuul | 22:19 | |
| openstackgerrit | Merged zuul/zuul-jobs master: local-log-download : role with script to download all log files https://review.opendev.org/715756 | 22:49 |
| *** rlandy|brb is now known as rlandy | 22:58 | |
| *** hashar has quit IRC | 22:59 | |
| *** tosky has quit IRC | 23:03 | |
| mnaser | corvus: no i think it's actually good, i'm happy with the state of it | 23:43 |
| corvus | done | 23:44 |
| mnaser | corvus: yay, awesome. | 23:44 |
| openstackgerrit | Merged zuul/zuul-jobs master: golangci-lint: add job https://review.opendev.org/716452 | 23:56 |
| *** rlandy has quit IRC | 23:58 | |
| *** sshnaidm is now known as sshnaidm|afk | 23:59 | |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!