> 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
