Tuesday, 2021-08-24

*** yoctozepto8 is now known as yoctozepto00:33
*** yoctozepto6 is now known as yoctozepto01:21
*** yoctozepto3 is now known as yoctozepto02:12
*** yoctozepto8 is now known as yoctozepto03:43
*** yoctozepto8 is now known as yoctozepto03:57
*** ysandeep|away is now known as ysandeep04:09
*** yoctozepto0 is now known as yoctozepto04:12
*** yoctozepto1 is now known as yoctozepto04:40
*** ysandeep is now known as ysandeep|brb05:23
*** ysandeep|brb is now known as ysandeep05:35
*** yoctozepto7 is now known as yoctozepto05:43
*** yoctozepto0 is now known as yoctozepto06:24
*** redrobot0 is now known as redrobot06:29
*** yoctozepto9 is now known as yoctozepto07:00
*** rpittau|afk is now known as rpittau07:27
*** ysandeep is now known as ysandeep|lunch07:29
*** jpena|off is now known as jpena07:35
*** yoctozepto1 is now known as yoctozepto07:52
opendevreviewJing Li proposed openstack/diskimage-builder master: Add new element rocky  https://review.opendev.org/c/openstack/diskimage-builder/+/80579308:26
*** ykarel is now known as ykarel|lunch08:34
PaulFertserHello. 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 ysandeep09:35
opendevreviewJing Li proposed openstack/diskimage-builder master: Add new element rocky  https://review.opendev.org/c/openstack/diskimage-builder/+/80580009:46
opendevreviewLucas Alvares Gomes proposed openstack/project-config master: New project: OVN BGP Agent  https://review.opendev.org/c/openstack/project-config/+/80580209:57
opendevreviewLucas Alvares Gomes proposed openstack/project-config master: New project: OVN BGP Agent  https://review.opendev.org/c/openstack/project-config/+/80580210:16
*** ykarel|lunch is now known as ykarel10:23
*** ykarel_ is now known as ykarel10:56
*** ysandeep is now known as ysandeep|afk10:58
opendevreviewSlawek Kaplonski proposed openstack/project-config master: Update Neutron's Grafana dashboard  https://review.opendev.org/c/openstack/project-config/+/80580911:03
*** dviroel|out is now known as dviroel|ruck11:23
*** jpena is now known as jpena|lunch11:36
*** ysandeep|afk is now known as ysandeep12:00
PaulFertserdtantsur: 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
fungiPaulFertser: 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 well12:14
PaulFertserfungi: I hope it's ok for me to stay here to discuss and probably interactively test gertty related issues?12:15
fungii know there are people using gertty with upstream gerrit's gerrit, so i wonder if they're hitting the same issues with it12:15
fungiPaulFertser: sure, are you using gertty's current master branch state, or the 1.6.0 release from a couple years ago?12:16
fungiit's possible some fixes have merged which get it working better with new gerrit versions but haven't been tagged12:17
PaulFertserWe 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
PaulFertserfungi: current master, 1.6.0-32-gcd60a82f6ab512:18
PaulFertserBasically, 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
fungiyeah, 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 reports12:20
PaulFertserWhat 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
fungias 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 such12:23
fungiif 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
fungianyway, it has us a little gun-shy about untested upgrades as a result12:25
fungiPaulFertser: 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 raised12:27
fungithere 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 involved12:29
PaulFertserfungi: sure https://termbin.com/li5g12:30
PaulFertserfungi: that sudden replication did kinda expose sensitive information actually12:32
*** jpena|lunch is now known as jpena12:34
fungiokay, 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 comment12:34
fungithat 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 gerrit12:35
PaulFertserThe second part to be a line number, yes.12:36
PaulFertserI think that specific part is what Gertty produces on its own, separating by dashes for whatever odd reason.12:37
fungioh, yep you're right, it's key.split('-', 2) but i misread it as key.split('-')[2]12:37
funginot done with my coffee yet ;)12:37
PaulFertserCan't a normal tuple be used as a key?12:37
fungigood question, i'll read back farther to see how comment_lists comes about12:38
fungioh, yeah the BaseDiffView._init() method is building those, you're right12:39
PaulFertserI did my homework ;)12:40
fungiso it's expecting session.getRevision().comments entries to have a line attribute, which in at least some cases is None?12:43
kevinzHi clarkb: ack,  I will check12:43
fungithanks kevinz!12:43
fungiPaulFertser: is it certain diffs which crash or any diff?12:44
PaulFertserfungi: feels like any diff when a change is created after upgrade12:45
fungiguessing 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
PaulFertserAnd that filename "/PATCHSET_LEVEL" is odd, and comment is in fact a regular comment, not line comment.12:45
PaulFertserYou might be right, it's possible a file comment.12:47
fungiwell, file comments in earlier gerrit versions don't seem to crash gertty, but maybe the way they're recorded has changed12:47
PaulFertserNot sure how to tell by just looking https://review.openocd.org/c/openocd/+/647012:47
fungiso just to be clear, changes created by 2.13 viewed via 3.4 aren't crashing gertty, but changes created under 3.4 are12:48
PaulFertserIt's like a "patchset" comment12:48
PaulFertserfungi: yep12:48
fungiso this could certainly be a new comment type in 3.4 gertty just doesn't know how to parse12:49
PaulFertserMy own comments to that change as sent by gertty are marked as "resolved" right away, not sure if I did anything wrong.12:50
fungilooking 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
fungithere's also "Return empty comment context if line and range info are not available"12:53
fungiinteresting, there's an experimental feature in 3.4 to port comments to newer patchsets as well12:55
PaulFertserfungi: please see https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#file-id , it's a special filename /PATCHSET_LEVEL12:58
fungioh neat12:58
fungianyway, 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.012:59
PaulFertserThe comment is displayed nicely in the main change view so it should just be ignored for diff.13:01
fungiyeah, if gertty can identify those then it can probably safely skip them in diff view13:04
fungii guess we just match on the special filename to identify them13:05
opendevreviewShnaidman Sagi (Sergey) proposed openstack/diskimage-builder master: WIP Add building for CentOS 9  https://review.opendev.org/c/openstack/diskimage-builder/+/80584813:20
fungiaround 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
opendevreviewJeremy Stanley proposed ttygroup/gertty master: Skip patchset-level comments in diff view  https://review.opendev.org/c/ttygroup/gertty/+/80585013:26
fungiPaulFertser: see if it's as trivial as that ^ i guess?13:26
fungiwe may find there are other exceptions behind it though, so might need to discard it from the files list in diff.py as well13:27
PaulFertserfungi: 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
fungiyeah, it will need to be reviewed for sure13:30
fungiit's a naive attempt13:30
fungiand also one i haven't tested13:30
PaulFertserI'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
fungialso /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-id13:33
fungigertty seems to have special casing for /COMMIT_MSG (creating a virtual file for it), though nothing i can see for /MERGE_LIST13:34
PaulFertserShould be tested too then, but we do not have merges like that.13:34
fungii'll see if i'm able to diff a merge commit from some project in our gerrit13:35
PaulFertserThank you, that's what I implied asking for :)13:36
*** ykarel is now known as ykarel|afk13:37
PaulFertserbtw, 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
fungiPaulFertser: 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 auth13:38
fungiauth-type: basic13:39
PaulFertserfungi: yes, and that's what I had to add to the minimal config, and that was far from obvious (for me)13:39
fungiit would probably make sense for gertty to flip its default at some point, but that's corvus's call13:39
fungias it would be a non-backwards-compatible change13:40
fungithough maybe it could grow some autodetection mechanism to figure out which it needs13:40
opendevreviewSorin Sbârnea proposed opendev/elastic-recheck master: Replace alpine base image with official python one  https://review.opendev.org/c/opendev/elastic-recheck/+/80585713:46
PaulFertserfungi: a commented-out auth-type in the minimal config would have helped 13:47
fungiPaulFertser: a trivial test, but gertty is able to diff https://review.opendev.org/805856 without falling over13:47
opendevreviewSorin Sbârnea proposed opendev/elastic-recheck rdo: Ensure git review uses correct branch  https://review.opendev.org/c/opendev/elastic-recheck/+/80585813:48
PaulFertserfungi: 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 content13:54
fungiPaulFertser: 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
clarkbPaulFertser: 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
clarkbauthGroup: 'Anonymous Users' is the replication setting13:55
fungiminimal-gertty.yaml seems to intentionally not have commented-out entries13:55
PaulFertserfungi: but it has user and password, so probably not commented out auth-type: (basic|digest) could fit.13:56
PaulFertserclarkb: I meant I had issues with replication plugin itself, not an external task.13:57
clarkbPaulFertser: yes I know. replication functions as internal gerrit tasks and you can assign a privilege level using the authGroup flag13:58
clarkbif you do a gerrit show-queue you see replication tasks along with other internal task activity13:58
opendevreviewJeremy Stanley proposed ttygroup/gertty master: Suggest basic auth in minimal example  https://review.opendev.org/c/ttygroup/gertty/+/80585913:59
fungiPaulFertser: ^13:59
PaulFertserclarkb: I see what you mean, thanks.13:59
PaulFertserfungi: thank you :)14:00
corvusPaulFertser, fungi : just acking that i've read the conversation; thanks' for that.  i'll look into that later today14:25
fungithanks corvus!14:27
*** ysandeep is now known as ysandeep|away14:30
*** ykarel|afk is now known as ykarel14:56
opendevreviewMerged opendev/irc-meetings master: Update irc channel for security sig meetings  https://review.opendev.org/c/opendev/irc-meetings/+/80509315:08
PaulFertsercorvus: thank you! I'll continue lurking here in case you need me.15:12
PaulFertserAuto-detection auth type should be possible like in this answer: https://stackoverflow.com/a/1942724215:18
*** jpena is now known as jpena|off15:33
PaulFertserHm, 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|afk16:00
fungiPaulFertser: i'd noticed the same recently, but haven't found time to dig into the cause16:00
clarkbhttps://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
clarkbcertainly 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
clarkbhttps://vettabase.com/blog/why-tables-need-a-primary-key-in-mariadb-and-mysql/ aha I guess it is configurable16:06
mordredOh, I mean, innodb tables should ALWAYS have a primary key16:08
mordredEven if it's just an ignored autoinc integer16:09
mordredBecause rows are clustered by pk16:09
clarkbyup the article explains you get an internal integer value that monotonically increases based on row insertion as the primary key if you leave it off16:09
clarkband that is almost always going to be the wrong index for querying data16:10
clarkbI'll respond to that issue. I don't think this is mysql db cluster specific like lunnyseems to think16:10
corvusPaulFertser: 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
PaulFertsercorvus: 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
corvusopendev's gerrit (3.2) has "Add patchset description" next to the patchset selector, but gerrit's gerrit does not have that16:12
PaulFertsercorvus: https://review.openocd.org is vanilla 3.4.0 , and supports different auth methods, you can just register and see for yourself.16:13
PaulFertsercorvus: and I have access to its logs and settings so can clarify things.16:13
corvusi believe i'm talking about something different16:13
corvusbut maybe it's the same thing?16:13
corvusmaybe they moved the "patchset description" thing to be patchset comments?16:14
PaulFertsercorvus: 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
fungithey certainly don't document /PATCHSET_LEVEL as a valid file-id under 3.2, but maybe they changed the implementation16:14
corvusjust 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 comments16:16
fungihttps://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 fun16:20
fungilooks like it could be disabled in config on 3.3 but in 3.4 it's mandated16:20
fungis/mandated/no longer possible to disable/16:20
corvusfungi, 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
corvusi don't know why the ability to add patchset descriptions seems to have been removed in gerrit master16:22
corvusbut i thought it at least advisable to understand whether there is any potential interaction there.  there does not seem to be.16:22
PaulFertserMakes sense, was worth checking for sure.16:23
opendevreviewJeremy Stanley proposed ttygroup/gertty master: Skip patchset-level comments in diff view  https://review.opendev.org/c/ttygroup/gertty/+/80585016:23
fungicorrected the reference to the version gerrit added that in16:23
clarkbhttps://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 database16:24
clarkbhttps://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
clarkbI have WIP'd https://review.opendev.org/c/opendev/system-config/+/803231 with notes about the two issues I'ev discovered that seem important16:26
mordredclarkb: 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 it16:29
mordredthey did: git commit --allow-empty -m "Initial commit"16:30
clarkboh interesting16:30
clarkbthe issue isn't a single commit as much as an empty commit. Which makes sense if the problem is in rendering the git repo contents16:31
mordredI can imagine gitea being like "wat"16:31
fungi--allow-empty is a --yes-i-know-what-im-doing option for a good reason16:33
clarkbfyi sounds liek we'll be upgrading lists.kc.io at 16:00UTC Thursday assuming there isn't any last minute objection from kata16:35
clarkbthis means I should work on gerrit account cleanups tomorrow16:35
fungii'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 server16:38
clarkbI 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, etc16:38
opendevreviewJames E. Blair proposed ttygroup/gertty master: Skip patchset-level comments in diff view  https://review.opendev.org/c/ttygroup/gertty/+/80585016:42
opendevreviewJames E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer  https://review.opendev.org/c/ttygroup/gertty/+/80587116:42
corvusoops i may have undone fungi's change, 1 sec16:42
opendevreviewJames E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer  https://review.opendev.org/c/ttygroup/gertty/+/80587116:42
fungii'll restart my gertty on that stack, though i'm not interfacing with any gerrits which have patchset level comment support yet16:43
opendevreviewJames E. Blair proposed ttygroup/gertty master: Skip patchset-level comments in diff view  https://review.opendev.org/c/ttygroup/gertty/+/80585016:43
opendevreviewJames E. Blair proposed ttygroup/gertty master: Make commit and patchset comments nicer  https://review.opendev.org/c/ttygroup/gertty/+/80587116:43
opendevreviewMerged ttygroup/gertty master: Skip patchset-level comments in diff view  https://review.opendev.org/c/ttygroup/gertty/+/80585016:44
corvusfungi, 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 changes16:45
opendevreviewMerged ttygroup/gertty master: Suggest basic auth in minimal example  https://review.opendev.org/c/ttygroup/gertty/+/80585916:45
opendevreviewMerged ttygroup/gertty master: Suggest a 'cherry-picked from' line when cherry picking  https://review.opendev.org/c/ttygroup/gertty/+/79964116:46
opendevreviewMerged ttygroup/gertty master: examples: match 'commit <hash>'  https://review.opendev.org/c/ttygroup/gertty/+/79964216:47
opendevreviewMerged ttygroup/gertty master: Update author email address  https://review.opendev.org/c/ttygroup/gertty/+/75188716:47
fungiit seems like a good idea to me16:47
opendevreviewMerged ttygroup/gertty master: Make commit and patchset comments nicer  https://review.opendev.org/c/ttygroup/gertty/+/80587116:47
fungicorvus: any chance you've noticed the lack of keyboard navigation PaulFertser mentioned for depends-on/needed-by/conflicts?16:47
fungii haven't looked to see if there was already a fix proposed for that16:48
corvusfungi: 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 back16:48
fungii wondered if that might be it16:48
fungii'll see if i can unwind urwid16:49
corvusthanks... 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
PaulFertsercorvus: I now see in the "change" screen how odd "/PATCHSET_LEVEL" looks but tbh bare "Patchset" there would be just as confusing.16:50
PaulFertserI'm looking at this: https://paste.debian.net/1208883/16:50
fungiseems 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
corvusyeah.  i think the colors help a bit?16:51
fungicould be something in 2.1.1 or 2.1.2 i suppose but those are patch-level releases if they follow semver16:51
fungii'll try to bisect16:51
corvuswouldn'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
corvusthat'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
corvusthat's https://gerrit-review.googlesource.com/c/zuul/ops/+/316123 btw16:54
mnaseroh man16:54
mnaseri just noticed the "included in" button on gerrit changes16:54
mnaser10/10 truly the most useful feature ever16:54
PaulFertserYes, the meaning of "Patchset" there is totally unclear unless you're really into Gerrit internals I'm afraid.16:54
corvusPaulFertser: we could make it look like a change message instead of a comment.  or we could change the text to "Patchset comment"16:56
clarkbmnaser: 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 worthwile16:56
PaulFertsercorvus: is there any practical reason to discriminate between old style comments and patchset comments for the UI?16:57
PaulFertserSo I'd say making it look like a change message would look more natural.16:59
corvusPaulFertser: 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
corvuswe could make them look like changes messages for now and if we need to go back to doing this, switch later17:01
PaulFertserYes, 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
clarkbthis could be related to attention sets?17:02
fungiPaulFertser: 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
fungiclarkb: my guess was the patchset-level stuff is related to or at least working toward the "comment porting" experimental feature in 3.417:03
fungifor 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
corvusPaulFertser: 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
PaulFertsercorvus: I wouldn't be surprised by 1 comment as I would think it's just a "top-level comment".17:05
PaulFertserI think it'd be fine either way, people get used to UIs in no time, even to quirky ones :)17:08
corvusespecially gerrit users ;)17:10
fungithey get a new interface every few years!17:10
corvus[ i say approvingly :) ]17:10
fungii still miss the old "new review screen" a little17:10
fungithe novelty of it hadn't completely worn off for me before polygerrit happened17:11
corvusfungi: the one that prompted me to write gertty?  i don't  :)17:11
PaulFertserI 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
corvusPaulFertser: 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
corvusi could probably go along with that, but at least it didn't seem like a no-brainer.17:15
PaulFertsercorvus: thank you for considering all the options, I do not mind either outcome.17:15
PaulFertserProbably it's better to leave Patchset for consistency with PolyGerrit and REST API docs.17:15
PaulFertserAnd since gertty is meant for developers that's a strong argument to do that.17:15
PaulFertserSo yes, you're right and I'm wrong, 'Patchset' it is :)17:16
corvusPaulFertser: i think it's a close call either way.  we might wake up tomorrow and change our minds.  :)17:16
PaulFertserThanks a lot folks for the prompt replies and investigation, you're awesome.17:29
fungithanks for bringing things up, and providing useful feedback17:30
PaulFertserI 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
fungiyeah, 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 reasons17:35
clarkbthough we're still not planning to run auth direclty ourselves so you would have to find one that is acceptable17:41
PaulFertserI liked OpenID, I can't understand why it's out of fashion now.17:42
fungiftr, ubuntuone sso is openid ;)17:43
PaulFertserOAuth 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
PaulFertserWhy do not you enable any OpenID then?17:43
fungimainly because gerrit was a security breach waiting to happen because anyone could claim your e-mail address by running their own openid17:44
fungiit'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 gerrit17:45
PaulFertserAnd what bad would happen if someone registers with my e-mail address?17:45
fungialso 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 sees17:45
fungitreating 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 address17:47
PaulFertserfungi: 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
clarkbhow do you link identities in if not through the UI?17:48
clarkbexternal ids cannot be added through the api (though they can be deleted)17:48
clarkband you need to be an admin to see the external ids ref17:49
fungii guess the suggestion is that there's a rest api method available?17:49
clarkbthere are not. It has created major frustration in cleaning up these external id conflicts17:49
fungiwhich a user can use to add more ids17:49
PaulFertserclarkb: I log in as usual to PolyGerrit, then I edit the URL to read /login/?link  17:49
clarkbIf external ids were managed on a per user basis you could push to refs/users/xy/abxy to edit them like other user settings17:51
clarkbBut they are stored in noe combined notedb ref and access to that is limited as a result17:51
clarkbPaulFertser: I think when gerrit sees a new id it assumes a new account17:52
PaulFertserLinking identities on 3.4.0 is what I tried on our OpenID+OAuth just this Sunday, and it worked.17:52
clarkbit doesn't associate the two together. At least this is part of the issue I'm currently cleaning up in our notedb17:52
PaulFertserclarkb: it does, unless you do the linking step beforehand.17:52
clarkbok 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 ime17:53
PaulFertserclarkb: true unless you add ?link to the path in which case it adds anothe identity to the currently logged in user.17:54
clarkbaha that is what I was missing. Interesting. I wonder why they don't expose that ebtter17:54
PaulFertserclarkb: we'll see once my patch is reviewed17:54
PaulFertserI think it was just a fair mistake17:55
PaulFertserclarkb: see https://gerrit-review.googlesource.com/c/gerrit/+/31596417:57
PaulFertserIt was never exposed nicely enough I think.17:58
PaulFertserAlso the new UI lacks SSH server keys fingerprints.17:58
fungilikely in large part due to the fact that upstream gerrit doesn't use/blocks access to the ssh api and wishes it would go away18:05
clarkbI'm not entirely sure they wish it would go away, but there is definitely a tension there18:05
fungiso dropping ssh server keys from the ui may be intentional, or could just be that they didn't notice since they don't use it18:05
fungibut in combination with us deciding to drop the sshfp records for it from our dns, that does pose an interesting conundrum18:07
fungianyway, 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 before18:09
fungigerrit grew a cla enforcement feature)18:09
PaulFertserfungi: but it's not just for the API, it's also for normal git pushing for review :)18:10
fungithe 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 worse18:11
fungiPaulFertser: you can push to gerrit over https, which is what upstream gerrit seems to prefer for some years now18:11
fungiat least insofar as the gerrit they run for their own use18:12
PaulFertserfungi: I know but I prefer git over ssh :) Their own instance requires https, right.18:12
fungii 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
PaulFertserI 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
PaulFertserSomething I plan to clean up soon.18:15
fungiPaulFertser: yes, it does. we're deep into the cleanup process for that now18:16
PaulFertserMerging accounts manually on request with "gsql" wasn't a pleasant experience either. Notedb feels much nicer for that :)18:16
funginotedb 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 reindex18:17
PaulFertserbtw, few years ago no openid providers I would like to use left so indeed I have to run my own.18:17
PaulFertserfungi: no, you can push, then stop gerrit, then reindex, then start.18:17
fungigerrit won't let you push, if you mean push the fix through gerrit rather than locally18:18
PaulFertserI mean push locally of course18:18
PaulFertserThat's what I read on their mailing list and it works.18:18
fungiif 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 service18:18
PaulFertserIf that happens I'd just fetch/rebase and try pushing again.18:19
fungiif you don't have hundreds of thousands of accounts and many hundreds of conflicts to clean up, then that may be more tractable18:21
fungia lot of our challenge is likely related to scale18:21
PaulFertserHaha, of course I'm just tiny compared to what you have to care about.18:21
PaulFertserBut 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
fungiclarkb 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 us18:23
PaulFertserIndeed. 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
fungiand yeah, the goal has been to do as much as possible through the api in order to minimize need for outages18: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 involvement18:24
PaulFertserIt'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 it18:25
fungiwell, "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 account18:25
Clark[m]Or deleting an email in Gerrit and then logging in again. That but is fixed now thankfully18:26
fungiheh, as of... last week?18:27
fungii guess it's been a few weeks now18:27
mordredrecently at least18:28
opendevreviewMerged opendev/system-config master: Restrict generic inventory matchers to inventory/base  https://review.opendev.org/c/opendev/system-config/+/80548719:30
*** dviroel|ruck is now known as dviroel|ruck|afk19:33
fungiianw: earlier today (your last night, 12:43 utc), kevinz mentioned looking into it20:01
ianwfungi: oh, thanks, imissed that20:02
ianwhttps://ru-repo.openeuler.org/openEuler-20.03-LTS-SP2/update/x86_64/repodata/ still seems to have no files though20:02
ianwfungi/clarkb: this https://opendev.org/opendev/system-config/src/branch/master/playbooks/zuul/gerrit/run.yaml#L1 is mostly what i'm thinking of20:20
ianwwe can basically get zuul to copy whatever into the build context 20:21
ianwwhat 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 zuul20:22
opendevreviewMerged opendev/system-config master: borg-backup: randomise time on a per-server basis  https://review.opendev.org/c/opendev/system-config/+/80491620:22
ianwi 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 annoying20:27
ianwgerrit uses https://opendev.org/opendev/system-config/src/branch/master/playbooks/zuul/gerrit/opendevtheme.html#L59 ; i.e. from gitea static serving20:32
ianwit 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
mordredianw, 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
mordredweird memory errors - and also bazel really likes to mutate its environment in exciting and network excessive ways20:38
mordredso 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 image20:39
mordredso - 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
clarkbyup 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 own20:49
clarkbif we're ok with those two things for other services then we're fine copying the images in that manner.20:49
ianwhere's where i'm at -> https://etherpad.opendev.org/p/logo-locations20:50
ianwclarkb: urgh, those other users of the logo.svg are annoying20:55
ianwclarkb: so what's worse; copying in files via zuul or having to merge templates on gitea upgrades?20:58
clarkbgood 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 fine21:00
clarkbit would be easy to miss a new use of logo.svg that isn't overridden otherwise21:00
ianwhave 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
clarkbThey 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 times21:06
clarkbI think in the old manifest json you could've set that to a url value21:06
ianwhttps://docs.gitea.io/en-us/customizing-gitea/#changing-the-logo yeah that's the offical instructions link21:07
clarkbWhat if we ran the image builds with the context dir set to the system-config root21:07
clarkbthen we could copy the files from assets but have the build run self contained other than needing the dir overridden21:07
clarkbthat is pretty simple to do locally21:07
clarkbunlike the gerrit builds we don't need to build additional artifacts21:08
ianwit feels a bit icky to make context the entire system-config for one dir, but maybe it's practical21:09
clarkbya 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 versions21:11
clarkbI need to pop out for a bit but will continue to think this over21:12
ianwyeah i need breakfast too :)  21:13
clarkbI'm not coming up with anything better than shifting the build context root.21:51
clarkbThat option definitely isn't perfect but seems to have the least bad downsides21:51
ianwyeah i'll put together a change and we can see 21:52
clarkbdouble 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 entry22:20
opendevreviewIan Wienand proposed zuul/zuul-jobs master: build-docker-image: Add flag to use BuildKit  https://review.opendev.org/c/zuul/zuul-jobs/+/80591122:31
opendevreviewIan Wienand proposed opendev/system-config master: [wip] test common logos  https://review.opendev.org/c/opendev/system-config/+/80591222:32
ianwbind mounting doesn't work ... worth a go22:50
mordredianw: 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
ianwmordred: it just worked locally for me ...23:13
mordredianw: NEAT!23:13
* mordred goes to look at the dockerfile23:13
opendevreviewIan Wienand proposed opendev/system-config master: [wip] test common logos  https://review.opendev.org/c/opendev/system-config/+/80591223:14
ianw^ that might be horrid, i don't know.  it stuffs the image files into opendevorg/assets which is based on scratch23:14
ianwso basically a glorified tarballs.  and then it used the bind mount to pull those files into the gitea build23:15
ianwit worked here ...23:15
ianwit *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
clarkbianw: 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 different23:17
ianwclarkb: assets can have it's own Dockerfile 23:19
ianwor at least that's the theory23:19
clarkboh I see adjacent23:21
clarkbthat might be the most Docker way to solve the problem :)23:21
ianwi 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 internet23:25
ianwi.e. the docker way23:25
clarkbexactly :)23:26
ianw"the --mount option requires BuildKit" something didn't work23:36
fungii like the assets container image idea. it's just another "layer" i guess23:47
fungihadn't thought of that option but that's probably the most dockery23:47
fungia 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 work23:50
opendevreviewIan Wienand proposed zuul/zuul-jobs master: build-docker-image: Add flag to use BuildKit  https://review.opendev.org/c/zuul/zuul-jobs/+/80591123:51
ianwwe have docker 20.10 so i have no idea why the DOCKER_BUILDKIT flag didn't seem to take23:56
ianwi thought we got environment variables in like https://zuul.opendev.org/t/openstack/build/5b9b909f4a474eaaaf4bb41567aed7b8/console#3/0/20/ubuntu-focal but maybe not23:57

Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!