Cool - thanks, Sumit. I am less concerned with it being confusing than with it being the same as the previous section.
On Fri, Jan 8, 2016 at 10:43 AM, Sumit Gupta <[email protected]> wrote: > Hey Larry, thanks for the wiki update. It is clearly written and makes > sense to me. > > The Committer Workflow with git Branches section is more of a capture of > git commands than anything else and I can cleaned it up if it is confusing. > > Sumit. > > On 1/8/16, 9:35 AM, "larry mccay" <[email protected]> wrote: > > >All - > > > >Please review the contributing page updates that I just added. Not sure > >how > >to do drafts in the wiki... > >https://cwiki.apache.org/confluence/display/KNOX/Contribution+Process > > > >Note the new section at the top called: Code Change Governing Policy > > > >This is the overall description or our policy, its definition, our > >additional guidelines and our [REVIEW] notification mechanism. > > > >I also made minor language problems to align with this policy throughout > >the document. > > > >I notice that the Committer Workflow and Committer Workflow with git > >Branches seem redundant. Is there any meaningful difference between the > >two? > >Given that most of our commits do not use feature branches, perhaps we > >should change the first one to be patch based instead? > > > >thanks, > > > >--larry > > > >On Fri, Jan 8, 2016 at 8:43 AM, larry mccay <[email protected]> wrote: > > > >> Great - I will spend some time today putting something together that we > >> can iterate over. > >> > >> On Thu, Jan 7, 2016 at 2:29 PM, Sumit Gupta > >><[email protected]> > >> wrote: > >> > >>> +1. I’m good with this. > >>> > >>> > >>> On 1/7/16, 12:42 PM, "larry mccay" <[email protected]> wrote: > >>> > >>> >Okay - let's bring this to a close - with a slight clarification on > >>>the > >>> >[REVIEW] emails... > >>> > > >>> >As long as we are all in agreement on the following, I will attempt to > >>> >document it as our contribution policy (with a follow up email for the > >>> >draft): > >>> > > >>> >* Knox has a CTR policy for committers > >>> >* For any patches that make fundamental architectural or security > >>>related > >>> >changes - committers should solicit feedback on the design > >>> >* For any patches that are extensions to existing patterns for > >>>features - > >>> >such as adding new service API support, the committer may commit > >>>freely - > >>> >given sufficient tests and documentation (to the committer's > >>>discretion) > >>> >* For any patches that are simple bug fixes, the committer may commit > >>> >freely > >>> > > >>> >* In order to facilitate reviews from other community members post > >>> commit, > >>> >we will use an email with a [REVIEW] tag to indicate that comments are > >>> >being provided for a merged change and that it needs attention. > >>> > > >>> >There is nothing saying that we can't revisit the CTR policy in the > >>> future > >>> >and we will continue to leverage the contributions of the community at > >>> >large. If at any point, any of our development process seems to be > >>> barrier > >>> >for contributors we will need to be open to change. > >>> > > >>> >@Sumit - note that the above does not describe a [REVIEW] email for > >>>every > >>> >feature. > >>> > > >>> >Features continue to be described in JIRA and designs in attached > >>> >documents > >>> >or wiki. > >>> >Reviews should also be conducted in JIRA. > >>> >Reviews for something already committed and the JIRA is closed can > >>>have a > >>> >[REVIEW] email as a flag to the committers to give attention to the > >>> >feedback. > >>> > > >>> >If everyone is good with the above, I will draw up a draft. > >>> > > >>> >On Thu, Jan 7, 2016 at 11:42 AM, Sumit Gupta < > >>> [email protected]> > >>> >wrote: > >>> > > >>> >> Sorry for coming late to the discussion. Like Kevin, I seem to be > >>> >> constantly missing email from the dev list (the email server doesn¹t > >>> >>like > >>> >> me). > >>> >> > >>> >> I¹m not sure how having a CTR policy for most things and then a > >>> >>selective > >>> >> RTC policy helps drive community involvement. It seems hard to > >>> >>implement. > >>> >> If anything, I can see more [DISCUSS] threads and like Larry > >>>suggested > >>> a > >>> >> [REVIEW] email for all features. This would help awareness as well > >>>as > >>> be > >>> >> useful in obtaining critical feedback. I would have preferred a JIRA > >>> >> mechanism for asking for a review since we get JIRA emails on the > >>>dev > >>> >>list > >>> >> anyway, but I can see a review tag or comment getting lost in swarm > >>>of > >>> >> JIRA email. > >>> >> > >>> >> In short, I would prefer a single straightforward policy of CTR and > >>> >>other > >>> >> means of raising awareness and soliciting input. > >>> >> > >>> >> > >>> >> On 1/7/16, 8:40 AM, "larry mccay" <[email protected]> wrote: > >>> >> > >>> >> >I am not looking to change our policy at this point but instead to > >>> >>define > >>> >> >the expectations of CTR in the Knox community - largely based on > >>>what > >>> >>has > >>> >> >been done all along unofficially. The [REVIEW] email to dev@ seems > >>>a > >>> >> >decent > >>> >> >way to raise attention to a review for committed code. > >>> >> > > >>> >> >On Thu, Jan 7, 2016 at 12:15 AM, Kevin Minder > >>> >> ><[email protected]> > >>> >> >wrote: > >>> >> > > >>> >> >> So in short: Bugs==CTR, Features==RTC? > >>> >> >> > >>> >> > > >>> >> >I don't know that Features==RTC is required for extensions of > >>>existing > >>> >> >patterns. > >>> >> >For instance, adding support for a new service shouldn't require > >>>RTC. > >>> >> > > >>> >> >This does bring something to mind though. If there is complicated > >>> >>custom > >>> >> >HA > >>> >> >related dispatch code then that should probably be documented or > >>> >> >communicated to the dev@ and it would be up to the committer to > >>> >>determine > >>> >> >whether they would like to ask for a review. > >>> >> > > >>> >> >Understanding the HA dispatch algorithms for services by the dev > >>> >>community > >>> >> >or at least the ability to get to an understanding through readily > >>> >> >available documentation. > >>> >> > > >>> >> > > >>> >> >> > >>> >> >> From: larry mccay <[email protected]<mailto:[email protected]>> > >>> >> >> Date: Wednesday, January 6, 2016 at 11:06 PM > >>> >> >> To: Kevin Minder <[email protected]<mailto: > >>> >> >> [email protected]>> > >>> >> >> Cc: "[email protected]<mailto:[email protected]>" > >>> >> >><[email protected] > >>> >> >> <mailto:[email protected]>> > >>> >> >> Subject: Re: [DISCUSS] CTR, Requiring Patch Attachments and Cool > >>>Off > >>> >> >>Period > >>> >> >> > >>> >> >> Hi Lars - > >>> >> >> > >>> >> >> Thanks for bumping this thread again - we do need to bring it to > >>>a > >>> >> >>close. > >>> >> >> > >>> >> >> I certainly agree with Kevin on CTR as the current and well > >>>working > >>> >> >>policy > >>> >> >> for this project. > >>> >> >> > >>> >> >> @Kevin - I believe that we need to also consider the cool off > >>>period > >>> >> >>yet. > >>> >> >> I do not believe that external contributions are being blocked by > >>> the > >>> >> >>lack > >>> >> >> of a cool off period. > >>> >> >> > >>> >> >> We have numerous externally contributed features, enhancements to > >>> >> >>existing > >>> >> >> features and bug fixes. > >>> >> >> > >>> >> >> Sorry to hear that you have had bad experiences with other > >>>projects > >>> >> >> ignoring reviews. > >>> >> >> Let me make sure that I understand what you describe... > >>> >> >> > >>> >> >> I think that you are saying that you provided review comments and > >>> >> >> suggestions to someone's patch for some apache project and that > >>>the > >>> >> >>review > >>> >> >> comments were never responded to and the patch was committed > >>>without > >>> >> >> acknowledgement. Maybe you provided a review for code that was > >>> >>already > >>> >> >> committed and it was ignored. > >>> >> >> > >>> >> >> I can see the review being easier to miss/ignore when the code > >>>and > >>> >>JIRA > >>> >> >>is > >>> >> >> already committed and closed. > >>> >> >> > >>> >> >> My proposal is that we document the following (most of which is > >>>from > >>> >>my > >>> >> >> original email): > >>> >> >> > >>> >> >> * Changes, that are aligned with existing design patterns and > >>>Knox > >>> >> >> architecture, that are either incremental enhancements/features > >>>or > >>> >>bug > >>> >> >> fixes can be purely CTR > >>> >> >> * Changes that necessitate architectural changes, have > >>>significant > >>> >> >> security implications, or are generally large should be reviewed. > >>> >>This > >>> >> >>is > >>> >> >> to facilitate communication of such changes as well as to have > >>> >>another > >>> >> >>set > >>> >> >> of eyes for the code. > >>> >> >> * Architectural changes and completely new features should be > >>> >> >>communicated > >>> >> >> via the dev@ list and possibly within a wiki page for design > >>> >> >> discussion/communication. > >>> >> >> * Review feedback for patches that have already been committed > >>>and > >>> >>JIRAs > >>> >> >> closed/resolved should also be sent to the dev@ list with a > >>> [REVIEW] > >>> >> tag > >>> >> >> in the subject. Committers will need to assess the points raised > >>>and > >>> >> >> respond. Optionally, based on discussion the JIRA can be > >>>reopened, > >>> >> >>possibly > >>> >> >> commit reverted or new JIRAs filed to address the reviewer's > >>> >>feedback. > >>> >> >> > >>> >> >> I think that this will fully communicate our development process > >>>and > >>> >> >> provide rules of engagement for post commit reviews. > >>> >> >> > >>> >> >> Let me know your thoughts on this plan. > >>> >> >> > >>> >> >> thanks, > >>> >> >> > >>> >> >> --larry > >>> >> >> > >>> >> >> On Wed, Jan 6, 2016 at 10:31 PM, Kevin Minder < > >>> >> >> > >>>[email protected]<mailto:[email protected]>> > >>> >> >>wrote: > >>> >> >> First apologies. Seems my mail server has been randomly eating > >>> >>Apache > >>> >> >> mail. I didn¹t see the original mail here. > >>> >> >> > >>> >> >> First point, attached patches. I used to attach patches to all > >>>my > >>> >>jira. > >>> >> >> Then I realized that a) the jira had links with diffs for the > >>>lazy > >>> >>and > >>> >> >>³git > >>> >> >> format-patch <commit-sha>² will generate the patch for those that > >>> >>want > >>> >> >>to > >>> >> >> use their favorite tool on a local patch file. At that point > >>> >>requiring > >>> >> >> patch generation from committers for a CTR project just seemed > >>>like > >>> >> >>process > >>> >> >> for the sake of process. This being said we could do a better > >>>job > >>> of > >>> >> >> documenting this for those unfamiliar with git if Larry hasn¹t > >>> >>already > >>> >> >> taken care of that. > >>> >> >> > >>> >> >> Second point, CTR. We have been CRT from the beginning and this > >>> >>model > >>> >> >> certainly made sense given the rapid pace of development. Things > >>> >>have > >>> >> >> certainly slowed down recently and we can continue the > >>>discussion. > >>> >> >> However, the core of the question you are really asking is would > >>> more > >>> >> >> people contribute if we were RTC. I don¹t think so. We fairly > >>>bend > >>> >>of > >>> >> >> backwards to embrace traffic on user@knox and dev@knox. I think > >>> that > >>> >> is > >>> >> >> far more significant than a CTR vs RTC policy. > >>> >> >> > >>> >> >> > >>> >> >> > >>> >> >> On 1/6/16, 7:49 PM, "Lars Francke" > >>><[email protected]<mailto: > >>> >> >> [email protected]>> wrote: > >>> >> >> > >>> >> >> >Does anyone have anything further to add here? Looking forward > >>>to a > >>> >>few > >>> >> >> >more opinions. > >>> >> >> > > >>> >> >> >On Fri, Dec 11, 2015 at 3:52 PM, Lars Francke > >>> >><[email protected] > >>> >> >> <mailto:[email protected]>> > >>> >> >> >wrote: > >>> >> >> > > >>> >> >> >> Hi, > >>> >> >> >> > >>> >> >> >> thanks Larry. > >>> >> >> >> > >>> >> >> >> Lars provided the Knox community with some feedback into our > >>> >> >>development > >>> >> >> >>> practices and JIRA usage [1]. > >>> >> >> >>> > >>> >> >> >>> I wanted to bring up a DISCUSS thread on how our CTR policy > >>>may > >>> >>or > >>> >> >>may > >>> >> >> >>> not relate to a couple points made in his feedback. In > >>> >>particular: > >>> >> >> >>> > >>> >> >> >>> 1. Whether a CTR based policy should require actual patches > >>> >> >>attached to > >>> >> >> >>> every JIRA or does the git link to a commit provide the same > >>> >> >>ability to > >>> >> >> >>> review post commit > >>> >> >> >>> > >>> >> >> >> > >>> >> >> >> I think having a patch attached before commit is a good thing > >>> >> >>because: > >>> >> >> >> * It allows feedback before a change goes in. In my experience > >>> >>that > >>> >> >>is > >>> >> >> >> easier to change than committed code > >>> >> >> >> * It allows looking at the exact change set in a standardised > >>> form > >>> >> >>(diff > >>> >> >> >> format) without having to use whatever web frontend (pure Git, > >>> >> >>Github, > >>> >> >> ...) > >>> >> >> >> is currently being used (so a patch file is useful even when > >>>only > >>> >> >> attached > >>> >> >> >> after committing)[1] > >>> >> >> >> * Should the Git web interface change (or be down) at some > >>>point > >>> >>in > >>> >> >>the > >>> >> >> >> future all those links might go stale (say Apache switches to > >>> >>Github > >>> >> >>or > >>> >> >> >> Gitlab or whatever) > >>> >> >> >> > >>> >> >> >> The downsides I can come up with is the extra work required to > >>> >>attach > >>> >> >> the > >>> >> >> >> patch (before or after commit). > >>> >> >> >> > >>> >> >> >> > >>> >> >> >>> 2. Whether a cool off period of 24 hrs would encourage more > >>> >>external > >>> >> >> >>> contributions and if so, how > >>> >> >> >>> > >>> >> >> >> > >>> >> >> >> This one is probably a matter of weighing off between fast > >>> >>iteration > >>> >> >>and > >>> >> >> >> possible feedback on patches. I assume with a project the > >>>size of > >>> >> >>Knox > >>> >> >> >> there is not much feedback coming in for each patch so the > >>> >>benefits > >>> >> >>of > >>> >> >> >> immediate commits probably outweigh the (assumed) benefits of > >>> >> >>waiting. > >>> >> >> >> > >>> >> >> >> My personal opinion though is that a wait period is a good > >>>thing. > >>> >> >>I've > >>> >> >> >> been bitten (and frustrated) in the past by reviews that have > >>> been > >>> >> >> ignored > >>> >> >> >> by certain communities/members/companies where it was clear > >>>that > >>> a > >>> >> >> release > >>> >> >> >> schedule had to be met. Ignoring reviews is easier with code > >>> >>that's > >>> >> >> already > >>> >> >> >> been committed. Again this is my personal (bad) experience > >>>and I > >>> >> >>have no > >>> >> >> >> reason to believe that the Knox community behaves the same. A > >>> cool > >>> >> >>off > >>> >> >> >> period doesn't mean that reviews are mandatory, it just > >>> >> >>invites/allows > >>> >> >> >> feedback in my opinion. > >>> >> >> >> > >>> >> >> >> One "real" benefit for pre-commit reviews is that there's > >>>tools > >>> >> >> available > >>> >> >> >> and in use at Apache for that (Reviewboard and JIRA to a > >>>degree) > >>> >>but > >>> >> >> >> there's currently no tooling support for post-commit reviews. > >>> >> >> >> > >>> >> >> >> Caveat to all of this: I'm only here for a few days a year > >>> >>probably > >>> >> >>and > >>> >> >> >> won't contribute much if at all so you should decide carefully > >>> >> >>whether > >>> >> >> you > >>> >> >> >> want to change working practices for an unknown benefit. > >>> >> >> >> > >>> >> >> >> Cheers, > >>> >> >> >> Lars > >>> >> >> >> > >>> >> >> >> [1] I know that the current web interface has an option to > >>> >>download > >>> >> >>the > >>> >> >> >> patch but it's a different process than most other "Hadoop > >>> >>related" > >>> >> >> >> projects. > >>> >> >> >> > >>> >> >> > >>> >> >> > >>> >> > >>> >> > >>> > >>> > >> > >
