Thanks for all your feedback and getting this closed. I obviously still have a different personal preference but I still think it's good that this was discussed and documented, thank you all.
On Fri, Jan 8, 2016 at 6:13 PM, larry mccay <[email protected]> wrote: > I see... > > I can write up a version for patch based development - which is basically > what I use in Knox and in Hadoop. > The attachment of the patch on the JIRA being optional for Knox being the > only difference. > > If others are using the feature branch approach locally and think that > better represents what we want then we can leave it as is - maybe just some > clarification. > > On Fri, Jan 8, 2016 at 11:56 AM, Kevin Minder < > [email protected]> > wrote: > > > I agree they are confusing. These are really just notes from my early > > attempts at trying to figure out the best way to use git. Please feel > free > > to clean them up. > > > > > > > > > > On 1/8/16, 11:46 AM, "Sumit Gupta" <[email protected]> wrote: > > > > >I see. The two are not the same, which is why I think they are possibly > > >confusing. The “Committer Workflow using Git Branches” is about how to > use > > >remote git branches for feature development whereas “Committer Workflow” > > >just uses a local git branch. > > > > > > > > > > > >On 1/8/16, 10:53 AM, "larry mccay" <[email protected]> wrote: > > > > > >>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. > > >>> >>> >> >> >> > > >>> >>> >> >> > > >>> >>> >> >> > > >>> >>> >> > > >>> >>> >> > > >>> >>> > > >>> >>> > > >>> >> > > >>> > > >>> > > > > > >
