| clarkb | mnasiadka: I still have reservations and as I said I won't repvent it from merging. Basically I'm not going to state my reservations (which I've done) but I won't -2 it. | 16:00 |
|---|---|---|
| clarkb | that means I won't approve it either | 16:00 |
| clarkb | basically if others feel strongly enough about it to proceed then great but I won't be in that group | 16:00 |
| clarkb | My stance is that we've had this default for liek a decade. If those defaults don't work for some reason you are more than welcome to override them. | 16:01 |
| clarkb | but if we suddenly feel these defaults are inapproprite then cool I'm happy to be the lone person who thinks otherwise | 16:01 |
| mnasiadka | clarkb: we default to hidden from Sep 2023 - https://review.opendev.org/c/openstack/diskimage-builder/+/895486 - so far from a decade | 16:05 |
| clarkb | the timeout is the one I acre about | 16:05 |
| mnasiadka | Hmm, seems we changed from 5 seconds to 1 in https://review.opendev.org/c/openstack/diskimage-builder/+/925451 | 16:08 |
| clarkb | but that also indicates there was some other default? Maybe we only thought it was that way and it never was? | 16:08 |
| mnasiadka | I have no clue, we could better document that we recommend 0/1 for VM images and somewhere around 5 for bare metals (so you can press that damn button on time) - we could also compromise on something like 3 seconds if that makes any difference for you | 16:10 |
| clarkb | oh based on the comments in taht change some images override the value | 16:10 |
| JayF | clarkb: mnasiadka: commented; i'm similar to clark as I won't block it but I'm not sure we should have that churn | 16:11 |
| JayF | (I do, however, <3 the append_or_replace helper) | 16:12 |
| clarkb | JayF: oh I kinda wish I -2'd that change particularly now | 16:12 |
| clarkb | the old code was never wrong | 16:13 |
| JayF | I mean, you have two cores who are like, half-uneasy, does that add up ot a full -1/2? lol | 16:13 |
| clarkb | the reason for the append or replace helper was apparently red hat customers found bash env files to be confusing | 16:13 |
| JayF | I just know from working at Rackspace that if all the sudden some of our VM images had a new 5 second wait, we'd be certain to get support tickets about it. | 16:13 |
| clarkb | and unfrotauntely we've shifted the confusion to dib maintainers | 16:13 |
| JayF | and that'd be not just at provision time but every reboot | 16:14 |
| JayF | for many VMs, that'd be literally more than doubling the amount of time to be able to log back in from a nova reboot | 16:14 |
| clarkb | ya thats why I'm uneasy about it. I think cases where you have to debug grub are rare and in those cases you're likely deep in the trenches editing images anyway | 16:15 |
| JayF | Mine is much more basic than that | 16:15 |
| clarkb | re hidden -> menu you just hit <esc> and the menu shows up | 16:15 |
| clarkb | so I think that change is fairly inconsequential either way | 16:15 |
| JayF | if the situation was flipped, and we currently had the proposed defaults in tree and vice-versa, I'd still be -1 | 16:15 |
| clarkb | ya though apparently this default was not set in the base bootloader element until very recently | 16:16 |
| JayF | they are just optimizing for different use cases, and many folks who thought the were done worrying about image building will get behavior changes | 16:16 |
| clarkb | but overriding it elsewhere was common | 16:16 |
| clarkb | was is common | 16:16 |
| * JayF wonders if there's a deeper requirement there that isn't being well-communicated | 16:16 | |
| clarkb | mnasiadka: there is something weird about this file if you run git log -p against it then search for DIB_GRUB_TIMEOUT. I don't see a commit ever adding it for the first time | 16:21 |
| clarkb | it first shows up in the context around a diff but not in a diff itself | 16:21 |
| clarkb | 61087d33e9ef67f05ef4a3b0dfc90ab521604292 seems to add it if I look at repo wide diffs. Maybe there was a file rename | 16:22 |
| clarkb | oh ya I think this got lost as part of the elements/ -> diskimage_builder/elements move | 16:22 |
| clarkb | "lost" git still sees it on both sides but doesn't map them together for me for some reason | 16:22 |
| clarkb | and ya that confirms the original value was 5 seconds. Overrides would've happend elsewhere not in this base element so the change in 2024 was a potential regression | 16:23 |
| clarkb | ok I'm less concerned about the timeout now that I've traced this all the way back | 16:25 |
| JayF | okay cool | 16:25 |
| JayF | can you post a comment in there with ^^ this info | 16:25 |
| JayF | I will likely +2 at that point, given my argument was "status quo prevents churn", this means we churned accidentally and want to unchurn :D | 16:26 |
| clarkb | ya I'm trying to sort out the hidden change next | 16:26 |
| clarkb | mnasiadka: JayF I posted a comment with a -1 because I found a bug in the docs update | 16:35 |
| clarkb | I'm willing to fix that myself and push a new patch if we want to turn this around more quickly. Happy to review someone else's update too | 16:35 |
| clarkb | let me load ssh keys then I'll get an update pushed which we can land | 16:36 |
| opendevreview | Clark Boylan proposed openstack/diskimage-builder master: Change grub variables for style and timeout https://review.opendev.org/c/openstack/diskimage-builder/+/937684 | 16:47 |
| clarkb | that fixes the docs bug and adds a comment to try and prevent the back and forth from continuing in the future | 16:47 |
| JayF | :eyes | 16:48 |
| JayF | * 👀 | 16:48 |
| opendevreview | Clark Boylan proposed openstack/diskimage-builder master: Change grub variables for style and timeout https://review.opendev.org/c/openstack/diskimage-builder/+/937684 | 16:49 |
| clarkb | I realized I went over the 80 char line length limit in the rst file. I wasn't sure if that would make CI unhappy but decided to not give it a chance to be sad | 16:49 |
| clarkb | second patchset from me is otherwise identical | 16:49 |
| mnasiadka | Thanks guys, got a bit distracted because it’s 6pm here :) | 17:06 |
| opendevreview | Merged openstack/diskimage-builder master: Change grub variables for style and timeout https://review.opendev.org/c/openstack/diskimage-builder/+/937684 | 21:12 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!