Folks, So it appears as though folks are either neutral or in favor of RTC and Gitflow. Lets just go ahead and ensure we start making it happen then. Perhaps for now a simple approach of before pushing to the develop branch you have another committer review your code. A pull request or patch or some agreed upon approach should be fine. The reviewer can simply comment or +1 on the relevant Jira ticket. Either the reviewer or the contributor depending on the circumstance then can ensure the code gets merged. As we mature and feel our way through this we can establish a more documented review process. It would be good for folks to document the things to look for in a review. That will walk us toward code style issues/preferences if we have them and so on. Key thing is not to forget what some folks have mentioned here which is the review process is about mentoring and building up others just as much as it is about ensuring code quality.
Thanks for all the great comments and feedback. It is really exciting to see the initial seeds of community growth. Thanks Joe On Thu, Dec 11, 2014 at 8:19 AM, Joe Witt <[email protected]> wrote: > Mark, > > I can appreciate your skepticism and view here and am glad you're willing > to consider this as being able to make either work. > > One thing to remember (and this is me talking to all of us more than it is > at - you).... > > The reviews will be by committers and pmc members and will be of any > contribution regardless of standing in the project. This creates a level > playing field of contribution but a healthy model of curating contributions > to a high level of quality while at the same time helping to both train and > identify new committers. That is fantastic. If we have folks without any > experience doing reviews then we as a community/project have done something > fundamentally wrong. Now, there are perhaps parts of the application which > require a certain level of subject matter expertise such that an in-depth > peer review may be tough to come by. Those will be slower. But then > again...those are likely then the most critical to build depth on. > > If CTR and RTC are a spectrum where CTR is -1 and RTC is +1 I really hope > we end up at 0.5. > > Thanks > Joe > > > > On Thu, Dec 11, 2014 at 8:02 AM, Mark Payne <[email protected]> wrote: > >> So I'll admit that my experience in this area has been terrible is may be >> very different from what I will experience here. "Code Review" in my world >> has often meant "let this new developer without any experience review your >> code and then you can spend 3 days explaining each line to him." And this >> will absolutely cripple us. I don't think this is what we're talking about >> here, so I'll retract that statement :) >> So I'll remain with Tony in the middle and give no +1 to RTC or CTR. >> Thanks-Mark >> >> > Date: Wed, 10 Dec 2014 18:42:22 -0800 >> > Subject: Re: git workflow for nifi >> > From: [email protected] >> > To: [email protected] >> > >> > > though I think it will VERY MUCH impede the speed at which any new >> features / bug fixes can be accomplished. >> > >> > I don't agree, at least not in the long run. Also, I think a review is >> > only required before checking into the main development branch. If >> > we're using gitflow, then commits to feature branches should be >> > handled by who ever is managing that feature. That can mean doing >> > reviews as they come in or waiting until the feature is done and >> > reviewing it all at once. But having code reviews saves a lot of time >> > in the long run. >> > >> > Another thing to consider is that over time, committers should be >> > spending less time working on new features/fixing bugs and more time >> > on growing the community and providing quality code reviews is one of >> > the best ways to do that. >> > >> > On Wed, Dec 10, 2014 at 6:34 PM, Mark Payne <[email protected]> >> wrote: >> > > 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 >> > >>> >> > >> > >> > >> > -- >> > Joey Echeverria >> >> > >
