| opendevreview | Merged openstack/project-config master: openstack-afs.yaml : correct indentation https://review.opendev.org/c/openstack/project-config/+/869772 | 00:17 |
|---|---|---|
| *** swalladge is now known as Guest1027 | 01:51 | |
| opendevreview | Merged openstack/project-config master: Add nb04 config https://review.opendev.org/c/openstack/project-config/+/869769 | 02:41 |
| *** bhagyashris|brb is now known as bhagyashris | 06:39 | |
| *** bhagyashris is now known as bhagyashris|afk | 06:39 | |
| *** soniya29 is now known as soniya29|lunch | 08:01 | |
| *** jpena|off is now known as jpena | 08:29 | |
| *** soniya29|lunch is now known as soniya29 | 09:18 | |
| opendevreview | daniel.pawlik proposed openstack/ci-log-processing master: Add Opensearch dashboards backup script; add dashboards objects https://review.opendev.org/c/openstack/ci-log-processing/+/860811 | 09:22 |
| opendevreview | daniel.pawlik proposed openstack/ci-log-processing master: DNM Add ndjson OpenSearch Dashboards objects https://review.opendev.org/c/openstack/ci-log-processing/+/869800 | 09:28 |
| opendevreview | daniel.pawlik proposed openstack/ci-log-processing master: Add Opensearch dashboards backup script; add dashboards objects https://review.opendev.org/c/openstack/ci-log-processing/+/860811 | 09:39 |
| *** rlandy|out is now known as rlandy | 11:15 | |
| *** bhagyashris|afk is now known as bhagyashris | 11:32 | |
| Tengu | hello there! Quick question for you, Infra folks ( fungi, clarkb maybe?): do we have the "ansible-galaxy" command available in the test job for system-config repository? If so, I'd be happy to update https://review.opendev.org/c/opendev/system-config/+/869819 in order to be 100% sure it's working now. | 12:45 |
| *** soniya29 is now known as soniya29|afk | 13:04 | |
| clarkb | Tengu: I'm not sure I understand the question. Test jobs regardless of repository are typically expected to manage their dependencies direclt.y If you need a command installed then you need to install it | 15:50 |
| Tengu | clarkb: the question was: is that command already available in the test associated to system-config repository, if not, how shall I add it, and, well, "is it a good idea"? | 15:51 |
| clarkb | Tengu: I don't think it is already available. We don't use ansible galaxy anywhere anymore instead we just directly install git repos iirc. | 15:52 |
| clarkb | You can install it (via pip?) in a test job to test the mirror setup | 15:52 |
| Tengu | clarkb: ok. I'll have a look shortly. | 15:53 |
| Tengu | I'd rather double-check the proxy is working ^^'. The "it's work locally" is nice, but... ;) | 15:54 |
| Tengu | 'k, found a way that should make things OK - without installing ansible. | 16:35 |
| *** d34dh0r5| is now known as d34dh0r53 | 16:56 | |
| *** d34dh0r53 is now known as d34dh0r5- | 16:58 | |
| *** dmellado_ is now known as dmellado | 17:03 | |
| *** jpena is now known as jpena|off | 17:21 | |
| dansmith | fungi: did you ever notice or hear anything about the new line limit restriction for syntax highlighting? | 19:11 |
| dansmith | it's low enough that a large number of nova files (that I've look at thus far) aren't highlighted, which is a real bummer | 19:12 |
| fungi | ahh, no, i ended up sidetracked by other fires | 19:17 |
| fungi | sorry about that | 19:17 |
| fungi | it might be of general interest to infra-root sysadmins and other folk now that more people are around again | 19:18 |
| fungi | were you able to narrow down what the limit might be? | 19:18 |
| clarkb | fungi: its like 500 characters or something | 19:18 |
| clarkb | gerrit gives you a popup warning on the files affected and tells you what the limit is | 19:18 |
| fungi | ahh, no this is gerrit silently not providing any syntax highlighting, seems like it's when the file is maybe over 2k lines long? | 19:19 |
| clarkb | it isn't silent | 19:19 |
| fungi | we weren't able to narrow down whether it was for sure the file length, and whether if so whether it's linecount or bytecount | 19:19 |
| clarkb | it give you a popup that says it is doing it | 19:19 |
| clarkb | and iirc its by line size. Once it finds a line that is over the threashold it gives up on the entire file | 19:19 |
| fungi | it wasn't giving me a popup, but maybe my browser was blocking/hiding that | 19:20 |
| clarkb | its one of thos elittle yellow alert things | 19:20 |
| dansmith | clarkb: I don't get the popup almost any of the time | 19:20 |
| dansmith | I saw it once yesterday, so I've been looking for it, but it is very much not always showing | 19:20 |
| clarkb | can someone link me ot an example file so I can look? I noticed this after the upgrade and it definite gave me the little yellow window saying why it wasn't highlighting | 19:20 |
| dansmith | clarkb: and what I'm seeing is the entire file is not highlighted, not per line | 19:20 |
| clarkb | dansmith: correct | 19:20 |
| fungi | there were a few examples in channel last week or so, i'll check scrollback | 19:21 |
| dansmith | so maybe the popup is just for the line thing, but we have <80 lines | 19:21 |
| clarkb | it gives up on the entire file once it hits a line above a certain length aiui | 19:21 |
| dansmith | https://review.opendev.org/c/openstack/nova/+/863918/6/nova/tests/unit/compute/test_compute_mgr.py | 19:21 |
| fungi | looks like we talked about it on 2023-01-03 | 19:21 |
| dansmith | I meant "<80 char lines almost everywhere" | 19:21 |
| fungi | no highlighting on https://review.opendev.org/c/openstack/nova/+/867832/2/nova/compute/manager.py for example | 19:21 |
| clarkb | I wonder if it also applies to file length and doesn't give you the warning for that | 19:22 |
| clarkb | or maybe it gives your user a warning once? | 19:22 |
| clarkb | https://review.opendev.org/c/opendev/system-config/+/866781/2/inventory/base/group_vars/all.yaml theres the warning for line length | 19:22 |
| dansmith | not that I ever saw.. first one since the upgrade Ive seen was yesterday, which was probably line length | 19:22 |
| dansmith | now that says the whole file won't be highlighted because of the one line | 19:23 |
| clarkb | my hunch is that they are doing similar on file size but not warning you about it | 19:23 |
| dansmith | but I'm pretty sure those py files are not triggering that as they should all be short lines | 19:23 |
| dansmith | yeah | 19:23 |
| fungi | we saw syntax highlighting on a 3k line file but no syntax highlighting on a 7k line file | 19:23 |
| dansmith | yeah | 19:23 |
| clarkb | polygerrit-ui/app/services/highlight/highlight-service.ts:export const CODE_MAX_LINES = 20 * 1000; | 19:25 |
| clarkb | polygerrit-ui/app/services/highlight/highlight-service.ts:const CODE_MAX_LENGTH = 25 * CODE_MAX_LINES; | 19:25 |
| dansmith | it definitely stops on files <20KLOC | 19:26 |
| clarkb | your example is under that limit | 19:26 |
| clarkb | also the code that checks CODE_MAX_LINES is adjacent to the one that checks max line length and they both fire warnings the same way | 19:26 |
| fungi | maybe pygments or whatever lib provides the highlighting has its own baked-in limit which is coming into play | 19:27 |
| dansmith | well, it's clearly new, so it'd have to be some new restriction in that lib that came in via the upgrade or something | 19:27 |
| clarkb | its looking at getDiffLength though so it may be related to the larger diff and not the size of one side | 19:27 |
| dansmith | pretty sure it happens on large files with small diffs | 19:27 |
| dansmith | https://review.opendev.org/c/openstack/nova/+/863919/6/nova/tests/unit/compute/test_compute_mgr.py | 19:28 |
| dansmith | that's a few lines of diff, large file, no highlight | 19:28 |
| clarkb | ya I suspect the reason we don't get the warning here is we're below those constants. But then something else like the thing actually doing the highlighting is giving up | 19:29 |
| clarkb | if you look in the console log it shows "Diff Syntax Render: 0" Cross checking with the code this seems to be the timing for how long it took to syntax highlight | 19:33 |
| dansmith | do you see these? Deprecated as of 10.7.0. highlight(lang, code, ...args) has been deprecated. | 19:33 |
| dansmith | and a bunch of font failures | 19:34 |
| dansmith | I don't get the diff syntax render message | 19:34 |
| dansmith | I get the deprecated and font messages even on files that do highlight, so must be unrelated | 19:35 |
| clarkb | I get the font stuff but don't see the highlight deprecation | 19:35 |
| clarkb | in the syntax highlighter worker it checks if it is enabled and if not returns early. So now i'm back to thinking something trips that enabled flag to fale | 19:35 |
| clarkb | *false | 19:35 |
| dansmith | also something about a deprecated plugin api with opendev-theme | 19:35 |
| dansmith | these are all my warnings, just FYI: https://pastebin.com/GaFf0fAZ | 19:36 |
| clarkb | the theme is a known thing. They changed their js framework again so all of our existing stuff has to be updated prior to the 3.7 upgrade | 19:37 |
| clarkb | but it should be fine in 3.6 | 19:37 |
| dansmith | okay | 19:37 |
| clarkb | there is some formula they use to calculate the diff size. I wonder if that could be it (but then it doesn't explain why we don't get the alert for this case) | 19:48 |
| fungi | could it be that the highlighter is taking too long on larger files for some reason, so gerrit's giving up on that and just showing the raw uncolorized version? | 19:53 |
| clarkb | fungi: maybe, though it it self reporting it took 0 time (ms?) to do the syntax rendering which implies to me it isn't even trying | 19:54 |
| clarkb | ok heres my hunch its CODE_MAX_LENGTH (not CODE_MAX_LINES) that we are tripping over | 19:55 |
| clarkb | that allows for so many bytes I think before it short circuits and I'm guessing we're over that | 19:56 |
| clarkb | let me get a code link | 19:56 |
| clarkb | https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.6.3/polygerrit-ui/app/services/highlight/highlight-service.ts#133 I'm pretty sure that is causing it | 19:57 |
| clarkb | why they don't do the check when they do the other checks and raise the alert I don't know but no alert is raised there | 19:57 |
| clarkb | manager.py is ~551364 bytes | 19:58 |
| clarkb | we are allowed 25 * 20 * 1000 = 500000 bytes which we are just over | 19:59 |
| clarkb | I can file a bug for that if we can articulate what the issue(s) is/are. At the very least it seems like they should do these length checks consistently so they all alert consistently to avoid user confusion | 20:02 |
| dansmith | why would that not be a configurable limit? | 20:02 |
| clarkb | you'd have to ask them. But its probably based on some benchmarking of their syntax highlighting tool | 20:02 |
| dansmith | but it clearly got turned down recently | 20:03 |
| clarkb | or maybe wasn't there before | 20:03 |
| dansmith | and even still, if we all have fast machines with lots of memory, should be our choice | 20:03 |
| clarkb | so maybe "user programmable limits" for syntax highlighting | 20:03 |
| dansmith | yeah | 20:03 |
| dansmith | what I meant by my why question is, why wouldn't the bug be "this limit should be configurable" | 20:04 |
| clarkb | it wouldn't surprise me if google has a response time requirement for all google apps and this kept them under it | 20:04 |
| clarkb | confirmed that a googler made the change. They also annotated the commit as not deserving a release note | 20:05 |
| dansmith | lol | 20:05 |
| clarkb | it says "safe guard for not killing the browser" | 20:07 |
| clarkb | but unfortinately no additioanl info on how those limits were chosen | 20:07 |
| clarkb | there is a google bug id though | 20:07 |
| clarkb | so if you worked at google you could probably find out | 20:08 |
| clarkb | I need to figure out lunch, but I can work on a bug after | 20:08 |
| dansmith | well, unless they made it substantially suckier, they just chose a value low enough to make their target and not actually avoid killing the browser | 20:08 |
| clarkb | I think there are two parts the first is consistently reporting the givng up to avoid confusion and second suggesting it be user configurable | 20:08 |
| dansmith | or, not just | 20:09 |
| dansmith | yeah | 20:09 |
| dansmith | thanks clarkb | 20:09 |
| opendevreview | Jay Faulkner proposed openstack/project-config master: Noop change to ironic-inspector.config https://review.opendev.org/c/openstack/project-config/+/869872 | 21:51 |
| clarkb | dansmith: fungi https://bugs.chromium.org/p/gerrit/issues/detail?id=16592 | 22:07 |
| dansmith | nice, thanks | 22:08 |
| opendevreview | Jay Faulkner proposed openstack/project-config master: Noop change to ironic-inspector.config https://review.opendev.org/c/openstack/project-config/+/869872 | 22:10 |
| opendevreview | Jay Faulkner proposed openstack/project-config master: Remove redundant editHashtag lines https://review.opendev.org/c/openstack/project-config/+/869878 | 22:35 |
| *** rlandy is now known as rlandy|out | 22:58 | |
| opendevreview | Merged openstack/project-config master: Remove redundant editHashtag lines https://review.opendev.org/c/openstack/project-config/+/869878 | 22:58 |
| opendevreview | Jay Faulkner proposed openstack/project-config master: Correct syntax on toggleWipState https://review.opendev.org/c/openstack/project-config/+/869882 | 23:12 |
| *** dasm is now known as dasm|off | 23:20 | |
| opendevreview | Merged openstack/project-config master: Correct syntax on toggleWipState https://review.opendev.org/c/openstack/project-config/+/869882 | 23:50 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!