Just FYI, I'm going to be posting a Jira (which will have some dependencies as well) to track this merge, hopefully some time today...
On Tue, Jan 24, 2023 at 12:26 PM Ekaterina Dimitrova <e.dimitr...@gmail.com> wrote: > 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/> >>> >>>
