I actually see people all the time making a final check before merge as part of the review. And I personally see it only as a benefit when it comes to serious things like Accord, as an example. Even as a help for the author who is overwhelmed with the big amount of work already done - someone to do quick last round of review. Team work after all.
Easy rebase - those are great news. I guess any merge conflicts that were solved will be documented and confirmed with reviewers before merge on the ticket where the final CI push will be posted. I also assumed that even without direct conflicts a check that there is no contradiction with any post-September commits is done as part of the rebase. (Just adding here for completeness) One thing that I wanted to ask for is when you push to CI, you or whoever does it, to approve all jobs. Currently we have pre-approved the minimum required jobs in the pre-commit workflow. I think in this case with a big work approving all jobs in CircleCI will be of benefit. (I also do it for bigger bodies of work to be on the safe side) Just pointing in case you use a script or something to push only the pre-approved ones. Please ping me in Slack if It’s not clear what I mean, happy to help with that On Tue, 24 Jan 2023 at 11:52, Benedict <bened...@apache.org> wrote: > Perhaps the disconnect is that folk assume a rebase will be difficult and > have many conflicts? > > We have introduced primarily new code with minimal integration points, so > I decided to test this. I managed to rebase locally in around five minutes; > mostly imports. This is less work than for a rebase of fairly typical > ticket of average complexity. > > Green CI is of course a requirement. There is, however, no good procedural > or technical justification for a special review of the rebase. > > Mick is encouraged to take a look at the code before and after rebase, and > will be afforded plenty of time to do so. But I will not gate merge on this > adhoc requirement. > > > > On 24 Jan 2023, at 15:40, Ekaterina Dimitrova <e.dimitr...@gmail.com> > wrote: > > > > Hi everyone, > I am excited to see this work merged. I noticed the branch is 395 commits > behind trunk or not rebased since September last year. I think if Mick or > anyone else wants to make a final pass after rebase happens and CI runs - > this work can only benefit of that. Squash, rebase and full CI run green is > the minimum that, if I read correctly the thread, we all agree on that > part. > I would say that CI and final check after a long rebase of a patch is a > thing we actually do all the time even for small patches when we get back > to our backlog of old patches. This doesn’t mean that the previous reviews > are dismissed or people not trusted or anything like that. > But considering the size and the importance of this work, I can really see > only benefit of a final cross-check. > As Henrik mentioned me, I am not sure I will have the chance to review > this work any time soon (just setting the right expectations up front) but > I see at least Mick already mentioning he would do it if there are no other > volunteers. Now, whether it will be separate ticket or not, that is a > different story. Aren’t the Accord tickets in an epic under which we can > document the final rebase, CI runs, etc? > > On Tue, 24 Jan 2023 at 9:40, Henrik Ingo <henrik.i...@datastax.com> wrote: > >> When was the last time the feature branch was rebased? Assuming it's a >> while back and the delta is significant, surely it's normal process to >> first rebase, run tests, and then present the branch for review? >> >> To answer your question: The effect of the rebase is then either baked >> into the original commits (which I personally dislike), or you can also >> have the rebase-induced changes as their own commits. (Which can get >> tedious, but has the benefit of making explicit what was only a change due >> to rebasing.) Depending on which approach you take when rebasing, a >> reviewer would then review accordingly. >> >> henrik >> >> On Tue, Jan 24, 2023 at 11:14 AM Benedict <bened...@apache.org> wrote: >> >>> No, that is not the normal process. What is it you think you would be >>> reviewing? There are no diffs produced as part of rebasing, and the purpose >>> of review is to ensure code meets out standards, not that the committer is >>> competent at rebasing or squashing. Nor are you familiar with the code as >>> it was originally reviewed, so would have nothing to compare against. We >>> expect a clean CI run, ordinarily, not an additional round of review. If we >>> were to expect that, it would be by the original reviewer, not a third >>> party, as they are the only ones able to judge the rebase efficiently. >>> >>> I would support investigating tooling to support reviewing rebases. I’m >>> sure such tools and processes exist. But, we don’t have them today and it >>> is not a normal part of the review process. If you want to modify, clarify >>> or otherwise stipulate new standards or processes, I suggest a separate >>> thread. >>> >>> > How will the existing tickets make it clear when and where their >>> final merge happened? >>> >>> By setting the release version and source control fields. >>> >>> >>> >>> On 24 Jan 2023, at 08:43, Mick Semb Wever <m...@apache.org> wrote: >>> >>> >>> >>> .... But it's not merge-than-review, because they've already been >>>> reviewed, before being merged to the feature branch, by committers >>>> (actually PMC members)? >>>> >>>> You want code that's been written by one PMC member and reviewed by 2 >>>> other PMC members to be put up for review by some random 4th party? For how >>>> long? >>>> >>> >>> >>> It is my hope that the work as-is is not being merged. That there is a >>> rebase and some trivial squashing to do. That deserves a quick check by >>> another. Ideally this would be one of the existing reviewers (but like any >>> other review step, no matter how short and trivial it is, that's still an >>> open process). I see others already doing this when rebasing larger patches >>> before the final merge. >>> >>> Will the branch be rebased and cleaned up? >>> How will the existing tickets make it clear when and where their final >>> merge happened? >>> >>> >>> >> >> -- >> >> Henrik Ingo >> >> c. +358 40 569 7354 >> >> w. www.datastax.com >> >> <https://www.facebook.com/datastax> <https://twitter.com/datastax> >> <https://www.linkedin.com/company/datastax/> >> <https://github.com/datastax/> >> >>
