If RTC means what Joey just wrote then I am a strong +1 on this. Though I am very curious to see if there are any strong CTR arguments.
On Wed, Dec 10, 2014 at 9:38 PM, Joey Echeverria <[email protected]> wrote: > > - What constitutes a valid review? > > I think the community will get to decide, but what I've seen in other > projects is there has to be a +1 from a committer on the project. The > +1 can be a comment in JIRA, clicking the ship it button in Review > Board, or any other method where the +1 has been recorded in ASF > infrastructure. For something like Hadoop, this has been fairly formal > with a +1 needing to happen in JIRA before something can be committed. > I know Parquet uses pull requests, but I'm not sure if they allow +1's > to just be comments in github or if they need to also be provided in > the JIRA. > > My general belief is that either JIRA or a review tool is fine. > Generally, a formal vote isn't required for a review and would only be > needed if a committer has vetoed a change and consensus can't be > reached through the normal review back and forth. > > > > - What is the best tooling/process around this to actually > conduct/document > > the review? > > There are two things you typically want, a standard for how > code/patches are submitted. Going down to the what git commands to run > to format the patch is very useful as each project is a little > different. If we're able to use pull requests, then that's less > critical as the pull request will handle those details. The second is > where to do the review comments themselves. I really like tools like > Review Board or GitHub's pull request review interface. Having a web > based way of providing per-line comments makes things flow much more > easily than comments in JIRA or mailing list discussions. > > > - What are some key things a reviewer should be checking for and helping > to > > enforce? What are some out of bounds things? > > There are two things that the reviewer should be focusing on. How will > this change improve the project and how can I help mentor the > contributor to generally improve the quality of their contributions. > That means that anything is in scope, but the focus of the review > should always be on how to guide the contributor to make the best > improvement to the project. If we want to adopt formal code style > guidelines, then we should document those and ideally have automated > ways of checking those (checkstyle, etc.) so that contributors can > verify for themselves rather than have to wait for review feedback. > > Generally a veto on a code change requires a "valid technical reason". > I think the community has to decide what falls under that definition. > In my head it would include things like: introduces a security hole, > introduces a major performance regression, or violates or > compatibility guidelines for the release that it's targeting. > > -Joey > > On Wed, Dec 10, 2014 at 6:18 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 >
