I think the big question, which you brought up, is "what constitutes a review?" If another committer looking over the code and signing off is sufficient then I'm not horribly opposed, though I think it will VERY MUCH impede the speed at which any new features / bug fixes can be accomplished.
If we are talking about waiting for several people to vote, then I think it's an absolute non-starter and destroys any hope of moving even at a fraction of the rate we are used to. This is especially concerning if we are waiting for a consensus for something like a trivial bug fix like an NPE. Thanks -Mark > On Dec 10, 2014, at 9:20 PM, Joe Witt <[email protected]> wrote: > > NiFi grew up in a CT[R] environment where the review was very rare and so > really we ended up with CTDTFSOR - Commit then deploy then feel sense of > regret. > > So the bad side: We ended up with a lot of avoidable defects where by > avoidable I mean they could have likely been detected in a decent review > effort. > > The good side: We were fast. Very fast. New features got out quick and if > something was funky we fixed it very fast. For the vast majority of > mistakes the problem was highly isolated (thanks to our base design > construct of FBP) and rarely caused extremely stressful days. > > On balance though and given the benefits it has to community growth I > personally am very supportive of us adopting RTC immediately provided we > can sort out some key questions: > > - What constitutes a valid review? > This page http://www.apache.org/foundation/glossary.html#ReviewThenCommit > suggests that a review is a consensus approval. I am very concerned by > that definition if we're talking every commit means we need a vote. For > the vast majority of commits this just seems to onerous and too time > consuming and frankly to me takes some of the fun out of developing. If > for us a review is simply that a 'committer' has reviewed the code then I > am perfectly happy with this and I am thinking this is in-line with the > model Benson described for his dayjob "'make branch, submit pr, get review, > merge'" > > - What is the best tooling/process around this to actually conduct/document > the review? > I think > https://cwiki.apache.org/confluence/display/KAFKA/Patch+submission+and+review#Patchsubmissionandreview-Simplecontributorworkflow > does a good job of describing the mechanics of a review/submission > process. And I'd just assume a patch is as good as a pull request. I > think even as a good starting point having the JIRA ticket commented on by > the reviewer would be good. > > - What are some key things a reviewer should be checking for and helping to > enforce? What are some out of bounds things? > > > > This seems like a really important discussion and something that we should > center on sooner than later. There are ways to nuance this such as RTC for > the core and CTR for extensions and so on but I think that just leads to > more confusion. We should likely pick our poison. I see strong benefits in > both directions and so I can deal either way personally. I see a couple > strong arguments here in this thread already for RTC . > > Anyone willing to argue for CTR? > > Thanks > Joe > > > On Wed, Dec 10, 2014 at 8:21 PM, Benson Margulies <[email protected]> > wrote: > >> It seems to me, and this is purely opinion, that RTC has a lot to be said >> for a new project, in terms of getting more people involved, more eyeballs, >> and more of a community feeling. My dayjob workflow is 'make branch, submit >> pr, get review, merge' and that seems pretty applicable. However, I am not >> wearing any magic mentor hat-o-authority. >> >>> On Wed, Dec 10, 2014 at 2:23 PM, Billie Rinaldi <[email protected]> wrote: >>> >>>> On Tue, Dec 9, 2014 at 8:07 AM, Joe Witt <[email protected]> wrote: >>>> >>>> Benson - thanks for the headsup on the maven plugin. Seems like using >>> them >>>> to their fullest capability and then manually merging to master is >>>> perfectly fine to me. >>>> >>>> Billie >>>> As for CTR i don't think i have a good enough appreciation for the >>>> process/value proposition here. Curious of other folks views. >>> >>> Both CTR and RTC have their own advantages, so either would be fine ( >>> http://www.apache.org/foundation/glossary.html#CommitThenReview). In >>> retrospect I see my question could be viewed as leaning towards CTR, but >> it >>> was just based on an observation that people already seemed to be making >>> commits without review. >>> >>> >>>> >>>> It seems reasonable to keep the feature branch relaxed and in that >> sense >>>> what Gilman did today seems fine. We can get bettter at those >> judgement >>>> calls and documenting the criteria as we go along. >>>> >>>> Thanks >>>> Joe >>>> >>>> >>>> >>>> On Tue, Dec 9, 2014 at 11:00 AM, Benson Margulies < >> [email protected] >>>> >>>> wrote: >>>> >>>>> PR's are certainly convenient. There's no much difference, for a >>>> committer, >>>>> between pushing a branch to the official repo and pushing a branch to >>>> some >>>>> personal repo on github. The same integration workflow can be used >>> either >>>>> way to close out the PR upon merge. >>>>> >>>>> However, in a CTR project, it seems perhaps excessive to _require_ >>>> feature >>>>> branches for small fixes as opposed to just committing them directly >> to >>>>> develop. At day job we do mostly do branch-per-jira, but some people >>>> might >>>>> find that onerous. >>>>> >>>>> Pushing to the official repo leaves more history that someone might >>> find >>>>> interesting some day. Also more clutter; some people are very >> concerned >>>>> about repacking before merging to develop. >>>>> >>>>> Another issue with gitflow is the master branch. The master branch is >>>>> supposed to get merged to for releases. The maven-release-plugin >> won't >>> do >>>>> that, and the jgitflow plugin is unsafe. So one option is to 'use >>>> gitflow' >>>>> but not bother with the master versus develop distinction, the other >> is >>>> to >>>>> do manual merges to master at release points. >>>>> >>>>> >>>>> On Tue, Dec 9, 2014 at 10:22 AM, Joe Witt <[email protected]> >> wrote: >>>>> >>>>>> Hello >>>>>> >>>>>> So a question on gitflow given that commits are now underway. When >>>>> working >>>>>> on a feature in a local repo which is a branch off the develop >>>>> branch...do >>>>>> you push the feature branch to the central repo? This then can be >>>> merged >>>>>> by someone else as a sort of code review/pull process? >>>>>> >>>>>> Thanks >>>>>> Joe >>>>>> >>>>>> On Mon, Dec 8, 2014 at 11:40 PM, Joe Witt <[email protected]> >>> wrote: >>>>>> >>>>>>> All, >>>>>>> >>>>>>> Now that we have our code up it is important to establish a >> process >>>>>> around >>>>>>> git in particular. A general consensus in the thread appears to >> be >>>>> that >>>>>>> gitflow workflow is a reasonable option. >> https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow >>>>>>> >>>>>>> To that end I've added a develop branch off of master from which >>>>> features >>>>>>> can be built. As we converge toward a release then we'll >>>>>> address/introduce >>>>>>> some of the other aspects of gitflow. >>>>>>> >>>>>>> Please discuss/comment if there are views that we should be >> taking >>>>>> another >>>>>>> path. >>>>>>> >>>>>>> Thanks >>>>>>> Joe >>>>>>> >>>>>>> On Sat, Nov 29, 2014 at 8:16 AM, Benson Margulies < >>>>> [email protected] >>>>>>> >>>>>>> wrote: >>>>>>> >>>>>>>> On Sat, Nov 29, 2014 at 3:45 AM, Sergio Fernández < >>>> [email protected]> >>>>>>>> wrote: >>>>>>>>> Hi Adam, >>>>>>>>> >>>>>>>>> one remarks about this: >>>>>>>>> >>>>>>>>>> On 28/11/14 18:07, Adam Taft wrote: >>>>>>>>>> >>>>>>>>>> Knowing how we work today, if it were me, I would suggest >> using >>>> the >>>>>>>> above >>>>>>>>>> workflow combined with the "forking workflow" to guard access >>> to >>>>> the >>>>>>>>>> production release (master) branches. A very small subset of >>> the >>>>>>>>>> incubator's commiters should have the ability to merge the >>>>> "develop" >>>>>>>>>> branch >>>>>>>>>> down to a master "release" branch. >>>>>>>>> >>>>>>>>> >>>>>>>>> Suck workflow is not in place in ASF. On the one hand, the >>> current >>>>> git >>>>>>>>> infrastructure does not provide such branches' management, >> like >>>>>>>>> bitbucket/stash do for instance. On the other hand, and more >>>>>> important, >>>>>>>> a >>>>>>>>> project is not hierarchical organization, but a a meritocratic >>>> one. >>>>>>>>> >>>>>>>>> I recommend you this blog post in case you want to read a bit >>>> more: >> http://communityovercode.com/2012/05/meritocracy-and-hierarchy/ >>>>>>>>> >>>>>>>>> If someone has permissions to do (i.e., he is a committer), he >>> can >>>>> do >>>>>>>> it, >>>>>>>>> simple The tool provide you instruments to revert those >> changes >>> in >>>>>> case >>>>>>>> on >>>>>>>>> involuntary errors. >>>>>>>>> >>>>>>>>>> It would be ideal to have someone who >>>>>>>>>> is NOT performing the majority of changes on the "develop" >>> branch >>>>>> take >>>>>>>>>> this >>>>>>>>>> role to coordinate releases, ensure minimal coding standards, >>> run >>>>>>>> through >>>>>>>>>> unit and integration tests, before signing off on the release >>> and >>>>>>>> issuing >>>>>>>>>> the release artifacts. >>>>>>>> >>>>>>>> You seem to be imagining an individual with a job which is >> shared >>> in >>>>>>>> by the community. In healthy communities, a release happens when >>>>>>>> there's a consensus to have a release. There is no person who >>>> 'ensures >>>>>>>> minimal coding standards', that's everyone watching commits. >>> There's >>>>>>>> no one 'running unit and integration tests' because (a) every >>>>>>>> committer does this before every commit, (b) Jenkins does it, >> (c) >>>> the >>>>>>>> release process does it. (d) there's no signing off on a >> release. >>>> The >>>>>>>> RM puts it up for a vote, and PMC members vote. >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Release comes after that. The release manager is responsible >> on >>>>>>>> creating a >>>>>>>>> proper release, which must include a code release and should >>>> include >>>>>>>>> binaries too. Each artifact release must be signed. >> Demonstrate >>>> your >>>>>>>> ability >>>>>>>>> as a project to produce releases is one of the goals of the >>>>>> incubation. >>>>>>>> But >>>>>>>>> we are not yet there, step by step. >>>>>>>>> >>>>>>>>> Hope that helps. >>>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Sergio Fernández >>>>>>>>> Partner Technology Manager >>>>>>>>> Redlink GmbH >>>>>>>>> m: +43 660 2747 925 >>>>>>>>> e: [email protected] >>>>>>>>> w: http://redlink.co >>
