| *** diablo_rojo is now known as Guest2968 | 03:27 | |
| opendevreview | Elod Illes proposed openstack/openstack-manuals master: [www] Set Zed state to Unmaintained https://review.opendev.org/c/openstack/openstack-manuals/+/918035 | 10:40 |
|---|---|---|
| spotz[m] | I'll be at RH Summit next week, I'll be checking email more then Matrix | 15:51 |
| opendevreview | Ghanshyam proposed openstack/governance master: Remove ref of bylaw Appendix 4 https://review.opendev.org/c/openstack/governance/+/916942 | 19:10 |
| gmann | gouthamr: I think these two are ready to merge, can you please check https://review.opendev.org/c/openstack/governance/+/916822 https://review.opendev.org/c/openstack/governance/+/908880 | 19:12 |
| gmann | it will reduce my tracking on open reviews | 19:12 |
| gouthamr | gmann: ah ack | 19:12 |
| opendevreview | Merged openstack/openstack-manuals master: [www] Set Zed state to Unmaintained https://review.opendev.org/c/openstack/openstack-manuals/+/918035 | 19:27 |
| opendevreview | Merged openstack/governance master: Move distributed-project-leadership model into doc https://review.opendev.org/c/openstack/governance/+/916822 | 19:38 |
| opendevreview | Merged openstack/governance master: Clarify the timeline for project to move to Inactive state https://review.opendev.org/c/openstack/governance/+/908880 | 19:38 |
| gouthamr | i'm confused about zuul's update on this patch: https://review.opendev.org/c/openstack/governance/+/916822 | 20:45 |
| gouthamr | haven't seen that sort of comment before.. "Change has been successfully merged. 1 is the latest approved patch-set. The change was submitted with unreviewed change" | 20:46 |
| spotz[m] | I 'think' because frickler and gmann had comments that weren't marked acked or done | 20:46 |
| gouthamr | that would be an unresolved comment though? | 20:47 |
| spotz[m] | https://review.opendev.org/c/openstack/governance/+/916822/1..2/reference/index.rst | 21:14 |
| jrosser_ | you can tell which files have been looked at and which not | 21:16 |
| jrosser_ | reference/index.txt has "reviewed" at the end of the row, the other file does not | 21:16 |
| jrosser_ | though i do not know if this is per user (as i have clicked spotz[m] link), or in agreggate | 21:18 |
| clarkb | the reviewed flag for files is maintained for each user so it shows you what you have reviewed | 21:19 |
| clarkb | its there to help you keep track of what you need to be looking at which is useful in larger changes | 21:19 |
| * gouthamr is still confused.. does this mean that I didn't review reference/index.rst ? and i pressed the W+1 button? | 21:27 | |
| clarkb | I'm not sure. That message is actually coming from Gerrit | 21:29 |
| gouthamr | clarkb: yes; the repo looks fine, so i'll ignore it :) | 21:32 |
| clarkb | reading the gerrit code this was a CLEAN_MERGE which falls through some code which checks if latestApprovedPatchsetId.get() == currentPatchset.id().get() and when it doesn't it lists the files that were modified between the two patchsets | 21:42 |
| clarkb | in this case the file diff it posted in the comment is the delta between patchset 1 and patchset 2. It is saying patchset 1 is the one that was approved and that was carried over. But I'm not quite sure I understand that yet | 21:42 |
| clarkb | to determine the latest approved patchset it only looks at non copied code review values apparently: for (PatchSetApproval patchSetApproval : notes.getApprovals().onlyNonCopied().values()) { if (!patchSetApproval.label().equals(LabelId.CODE_REVIEW)) { continue; } | 21:49 |
| clarkb | I suspect that this is a side effect of making code review a non submit requirement which breaks the max approval patchset checking | 21:52 |
| clarkb | yup I get it now | 21:53 |
| clarkb | code review's max value in openstack/project-config/gerrit/acls/openstack/governance.config is +2. And this loop is basically saying look at every approval vote, skip any that are not code-review (so we only look at code review) then check which patchset had the max code review value set. In this case no patchset had the max code review value set ebacuse the highest value was +1 | 21:54 |
| clarkb | not +2. But that loop starts by initializating the maxApprovedPatchsetId to 1 | 21:54 |
| clarkb | since 1 < 2 we then trip into "hey you didn't review the things in the delta" | 21:54 |
| clarkb | gouthamr: long story short this is probably a bug in gerrit. They shouldn't treat code-review as a special case. However you can work around it by udpating project-config's governance.config to stop making +2 a valid code-review vote value | 21:55 |
| clarkb | if +1 were the max value (which appears to be what people can and do vote) then it would see that several people +1'd patchset 2 and you wouldn't get that mesasge. I'll link to the code momentarily | 21:56 |
| clarkb | https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.8.5/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java#296 called via https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.8.5/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java#140 | 21:57 |
| gouthamr | ah; the value of code review is not used for approving/merging... https://opendev.org/openstack/project-config/src/commit/f0cde8e841746c428b3613160c8eb5a8d7f7e4ca/gerrit/acls/openstack/governance.config#L9-L13 | 22:01 |
| clarkb | yes, but in the first link I gave they basically treat code-review as special and something to look for to make sure people code reviewed; | 22:02 |
| clarkb | This may be a bug in gerrit or they may respond and say "you should always use code review its the base level of review" or whatever | 22:02 |
| clarkb | and people did code review so that isn't the triggering issue. The triggering issue is that no one voted code-review +2 which is the max value of code review on that repo | 22:03 |
| clarkb | if you changed the max value to +1 (since no one seems to +2 anyway) then this check would work properly | 22:03 |
| gouthamr | ack; i can do that.. | 22:04 |
| gouthamr | thanks for revealing this mystery clarkb :) | 22:04 |
| clarkb | fwiw I can reach out upstream if we feel strongly about it being a bug. However, since we're already voting +1 on code review I think we're following the idea that every change should have a code review vote | 22:04 |
| clarkb | and we just have to align the possible values with the practice in the field | 22:05 |
| gouthamr | yeah | 22:05 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!