| *** yoctozepto8 is now known as yoctozepto | 00:33 | |
| *** yoctozepto6 is now known as yoctozepto | 01:21 | |
| *** yoctozepto3 is now known as yoctozepto | 02:12 | |
| *** yoctozepto8 is now known as yoctozepto | 03:43 | |
| *** yoctozepto8 is now known as yoctozepto | 03:57 | |
| *** ysandeep|away is now known as ysandeep | 04:09 | |
| *** yoctozepto0 is now known as yoctozepto | 04:12 | |
| *** yoctozepto1 is now known as yoctozepto | 04:40 | |
| *** ysandeep is now known as ysandeep|brb | 05:23 | |
| *** ysandeep|brb is now known as ysandeep | 05:35 | |
| *** yoctozepto7 is now known as yoctozepto | 05:43 | |
| *** yoctozepto0 is now known as yoctozepto | 06:24 | |
| *** redrobot0 is now known as redrobot | 06:29 | |
| *** yoctozepto9 is now known as yoctozepto | 07:00 | |
| *** rpittau|afk is now known as rpittau | 07:27 | |
| *** ysandeep is now known as ysandeep|lunch | 07:29 | |
| *** jpena|off is now known as jpena | 07:35 | |
| *** yoctozepto1 is now known as yoctozepto | 07:52 | |
| opendevreview | Jing Li proposed openstack/diskimage-builder master: Add new element rocky https://review.opendev.org/c/openstack/diskimage-builder/+/805793 | 08:26 |
|---|---|---|
| *** ykarel is now known as ykarel|lunch | 08:34 | |
| PaulFertser | Hello. I would appreciate if I could talk to Gertty devs. Would like to file a bugreport about reproducible crash etc, but without having to register a ubuntu account. Am I missing something? | 09:16 |
| *** ysandeep|lunch is now known as ysandeep | 09:35 | |
| opendevreview | Jing Li proposed openstack/diskimage-builder master: Add new element rocky https://review.opendev.org/c/openstack/diskimage-builder/+/805800 | 09:46 |
| opendevreview | Lucas Alvares Gomes proposed openstack/project-config master: New project: OVN BGP Agent https://review.opendev.org/c/openstack/project-config/+/805802 | 09:57 |
| opendevreview | Lucas Alvares Gomes proposed openstack/project-config master: New project: OVN BGP Agent https://review.opendev.org/c/openstack/project-config/+/805802 | 10:16 |
| *** ykarel|lunch is now known as ykarel | 10:23 | |
| *** ykarel_ is now known as ykarel | 10:56 | |
| *** ysandeep is now known as ysandeep|afk | 10:58 | |
| opendevreview | Slawek Kaplonski proposed openstack/project-config master: Update Neutron's Grafana dashboard https://review.opendev.org/c/openstack/project-config/+/805809 | 11:03 |
| *** dviroel|out is now known as dviroel|ruck | 11:23 | |
| *** jpena is now known as jpena|lunch | 11:36 | |
| *** ysandeep|afk is now known as ysandeep | 12:00 | |
| PaulFertser | dtantsur: Hello. I have a "production" Gerrit 3.4.0 server running here and unfortunately Gertty crashes trying to show diff for new commits. I have an easy way to workaround the crash but not a Launchpad account to offer a patch. Would you probably be willing to take a look (and to suggest a more informed solution)? | 12:04 |
| fungi | PaulFertser: corvus is the primary maintainer of gertty, also thanks for the heads up about potential incompatibilities with 3.4.0, we're hoping to be able to upgrade our deployment to it fairly soon as well | 12:14 |
| PaulFertser | fungi: I hope it's ok for me to stay here to discuss and probably interactively test gertty related issues? | 12:15 |
| fungi | i know there are people using gertty with upstream gerrit's gerrit, so i wonder if they're hitting the same issues with it | 12:15 |
| fungi | PaulFertser: sure, are you using gertty's current master branch state, or the 1.6.0 release from a couple years ago? | 12:16 |
| fungi | it's possible some fixes have merged which get it working better with new gerrit versions but haven't been tagged | 12:17 |
| PaulFertser | We at OpenOCD recently upgraded from 2.13 to 3.4.0 and it was all pretty smooth, the only nasty surprise was that we were using replication plugin to push to public git repos and it pushed all the refs/* users, metadata, changes etc. | 12:17 |
| PaulFertser | fungi: current master, 1.6.0-32-gcd60a82f6ab5 | 12:18 |
| PaulFertser | Basically, the crash is due to "handleUndisplayedComments" getting something like {'new-None-/PATCHSET_LEVEL': [(2237, [('comment-name', 'Tarek BOCHKATI'), ('comment', ': the warnings...')])]} and None is not a line number. | 12:20 |
| fungi | yeah, we upgraded from 2.13 to 3.2.0 last november and are very close to upgrading to 3.3 now, holding off going to 3.4 until the dust settles a little there since it looked like there were lots of new bug reports | 12:20 |
| PaulFertser | What also was bad for our project specifically is that PolyGerrit only shows "conflicts" information for "mergeable" commits, which is not helping to spot earlier duplicated work as many commits are not mergeable (especially with cherry-picking strategy). This is trivial to patch in Gertty, but not so much with PolyGerrit UI. | 12:22 |
| fungi | as for the user refs, we actually discovered a serious bug in gerrit's permissions model dating back to the notedb switch (now patched) but that held up our upgrade timeframe a lot making sure they had fixed it before we inadvertently exposed all our users' password hashes and such | 12:23 |
| fungi | if it hadn't been for extensive upgrade testing we wouldn't have caught that, clearly nobody else had in the years since they moved user refs to notedb (in 2.15 i think?) | 12:24 |
| fungi | anyway, it has us a little gun-shy about untested upgrades as a result | 12:25 |
| fungi | PaulFertser: can you paste the complete traceback somewhere like https://paste.opendev.org/ and link the url here? that'll hopefully make it a little easier to see where the exception is being raised | 12:27 |
| fungi | there is also a debug mode you can set by passing -d on the command line, which will log things much more verbosely, could help in identifying the queries involved | 12:29 |
| PaulFertser | fungi: sure https://termbin.com/li5g | 12:30 |
| PaulFertser | fungi: that sudden replication did kinda expose sensitive information actually | 12:32 |
| *** jpena|lunch is now known as jpena | 12:34 | |
| fungi | okay, so gertty is taking the key names from comment_lists taking the first one, splitting on hyphen (-) and expecting the third part of that to be a base-10 integer representation of the line number for an inline code comment | 12:34 |
| fungi | that does seem to be making some assumptions about the formatting of those comment key names, which probably isn't part of a stable api specification for gerrit | 12:35 |
| PaulFertser | The second part to be a line number, yes. | 12:36 |
| PaulFertser | I think that specific part is what Gertty produces on its own, separating by dashes for whatever odd reason. | 12:37 |
| fungi | oh, yep you're right, it's key.split('-', 2) but i misread it as key.split('-')[2] | 12:37 |
| fungi | not done with my coffee yet ;) | 12:37 |
| PaulFertser | Can't a normal tuple be used as a key? | 12:37 |
| fungi | good question, i'll read back farther to see how comment_lists comes about | 12:38 |
| fungi | oh, yeah the BaseDiffView._init() method is building those, you're right | 12:39 |
| PaulFertser | I did my homework ;) | 12:40 |
| fungi | so it's expecting session.getRevision().comments entries to have a line attribute, which in at least some cases is None? | 12:43 |
| kevinz | Hi clarkb: ack, I will check | 12:43 |
| fungi | thanks kevinz! | 12:43 |
| fungi | PaulFertser: is it certain diffs which crash or any diff? | 12:44 |
| PaulFertser | fungi: feels like any diff when a change is created after upgrade | 12:45 |
| fungi | guessing gerrit 3.4 has added some new comment type which isn't tied to line numbers (similar to the general file comments maybe?) | 12:45 |
| PaulFertser | And that filename "/PATCHSET_LEVEL" is odd, and comment is in fact a regular comment, not line comment. | 12:45 |
| PaulFertser | You might be right, it's possible a file comment. | 12:47 |
| fungi | well, file comments in earlier gerrit versions don't seem to crash gertty, but maybe the way they're recorded has changed | 12:47 |
| PaulFertser | Not sure how to tell by just looking https://review.openocd.org/c/openocd/+/6470 | 12:47 |
| fungi | so just to be clear, changes created by 2.13 viewed via 3.4 aren't crashing gertty, but changes created under 3.4 are | 12:48 |
| PaulFertser | It's like a "patchset" comment | 12:48 |
| PaulFertser | fungi: yep | 12:48 |
| fungi | so this could certainly be a new comment type in 3.4 gertty just doesn't know how to parse | 12:49 |
| PaulFertser | My own comments to that change as sent by gertty are marked as "resolved" right away, not sure if I did anything wrong. | 12:50 |
| fungi | looking in the 3.4.0 release notes, i wonder if it could be "Issue 14102; Return empty context if the comment range is outside file boundaries" | 12:51 |
| fungi | there's also "Return empty comment context if line and range info are not available" | 12:53 |
| fungi | interesting, there's an experimental feature in 3.4 to port comments to newer patchsets as well | 12:55 |
| PaulFertser | fungi: please see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#file-id , it's a special filename /PATCHSET_LEVEL | 12:58 |
| fungi | oh neat | 12:58 |
| fungi | anyway, hopefully corvus has some ideas too once he's around. i know he's been using gertty with gerrit-review.googlesource.com which claims to be running 3.4.0 | 12:59 |
| PaulFertser | The comment is displayed nicely in the main change view so it should just be ignored for diff. | 13:01 |
| fungi | yeah, if gertty can identify those then it can probably safely skip them in diff view | 13:04 |
| fungi | i guess we just match on the special filename to identify them | 13:05 |
| opendevreview | Shnaidman Sagi (Sergey) proposed openstack/diskimage-builder master: WIP Add building for CentOS 9 https://review.opendev.org/c/openstack/diskimage-builder/+/805848 | 13:20 |
| fungi | around line 239 we skip comments for old_revision, so maybe we can similarly there do a continue on that loop if path == '/PATCHSET_LEVEL'? | 13:20 |
| opendevreview | Jeremy Stanley proposed ttygroup/gertty master: Skip patchset-level comments in diff view https://review.opendev.org/c/ttygroup/gertty/+/805850 | 13:26 |
| fungi | PaulFertser: see if it's as trivial as that ^ i guess? | 13:26 |
| fungi | we may find there are other exceptions behind it though, so might need to discard it from the files list in diff.py as well | 13:27 |
| PaulFertser | fungi: if you're asking if that prevents the crash, yes, I'm sure it does. If you're asking if that's the right comprehensive solution, I do not know unfortunately. | 13:29 |
| fungi | yeah, it will need to be reviewed for sure | 13:30 |
| fungi | it's a naive attempt | 13:30 |
| fungi | and also one i haven't tested | 13:30 |
| PaulFertser | I've tested catching the exception and continuing , works fine, and in this limited testing only /PATCHSET_LEVEL comments are offending. We do not have merge commits so can't really test MERGE_LIST. | 13:32 |
| fungi | also /MERGE_LIST has been around at least since 3.2 as it appears in our docs version: https://review.opendev.org/Documentation/rest-api-changes.html#file-id | 13:33 |
| fungi | gertty seems to have special casing for /COMMIT_MSG (creating a virtual file for it), though nothing i can see for /MERGE_LIST | 13:34 |
| PaulFertser | Should be tested too then, but we do not have merges like that. | 13:34 |
| fungi | i'll see if i'm able to diff a merge commit from some project in our gerrit | 13:35 |
| PaulFertser | Thank you, that's what I implied asking for :) | 13:36 |
| *** ykarel is now known as ykarel|afk | 13:37 | |
| PaulFertser | btw, another issue I had is that gertty defaults to digest auth and somehow it doesn't work against our gerrit and there's no easy way to understand what's going wrong. | 13:37 |
| fungi | PaulFertser: gerrit switched to basic auth around 2.16 i think? whenever the password hashing went in. there is a config option to tell gertty to use basic auth | 13:38 |
| fungi | auth-type: basic | 13:39 |
| PaulFertser | fungi: yes, and that's what I had to add to the minimal config, and that was far from obvious (for me) | 13:39 |
| fungi | it would probably make sense for gertty to flip its default at some point, but that's corvus's call | 13:39 |
| fungi | as it would be a non-backwards-compatible change | 13:40 |
| fungi | though maybe it could grow some autodetection mechanism to figure out which it needs | 13:40 |
| opendevreview | Sorin Sbârnea proposed opendev/elastic-recheck master: Replace alpine base image with official python one https://review.opendev.org/c/opendev/elastic-recheck/+/805857 | 13:46 |
| PaulFertser | fungi: a commented-out auth-type in the minimal config would have helped | 13:47 |
| fungi | PaulFertser: a trivial test, but gertty is able to diff https://review.opendev.org/805856 without falling over | 13:47 |
| opendevreview | Sorin Sbârnea proposed opendev/elastic-recheck rdo: Ensure git review uses correct branch https://review.opendev.org/c/opendev/elastic-recheck/+/805858 | 13:48 |
| PaulFertser | fungi: I see https://review.opendev.org/changes/opendev%2Fsandbox~805856/revisions/1/files/%2FMERGE_LIST/diff?context=ALL&intraline&whitespace=IGNORE_NONE returning /MERGE_LIST content | 13:54 |
| fungi | PaulFertser: looking at the example configs, googlesource-gertty.yaml and opendev-gertty.yaml set "auth-type: basic" and reference-gertty.yaml has a comment which mentions that the value defaults to digest but also supports being set to basic (as does the config documentation) | 13:55 |
| clarkb | PaulFertser: re the gerrit upgrade we went from 2.16 to 3.2 ourselves and had a few bumps. For replication we set an authentication level on the replication tasks to anonymous users as well as control the remote repo creation separately to prevent All-Projects and All-Users from being replicated. | 13:55 |
| clarkb | authGroup: 'Anonymous Users' is the replication setting | 13:55 |
| fungi | minimal-gertty.yaml seems to intentionally not have commented-out entries | 13:55 |
| PaulFertser | fungi: but it has user and password, so probably not commented out auth-type: (basic|digest) could fit. | 13:56 |
| PaulFertser | clarkb: I meant I had issues with replication plugin itself, not an external task. | 13:57 |
| clarkb | PaulFertser: yes I know. replication functions as internal gerrit tasks and you can assign a privilege level using the authGroup flag | 13:58 |
| clarkb | if you do a gerrit show-queue you see replication tasks along with other internal task activity | 13:58 |
| opendevreview | Jeremy Stanley proposed ttygroup/gertty master: Suggest basic auth in minimal example https://review.opendev.org/c/ttygroup/gertty/+/805859 | 13:59 |
| fungi | PaulFertser: ^ | 13:59 |
| PaulFertser | clarkb: I see what you mean, thanks. | 13:59 |
| PaulFertser | fungi: thank you :) | 14:00 |
| corvus | PaulFertser, fungi : just acking that i've read the conversation; thanks' for that. i'll look into that later today | 14:25 |
| fungi | thanks corvus! | 14:27 |
| *** ysandeep is now known as ysandeep|away | 14:30 | |
| *** ykarel|afk is now known as ykarel | 14:56 | |
| opendevreview | Merged opendev/irc-meetings master: Update irc channel for security sig meetings https://review.opendev.org/c/opendev/irc-meetings/+/805093 | 15:08 |
| PaulFertser | corvus: thank you! I'll continue lurking here in case you need me. | 15:12 |
| PaulFertser | Auto-detection auth type should be possible like in this answer: https://stackoverflow.com/a/19427242 | 15:18 |
| *** jpena is now known as jpena|off | 15:33 | |
| PaulFertser | Hm, I can use mouse to follow Depends on/Needed by/Conflicts with but not able to select and follow with keyboard input. | 15:51 |
| *** rpittau is now known as rpittau|afk | 16:00 | |
| fungi | PaulFertser: i'd noticed the same recently, but haven't found time to dig into the cause | 16:00 |
| clarkb | https://github.com/go-gitea/gitea/issues/16802 is an issue to watch with gitea. They seem to be saying there that while mysql worked it worked by chance and wasn't officially supported? | 16:04 |
| clarkb | certainly grepping for postgres and mysql in the repo would indicate that both seem to be supported. But also mariadb doesn't seem to care about the primary key missing? mordred do you know what may cause that? | 16:05 |
| clarkb | https://vettabase.com/blog/why-tables-need-a-primary-key-in-mariadb-and-mysql/ aha I guess it is configurable | 16:06 |
| mordred | Oh, I mean, innodb tables should ALWAYS have a primary key | 16:08 |
| mordred | Even if it's just an ignored autoinc integer | 16:09 |
| mordred | Because rows are clustered by pk | 16:09 |
| clarkb | yup the article explains you get an internal integer value that monotonically increases based on row insertion as the primary key if you leave it off | 16:09 |
| clarkb | and that is almost always going to be the wrong index for querying data | 16:10 |
| clarkb | I'll respond to that issue. I don't think this is mysql db cluster specific like lunnyseems to think | 16:10 |
| corvus | PaulFertser: istr gerrit added patchset comments a while back, but i can't figure out how to add that through the ui now... did they remove that? | 16:11 |
| PaulFertser | corvus: I'm not sure why exactly but all new comments people were making with PolyGerrit on 3.4.0 ended up being patchset comments. | 16:12 |
| corvus | opendev's gerrit (3.2) has "Add patchset description" next to the patchset selector, but gerrit's gerrit does not have that | 16:12 |
| PaulFertser | corvus: https://review.openocd.org is vanilla 3.4.0 , and supports different auth methods, you can just register and see for yourself. | 16:13 |
| PaulFertser | corvus: and I have access to its logs and settings so can clarify things. | 16:13 |
| corvus | i believe i'm talking about something different | 16:13 |
| corvus | but maybe it's the same thing? | 16:13 |
| corvus | maybe they moved the "patchset description" thing to be patchset comments? | 16:14 |
| PaulFertser | corvus: please see https://review.openocd.org/c/openocd/+/6470 , the comment is in some "patchset" area, but it was normal user adding it as usually. | 16:14 |
| fungi | they certainly don't document /PATCHSET_LEVEL as a valid file-id under 3.2, but maybe they changed the implementation | 16:14 |
| corvus | just to be clear, i'm trying to answer one specific question right now: "what is the patchset description in 3.2?" and the follow-up to that: "what happened to the patchset description in 3.4?" i don't yet know if that has anything to do with patchset_level comments | 16:16 |
| fungi | https://www.gerritcodereview.com/3.3.html#new-features "The reply dialog posts patchset level comments instead of change messages." | 16:18 |
| clarkb | "rows in tables without a primary key may appear in a different order on different nodes." database clustering is fun | 16:20 |
| fungi | looks like it could be disabled in config on 3.3 but in 3.4 it's mandated | 16:20 |
| fungi | s/mandated/no longer possible to disable/ | 16:20 |
| corvus | fungi, PaulFertser: okay, i have confirmed that patchset descriptions are distinct from patchset comments. the descriptions don't show up as comments at all, they're more like metadata for the patchset. | 16:21 |
| corvus | i don't know why the ability to add patchset descriptions seems to have been removed in gerrit master | 16:22 |
| corvus | but i thought it at least advisable to understand whether there is any potential interaction there. there does not seem to be. | 16:22 |
| PaulFertser | Makes sense, was worth checking for sure. | 16:23 |
| opendevreview | Jeremy Stanley proposed ttygroup/gertty master: Skip patchset-level comments in diff view https://review.opendev.org/c/ttygroup/gertty/+/805850 | 16:23 |
| fungi | corrected the reference to the version gerrit added that in | 16:23 |
| clarkb | https://github.com/go-gitea/gitea/milestone/97 this seems to be the url where they are tracking gitea 1.15.1 items. Probably a good idea to wait for that point release before upgrading if we can avoid having a less happy database | 16:24 |
| clarkb | https://github.com/go-gitea/gitea/issues/16668 will also be an issue for us I think since we create new repos with a single commit (the .gitreview creation iirc) | 16:25 |
| clarkb | I have WIP'd https://review.opendev.org/c/opendev/system-config/+/803231 with notes about the two issues I'ev discovered that seem important | 16:26 |
| mordred | clarkb: I don't think 16668 is the same as what we do - we don't have an empty commit, we have a single commit with a file in it | 16:29 |
| mordred | they did: git commit --allow-empty -m "Initial commit" | 16:30 |
| clarkb | oh interesting | 16:30 |
| clarkb | the issue isn't a single commit as much as an empty commit. Which makes sense if the problem is in rendering the git repo contents | 16:31 |
| mordred | yeah | 16:31 |
| mordred | I can imagine gitea being like "wat" | 16:31 |
| fungi | --allow-empty is a --yes-i-know-what-im-doing option for a good reason | 16:33 |
| clarkb | fyi sounds liek we'll be upgrading lists.kc.io at 16:00UTC Thursday assuming there isn't any last minute objection from kata | 16:35 |
| clarkb | this means I should work on gerrit account cleanups tomorrow | 16:35 |
| fungi | i'll be around to help with the upgrade. i expect we'll do it in a root screen session like we did on the test server | 16:38 |
| clarkb | ++ | 16:38 |
| clarkb | I should also write down some notes on the plan. Stop services, make a snapshot, copy process out of the testing doc to dedicated doc to remove confusion of what has or hasn't been done, etc | 16:38 |
| opendevreview | James E. Blair proposed ttygroup/gertty master: Skip patchset-level comments in diff view https://review.opendev.org/c/ttygroup/gertty/+/805850 | 16:42 |
| opendevreview | James E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer https://review.opendev.org/c/ttygroup/gertty/+/805871 | 16:42 |
| corvus | oops i may have undone fungi's change, 1 sec | 16:42 |
| opendevreview | James E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer https://review.opendev.org/c/ttygroup/gertty/+/805871 | 16:42 |
| fungi | i'll restart my gertty on that stack, though i'm not interfacing with any gerrits which have patchset level comment support yet | 16:43 |
| opendevreview | James E. Blair proposed ttygroup/gertty master: Skip patchset-level comments in diff view https://review.opendev.org/c/ttygroup/gertty/+/805850 | 16:43 |
| opendevreview | James E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer https://review.opendev.org/c/ttygroup/gertty/+/805871 | 16:43 |
| opendevreview | Merged ttygroup/gertty master: Skip patchset-level comments in diff view https://review.opendev.org/c/ttygroup/gertty/+/805850 | 16:44 |
| corvus | fungi, PaulFertser: fungi's change lgtm -- i made https://review.opendev.org/805871 as a quick followup -- does that sound like a good idea? | 16:44 |
| corvus | (and obviously, we should think about having gertty actually leave patchset level comments, and also do the whole comment resolution thing) but those are slightly more involved changes | 16:45 |
| opendevreview | Merged ttygroup/gertty master: Suggest basic auth in minimal example https://review.opendev.org/c/ttygroup/gertty/+/805859 | 16:45 |
| opendevreview | Merged ttygroup/gertty master: Suggest a 'cherry-picked from' line when cherry picking https://review.opendev.org/c/ttygroup/gertty/+/799641 | 16:46 |
| opendevreview | Merged ttygroup/gertty master: examples: match 'commit <hash>' https://review.opendev.org/c/ttygroup/gertty/+/799642 | 16:47 |
| opendevreview | Merged ttygroup/gertty master: Update author email address https://review.opendev.org/c/ttygroup/gertty/+/751887 | 16:47 |
| fungi | it seems like a good idea to me | 16:47 |
| opendevreview | Merged ttygroup/gertty master: Make commit and patchset comments nicer https://review.opendev.org/c/ttygroup/gertty/+/805871 | 16:47 |
| fungi | corvus: any chance you've noticed the lack of keyboard navigation PaulFertser mentioned for depends-on/needed-by/conflicts? | 16:47 |
| fungi | i haven't looked to see if there was already a fix proposed for that | 16:48 |
| corvus | fungi: i have, and i think it's one of those weird urwid issues that are a pita to debug; i think it arrived via an urwid upgrade a while back | 16:48 |
| fungi | i wondered if that might be it | 16:48 |
| fungi | i'll see if i can unwind urwid | 16:49 |
| corvus | thanks... i'd love a fix. in the past i've usually found playing with how items are added and focused can sometimes affect things like that. and sometimes they don't. :/ | 16:50 |
| PaulFertser | corvus: I now see in the "change" screen how odd "/PATCHSET_LEVEL" looks but tbh bare "Patchset" there would be just as confusing. | 16:50 |
| PaulFertser | I'm looking at this: https://paste.debian.net/1208883/ | 16:50 |
| fungi | seems like key navigation was working circa urwid 2.0.1 based on release dates, i wonder if 2.1.0 is what broke it (though that was almost two years ago as well) | 16:51 |
| corvus | yeah. i think the colors help a bit? | 16:51 |
| fungi | could be something in 2.1.1 or 2.1.2 i suppose but those are patch-level releases if they follow semver | 16:51 |
| fungi | i'll try to bisect | 16:51 |
| corvus | wouldn't surprise me if a .1 broke it. it's subtle. | 16:52 |
| * corvus uploaded an image: (5KiB) < https://matrix.org/_matrix/media/r0/download/acmegating.com/UnetuxljKwvXvomFcUbtoaNe/image.png > | 16:52 | |
| corvus | that's what it looks like in my terminal with colors... it approximates the gerrit webui (which isn't to say we need to mimic it in all cases, just that absent a better way of presenting the info, is a good starting point) | 16:53 |
| corvus | that's https://gerrit-review.googlesource.com/c/zuul/ops/+/316123 btw | 16:54 |
| mnaser | oh man | 16:54 |
| mnaser | i just noticed the "included in" button on gerrit changes | 16:54 |
| mnaser | 10/10 truly the most useful feature ever | 16:54 |
| PaulFertser | Yes, the meaning of "Patchset" there is totally unclear unless you're really into Gerrit internals I'm afraid. | 16:54 |
| mnaser | https://usercontent.irccloud-cdn.com/file/KiW7hQsl/image.png | 16:55 |
| corvus | PaulFertser: we could make it look like a change message instead of a comment. or we could change the text to "Patchset comment" | 16:56 |
| clarkb | mnaser: there is a ton of new functioanlity in newer gerrit that is really useful which is why we decided losing functionality that we couldn't reasonably update was worthwile | 16:56 |
| PaulFertser | corvus: is there any practical reason to discriminate between old style comments and patchset comments for the UI? | 16:57 |
| PaulFertser | So I'd say making it look like a change message would look more natural. | 16:59 |
| corvus | PaulFertser: i wonder if they're doing patchset level comments to support some sort of workflow around new patchsets and comments? aside from that, i tend to agree. | 17:01 |
| corvus | we could make them look like changes messages for now and if we need to go back to doing this, switch later | 17:01 |
| PaulFertser | Yes, it's just a UI after all. If you have all data nicely fetched and handled you can present it to the user as needed. | 17:01 |
| clarkb | this could be related to attention sets? | 17:02 |
| fungi | PaulFertser: corvus: my very un-scientific redeploying indicates that the keyboard navigation broke between urwid 2.0.1 and 2.1.0 (downgrading to urwid<2.1 fixes it for me) | 17:02 |
| fungi | clarkb: my guess was the patchset-level stuff is related to or at least working toward the "comment porting" experimental feature in 3.4 | 17:03 |
| fungi | for the urwid thing, i can try to bisect to a particular commit between 2.0.1 and 2.1.0 but not right now, late for another meeting :( | 17:03 |
| corvus | PaulFertser: it would look a little weird because it will say "(1 comment)" but then there won't be a comment. the "(1 comment)" thing is actually part of the change message. we could parse that and strip it out, but then it's getting weird. | 17:04 |
| PaulFertser | corvus: I wouldn't be surprised by 1 comment as I would think it's just a "top-level comment". | 17:05 |
| PaulFertser | I think it'd be fine either way, people get used to UIs in no time, even to quirky ones :) | 17:08 |
| corvus | especially gerrit users ;) | 17:10 |
| fungi | they get a new interface every few years! | 17:10 |
| corvus | [ i say approvingly :) ] | 17:10 |
| fungi | i still miss the old "new review screen" a little | 17:10 |
| fungi | the novelty of it hadn't completely worn off for me before polygerrit happened | 17:11 |
| corvus | fungi: the one that prompted me to write gertty? i don't :) | 17:11 |
| fungi | heh | 17:12 |
| PaulFertser | I suggest to not bother with the number of comments and just reduce the patchset comment to change message. This way it wouldn't be too annoying or surprising imho. | 17:12 |
| corvus | PaulFertser: maybe i know too much about the internals, but when i see "(1 comment)" i start looking for the comment i'm not seeing with gertty. i guess i could train myself to know that 2 comments would mean that i'm missing something.... but at least, that's the thinking i had after i made tha change like that, then saw the results, then switched it to the current implementation. | 17:14 |
| corvus | i could probably go along with that, but at least it didn't seem like a no-brainer. | 17:15 |
| PaulFertser | corvus: thank you for considering all the options, I do not mind either outcome. | 17:15 |
| PaulFertser | Probably it's better to leave Patchset for consistency with PolyGerrit and REST API docs. | 17:15 |
| PaulFertser | And since gertty is meant for developers that's a strong argument to do that. | 17:15 |
| PaulFertser | So yes, you're right and I'm wrong, 'Patchset' it is :) | 17:16 |
| corvus | PaulFertser: i think it's a close call either way. we might wake up tomorrow and change our minds. :) | 17:16 |
| PaulFertser | Thanks a lot folks for the prompt replies and investigation, you're awesome. | 17:29 |
| fungi | thanks for bringing things up, and providing useful feedback | 17:30 |
| PaulFertser | I would do it the normal way (bug tracker etc) but I'm too allergic to ubuntu and it makes me feel very uneasy. | 17:34 |
| fungi | yeah, we'd like to move to an sso aggregator we manage, but until we find people who have time to work on that we're still relying on the launchpad sso for historical reasons | 17:35 |
| fungi | https://docs.opendev.org/opendev/infra-specs/latest/specs/central-auth.html | 17:35 |
| clarkb | though we're still not planning to run auth direclty ourselves so you would have to find one that is acceptable | 17:41 |
| PaulFertser | I liked OpenID, I can't understand why it's out of fashion now. | 17:42 |
| fungi | ftr, ubuntuone sso is openid ;) | 17:43 |
| PaulFertser | OAuth that's supposed to replace it is terrible, somehow it's not nearly generic enough so every provider needs to be supported separately. | 17:43 |
| PaulFertser | Why do not you enable any OpenID then? | 17:43 |
| fungi | mainly because gerrit was a security breach waiting to happen because anyone could claim your e-mail address by running their own openid | 17:44 |
| fungi | it's improved now such that it will at least refuse to create a new account if the address is already taken, but that's very recent improvements in the openid authenticator for gerrit | 17:45 |
| PaulFertser | And what bad would happen if someone registers with my e-mail address? | 17:45 |
| fungi | also gerrit still has no mechanism for users to add an openid to an existing account, it's just initial account creation for new openids it sees | 17:45 |
| fungi | treating e-mail addresses as unique identifiers for authorization (group additions and so on) is effectively thwarted if multiple accounts can have the same e-mail address | 17:47 |
| PaulFertser | fungi: current polygerrit lacks "link identity" button but I think that's just a UI oversight, linking identities still works. But I haven't tried linking openid to openid yet, but I'd expect that to work. | 17:47 |
| clarkb | how do you link identities in if not through the UI? | 17:48 |
| clarkb | external ids cannot be added through the api (though they can be deleted) | 17:48 |
| clarkb | and you need to be an admin to see the external ids ref | 17:49 |
| fungi | i guess the suggestion is that there's a rest api method available? | 17:49 |
| clarkb | there are not. It has created major frustration in cleaning up these external id conflicts | 17:49 |
| fungi | which a user can use to add more ids | 17:49 |
| PaulFertser | clarkb: I log in as usual to PolyGerrit, then I edit the URL to read /login/?link | 17:49 |
| clarkb | If external ids were managed on a per user basis you could push to refs/users/xy/abxy to edit them like other user settings | 17:51 |
| clarkb | But they are stored in noe combined notedb ref and access to that is limited as a result | 17:51 |
| clarkb | PaulFertser: I think when gerrit sees a new id it assumes a new account | 17:52 |
| PaulFertser | Linking identities on 3.4.0 is what I tried on our OpenID+OAuth just this Sunday, and it worked. | 17:52 |
| clarkb | it doesn't associate the two together. At least this is part of the issue I'm currently cleaning up in our notedb | 17:52 |
| PaulFertser | clarkb: it does, unless you do the linking step beforehand. | 17:52 |
| clarkb | ok I guess I don't undersatnd what you mean by he linking step still. the login/ path creates a new login which creates a new account for an unknown openid ime | 17:53 |
| PaulFertser | clarkb: true unless you add ?link to the path in which case it adds anothe identity to the currently logged in user. | 17:54 |
| clarkb | aha that is what I was missing. Interesting. I wonder why they don't expose that ebtter | 17:54 |
| PaulFertser | clarkb: we'll see once my patch is reviewed | 17:54 |
| PaulFertser | I think it was just a fair mistake | 17:55 |
| PaulFertser | clarkb: see https://gerrit-review.googlesource.com/c/gerrit/+/315964 | 17:57 |
| PaulFertser | It was never exposed nicely enough I think. | 17:58 |
| PaulFertser | Also the new UI lacks SSH server keys fingerprints. | 17:58 |
| fungi | likely in large part due to the fact that upstream gerrit doesn't use/blocks access to the ssh api and wishes it would go away | 18:05 |
| clarkb | I'm not entirely sure they wish it would go away, but there is definitely a tension there | 18:05 |
| fungi | so dropping ssh server keys from the ui may be intentional, or could just be that they didn't notice since they don't use it | 18:05 |
| fungi | but in combination with us deciding to drop the sshfp records for it from our dns, that does pose an interesting conundrum | 18:07 |
| fungi | anyway, to answer the original question, the reason we started out relying on launchpad's sso is that the openstack project required contributors to agree to an individual contributor license agreement which was enforced by adding them to a launchpad group, and there was a periodic process to pre-create their accounts in gerrit via a "sync" from that group membership (back in the days before | 18:09 |
| fungi | gerrit grew a cla enforcement feature) | 18:09 |
| PaulFertser | fungi: but it's not just for the API, it's also for normal git pushing for review :) | 18:10 |
| fungi | the incompleteness of gerrit's openid auth mechanism and ability to basically break itself by allowing creation of accounts with duplicates of values it expected to be unique meant that opening authentication up to multiple id sources would likely have made things far worse | 18:11 |
| fungi | PaulFertser: you can push to gerrit over https, which is what upstream gerrit seems to prefer for some years now | 18:11 |
| fungi | s/prefer/require/ | 18:12 |
| fungi | at least insofar as the gerrit they run for their own use | 18:12 |
| PaulFertser | fungi: I know but I prefer git over ssh :) Their own instance requires https, right. | 18:12 |
| fungi | i too prefer git over ssh, fwiw. also we rely on the event stream, which still has no good non-ssh alternative (though the checks api was getting close) | 18:13 |
| PaulFertser | I think current Gerrit checks identities for duplication, at least I can't push our existing All-Users meta/external-id thing, it finds enough identical e-mail addresses and just refuses, so I'd expect same check to be performed for registration. | 18:15 |
| PaulFertser | Something I plan to clean up soon. | 18:15 |
| fungi | PaulFertser: yes, it does. we're deep into the cleanup process for that now | 18:16 |
| PaulFertser | Merging accounts manually on request with "gsql" wasn't a pleasant experience either. Notedb feels much nicer for that :) | 18:16 |
| fungi | notedb can do it if you can merge a fix for all of then in one go. if you want to merge them incrementally, you need to turn off gerrit, push the commits through the fs, start gerrit again and reindex | 18:17 |
| PaulFertser | btw, few years ago no openid providers I would like to use left so indeed I have to run my own. | 18:17 |
| PaulFertser | fungi: no, you can push, then stop gerrit, then reindex, then start. | 18:17 |
| fungi | gerrit won't let you push, if you mean push the fix through gerrit rather than locally | 18:18 |
| PaulFertser | I mean push locally of course | 18:18 |
| PaulFertser | That's what I read on their mailing list and it works. | 18:18 |
| fungi | if you mean push to the filesystem while gerrit is running, then you need to hope nobody tries to change their account info between then and when you stop the service | 18:18 |
| PaulFertser | If that happens I'd just fetch/rebase and try pushing again. | 18:19 |
| fungi | if you don't have hundreds of thousands of accounts and many hundreds of conflicts to clean up, then that may be more tractable | 18:21 |
| fungi | a lot of our challenge is likely related to scale | 18:21 |
| PaulFertser | Haha, of course I'm just tiny compared to what you have to care about. | 18:21 |
| PaulFertser | But my point is that while cleaning the contacts one-by-one you do not really need to stop gerrit. You can push opportunistically, if that fails, rebase and repush. And then even reindexing is not really needed until you feel like you cleaned up a good batch and can tolerate some downtime while it's busy doing it. | 18:22 |
| fungi | clarkb has been working through account conflicts 50-100 at a time, trying to determine which are less likely to cause impact when disabled or which ones need merging, but since a lot of it requires interacting with account holders, doing them all in one go is unfortunately not viable for us | 18:23 |
| PaulFertser | Indeed. Another option is do something automatic for all of them and then for those few real users that will complain deal with it later :) | 18:24 |
| fungi | and yeah, the goal has been to do as much as possible through the api in order to minimize need for outages | 18:24 |
| Clark[m] | So far we've stuck to the "safe" set that doesn't require interaction from the users. I plan to finish the last batch yesterday then we have ~30 that will require user involvement | 18:24 |
| PaulFertser | It's those users' fault that they have created duplicated accounts so fair to assume you can break their experience a bit. | 18:25 |
| Clark[m] | The safe set meaning we can disable one or more of the conflicting accounts and delete their openids because it is clear they aren't used or some other situation precipitated it | 18:25 |
| fungi | well, "fault" is a bit strong. in a majority of cases it was something as simple as trying to change their e-mail address, not realizing the provider was giving them a new id, which in turn created a new gerrit account | 18:25 |
| Clark[m] | Or deleting an email in Gerrit and then logging in again. That but is fixed now thankfully | 18:26 |
| fungi | heh, as of... last week? | 18:27 |
| fungi | i guess it's been a few weeks now | 18:27 |
| mordred | recently at least | 18:28 |
| opendevreview | Merged opendev/system-config master: Restrict generic inventory matchers to inventory/base https://review.opendev.org/c/opendev/system-config/+/805487 | 19:30 |
| *** dviroel|ruck is now known as dviroel|ruck|afk | 19:33 | |
| fungi | ianw: earlier today (your last night, 12:43 utc), kevinz mentioned looking into it | 20:01 |
| ianw | fungi: oh, thanks, imissed that | 20:02 |
| ianw | https://ru-repo.openeuler.org/openEuler-20.03-LTS-SP2/update/x86_64/repodata/ still seems to have no files though | 20:02 |
| ianw | fungi/clarkb: this https://opendev.org/opendev/system-config/src/branch/master/playbooks/zuul/gerrit/run.yaml#L1 is mostly what i'm thinking of | 20:20 |
| ianw | we can basically get zuul to copy whatever into the build context | 20:21 |
| ianw | what we loose is the idea the the docker/gitea/Dockerfile makes the image "by itself" ... i.e. you can really only build the image in zuul | 20:22 |
| opendevreview | Merged opendev/system-config master: borg-backup: randomise time on a per-server basis https://review.opendev.org/c/opendev/system-config/+/804916 | 20:22 |
| ianw | i guess that is already the case with gerrit, and, does it really matter. is that something we care about? | 20:22 |
| Clark[m] | ianw: I prefer if the image build is self contained as it makes doing local stuff much easier. But I do have a local bazel setup now and can build Gerrit images if I had to without zuul. It isn't the end of the world but is annoying | 20:27 |
| ianw | gerrit uses https://opendev.org/opendev/system-config/src/branch/master/playbooks/zuul/gerrit/opendevtheme.html#L59 ; i.e. from gitea static serving | 20:32 |
| ianw | it seems to be lodgeit and gerrit from https://codesearch.opendev.org/?q=https%3A%2F%2Fopendev.org%2Fimg%2F&i=nope&literal=nope&files=&excludeFiles=&repos= | 20:34 |
| mordred | ianw, Clark the reason our gerrit build is building gerrit outside of the dockerfile and then copying the artifact in rather than using a builder image ... | 20:37 |
| mordred | weird memory errors - and also bazel really likes to mutate its environment in exciting and network excessive ways | 20:38 |
| mordred | so it wound up being more repeatable, weirdly, to do bazel on the vm and then copy it in ... I tried for a LONG WHILE to have the build step be in a builder image | 20:39 |
| mordred | so - anyway - I agree with ian - we can get zuul to copy whatever into the build context because that's already how it works :) | 20:40 |
| clarkb | yup and in the case of the gerrit builds we set the build context dir externally and the build can't produce a viable image on its own | 20:49 |
| clarkb | if we're ok with those two things for other services then we're fine copying the images in that manner. | 20:49 |
| ianw | here's where i'm at -> https://etherpad.opendev.org/p/logo-locations | 20:50 |
| ianw | clarkb: urgh, those other users of the logo.svg are annoying | 20:55 |
| ianw | clarkb: so what's worse; copying in files via zuul or having to merge templates on gitea upgrades? | 20:58 |
| clarkb | good question. With the gitea upgrades situation currently we only diff the files we currently have and don't have to search for changes to other files. If we write down a "grep for logo.svg usage" step so we don't forget that is probably fine | 21:00 |
| clarkb | it would be easy to miss a new use of logo.svg that isn't overridden otherwise | 21:00 |
| ianw | have we asked if we can make logo.svg explicitly overridable? or do they not really want to make it simple to re-label? | 21:03 |
| clarkb | They recently made it less configurable. Prior to 1.14 you could write out a manifest json file thing that override those values but now they don't do that because i think they claimed it affected page load times | 21:06 |
| clarkb | I think in the old manifest json you could've set that to a url value | 21:06 |
| ianw | https://docs.gitea.io/en-us/customizing-gitea/#changing-the-logo yeah that's the offical instructions link | 21:07 |
| clarkb | What if we ran the image builds with the context dir set to the system-config root | 21:07 |
| clarkb | then we could copy the files from assets but have the build run self contained other than needing the dir overridden | 21:07 |
| clarkb | that is pretty simple to do locally | 21:07 |
| clarkb | unlike the gerrit builds we don't need to build additional artifacts | 21:08 |
| ianw | it feels a bit icky to make context the entire system-config for one dir, but maybe it's practical | 21:09 |
| clarkb | ya it seems to compromise the least from a operator overhead. We'll have to be more careful when copying things in the Dockerfile since the paths allow us to copy much more but that file tends to be quite static other than changing golang, nodejs, and gitea versions | 21:11 |
| clarkb | I need to pop out for a bit but will continue to think this over | 21:12 |
| ianw | yeah i need breakfast too :) | 21:13 |
| clarkb | I'm not coming up with anything better than shifting the build context root. | 21:51 |
| clarkb | That option definitely isn't perfect but seems to have the least bad downsides | 21:51 |
| ianw | yeah i'll put together a change and we can see | 21:52 |
| clarkb | double checking the crontab entries for some of the hosts that all failed together the other day because they ran at the asme time when networking was sad they seem to have different minute values in the crontab entry | 22:20 |
| opendevreview | Ian Wienand proposed zuul/zuul-jobs master: build-docker-image: Add flag to use BuildKit https://review.opendev.org/c/zuul/zuul-jobs/+/805911 | 22:31 |
| opendevreview | Ian Wienand proposed opendev/system-config master: [wip] test common logos https://review.opendev.org/c/opendev/system-config/+/805912 | 22:32 |
| ianw | bind mounting doesn't work ... worth a go | 22:50 |
| mordred | ianw: let me know if you ever get the buildkit bindmounting to work. I keep trying and failing but a SUPER want to figure that out and be able to use it :) | 23:12 |
| ianw | mordred: it just worked locally for me ... | 23:13 |
| mordred | ianw: NEAT! | 23:13 |
| * mordred goes to look at the dockerfile | 23:13 | |
| opendevreview | Ian Wienand proposed opendev/system-config master: [wip] test common logos https://review.opendev.org/c/opendev/system-config/+/805912 | 23:14 |
| ianw | ^ that might be horrid, i don't know. it stuffs the image files into opendevorg/assets which is based on scratch | 23:14 |
| ianw | so basically a glorified tarballs. and then it used the bind mount to pull those files into the gitea build | 23:15 |
| ianw | it worked here ... | 23:15 |
| ianw | it *seems* logical that you could say "if you want to include logo assets, use the opendevorg/assets image and copy in the file to your image from there" | 23:16 |
| clarkb | ianw: but then we have the same issue with getting the assets into opendevorg/assets? But maybe the idea there is it is ok for the assets image build to be weird and different | 23:17 |
| ianw | clarkb: assets can have it's own Dockerfile | 23:19 |
| ianw | or at least that's the theory | 23:19 |
| clarkb | oh I see adjacent | 23:21 |
| clarkb | that might be the most Docker way to solve the problem :) | 23:21 |
| ianw | i mean, instead of copying a file from two directories up, we create an encapsulated file and push it to a third party via the internet, which hosts it, and which we then download from via the internet | 23:25 |
| ianw | i.e. the docker way | 23:25 |
| clarkb | exactly :) | 23:26 |
| ianw | "the --mount option requires BuildKit" something didn't work | 23:36 |
| fungi | i like the assets container image idea. it's just another "layer" i guess | 23:47 |
| fungi | hadn't thought of that option but that's probably the most dockery | 23:47 |
| fungi | a build layer in this case i suppose? | 23:48 |
| Clark[m] | It wouldn't even need to be a layer in this case as you can copy from one image to another without creating a layer relationship. This is how the python-builder and python-base images work | 23:50 |
| opendevreview | Ian Wienand proposed zuul/zuul-jobs master: build-docker-image: Add flag to use BuildKit https://review.opendev.org/c/zuul/zuul-jobs/+/805911 | 23:51 |
| ianw | we have docker 20.10 so i have no idea why the DOCKER_BUILDKIT flag didn't seem to take | 23:56 |
| ianw | i thought we got environment variables in like https://zuul.opendev.org/t/openstack/build/5b9b909f4a474eaaaf4bb41567aed7b8/console#3/0/20/ubuntu-focal but maybe not | 23:57 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!
