| *** altlogbot_3 has quit IRC | 09:24 | |
| *** altlogbot_3 has joined #openstack-jjb | 09:25 | |
| Odd_Bloke | philroche: https://storyboard.openstack.org/#!/story/2006254 <-- this is being reported as something your patch introduced | 13:21 |
|---|---|---|
| philroche | Odd_Bloke: Thanks for the heads up. I have replied asking reporters to try patch https://pastebin.ubuntu.com/p/F3w5wHcdtr/ | 13:32 |
| Odd_Bloke | philroche: It sounds like it should be fairly easy to reproduce (he said, having not tried at all himself ;), is it worth creating a testcase for the affected configuration so we know we won't regress it in future? | 13:35 |
| philroche | Yes, I am in the process of adding tests now | 13:35 |
| Odd_Bloke | OK, great! | 13:39 |
| Odd_Bloke | philroche: Ping me once you have something up and I'll review. | 13:39 |
| *** electrofelix has joined #openstack-jjb | 14:40 | |
| *** openstackgerrit has joined #openstack-jjb | 15:48 | |
| openstackgerrit | Phil Roche proposed jjb/jenkins-job-builder master: Avoid ResourceWarning by closing file handlers when finished https://review.opendev.org/673317 | 15:48 |
| openstackgerrit | Phil Roche proposed jjb/jenkins-job-builder master: Bugfix: deep_formatting non templates caused regressions when variables used https://review.opendev.org/673318 | 15:48 |
| philroche | Odd_Bloke: https://review.opendev.org/#/c/673318/1 has been proposed. It does fix the reported issues but I'm not sure it's the right approach long term | 15:52 |
| Odd_Bloke | philroche: I agree, that feels like we'll be playing whack-a-mole. | 16:00 |
| philroche | Yup | 16:00 |
| Odd_Bloke | So what's the root cause of the bug? Is it that we changed the default behaviour from "template in job-templates" to "template everything"? | 16:01 |
| Odd_Bloke | Or is it less broad than that? | 16:01 |
| Odd_Bloke | s/everything/\0 except DSLs/, I guess. | 16:04 |
| philroche | Correct. Yes | 16:04 |
| philroche | Well not exactly - more that we now call deep_format on everything (apart from dsl)so we can use !include-X calls in jobs rather than just job-templates and builders | 16:07 |
| Odd_Bloke | philroche: If we aren't in a job-template, what context does the Jinja2 rendering have? | 16:11 |
| philroche | The specific context I was trying to solve was being able to build yaml in multiple job configs from a common source, thus reducing duplicate configuration. | 16:14 |
| Odd_Bloke | philroche: So I'm wondering if we can solve this problem by rendering the Jinja2 template at include time. | 16:16 |
| philroche | As part of the CustomLoader? | 16:16 |
| Odd_Bloke | I don't _think_ the !include equivalent should be doing any template substitution, it should be including YAML _before_ the templating phase. | 16:17 |
| Odd_Bloke | (But let me be very clear that this is conjecture, I don't really have a good mental model of how this all works any longer.) | 16:17 |
| philroche | No, that does make sense | 16:17 |
| Odd_Bloke | So I think we can dispense with the loader entirely? | 16:17 |
| Odd_Bloke | And YamlIncludeJinja2AsYaml._from_file should render the template with no arguments? | 16:19 |
| Odd_Bloke | Does that make sense? | 16:19 |
| philroche | Yes, I think so. I'll try that approach | 16:22 |
| openstackgerrit | Phil Roche proposed jjb/jenkins-job-builder master: Bugfix: deep_formatting non templates caused regressions when variables used https://review.opendev.org/673318 | 16:58 |
| openstackgerrit | Phil Roche proposed jjb/jenkins-job-builder master: Bugfix: deep_formatting non templates caused regressions when variables used https://review.opendev.org/673318 | 17:00 |
| philroche | Odd_Bloke: https://review.opendev.org/#/c/673318 now updated and avoids the whack-a-mole approach without removing the ability to template !includes | 17:03 |
| openstackgerrit | Phil Roche proposed jjb/jenkins-job-builder master: Bugfix: deep_formatting non templates caused regressions when variables used https://review.opendev.org/673318 | 17:13 |
| Odd_Bloke | philroche: Did the changes to the local_yaml.py objects not work out? | 17:33 |
| philroche | They were not required | 17:33 |
| Odd_Bloke | philroche: I'm concerned about no longer formatting strings, that seems like a big behaviour change. | 17:36 |
| Odd_Bloke | (Which might not be necessary if we can drop the deep_format call you added in parser.py by making the local_yaml changes?) | 17:38 |
| philroche | Odd_Bloke: We still format strings if they are part of a template which is the same behaviour as prior to my original patchset | 17:39 |
| philroche | If not a template I don't see why we would want to attempt to format strings | 17:40 |
| philroche | Also, if we don't call deep_format recursively then the !include will never happen and will remain as "!include" string | 17:44 |
| Odd_Bloke | philroche: !include was working before any of these changes, so I don't see how any of them can now be required to make it work? | 17:52 |
| Odd_Bloke | And I _think_ the !include* substitution happens in the YAML parsing, well before we're formatting anything. | 17:52 |
| *** electrofelix has quit IRC | 18:06 | |
| philroche | Odd_Bloke: From my initial testing !includes only worked inside a job-template or a builder | 18:11 |
| Odd_Bloke | philroche: https://paste.ubuntu.com/p/8vXbxFmv8P/ and https://paste.ubuntu.com/p/s6qswph72j/ together work for me on 2.10.1. | 18:15 |
| philroche | Odd_Bloke: Yes, that is part of a builder | 18:15 |
| Odd_Bloke | Oh, I see, my mistake. | 18:16 |
| Odd_Bloke | Let me try something else. | 18:16 |
| philroche | Thanks for your help. I am going EOD now but I will continue investigating tomorrow | 18:17 |
| Odd_Bloke | philroche: OK, I don't think it's _builders_ per se, I think you can't !include mappings. | 18:17 |
| Odd_Bloke | philroche: zxiiro: I wonder if we should revert that change (and re-release) as we haven't got a clear path to resolution yet and it's breaking people. What do you think? | 18:18 |
| philroche | +1 | 18:18 |
| Odd_Bloke | OK, I'll propose that. | 18:21 |
| Odd_Bloke | philroche: Have a good evening! | 18:21 |
| zxiiro | philroche: Odd_Bloke just catching up.... I'm happy to produce a patch release but someone's going to have to propose the revert. | 18:31 |
| zxiiro | is there a patch already? | 18:31 |
| Odd_Bloke | zxiiro: I'm just configuring gerrit on this machine, then I will propose one. | 18:36 |
| zxiiro | sounds good | 18:37 |
| Odd_Bloke | Ugh, gerrit setup sucks. | 18:39 |
| openstackgerrit | Daniel Watkins proposed jjb/jenkins-job-builder master: Revert "Add support for rendering jinja template as yaml" https://review.opendev.org/673347 | 18:42 |
| Odd_Bloke | Finally. | 18:42 |
| Odd_Bloke | I'm still having to fill in my password manually every time I run something, but whatever. | 18:42 |
| Odd_Bloke | zxiiro: Pinging in case you didn't see the above. | 19:25 |
| zxiiro | Odd_Bloke: approved. sorry was busy with work | 19:26 |
| Odd_Bloke | Not a problem! | 19:27 |
| zxiiro | I'll cut a release once Zuul merges that | 19:27 |
| Odd_Bloke | Thanks! | 19:27 |
| openstackgerrit | Merged jjb/jenkins-job-builder master: Revert "Add support for rendering jinja template as yaml" https://review.opendev.org/673347 | 19:36 |
Generated by irclog2html.py 2.15.3 by Marius Gedminas - find it at mg.pov.lt!