> I don't agree, at least not in the long run. I'm with Joey. Also, CTR doesn't imply "no review". I actually think it makes things messier if changes need to be backed out.
On Wed, Dec 10, 2014 at 9:42 PM, Joey Echeverria <[email protected]> wrote: > > 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 >
