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 <kevin.min...@hortonworks.com> 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" <sumit.gu...@hortonworks.com> 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" <lmc...@apache.org> 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 < > sumit.gu...@hortonworks.com> > >>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" <lmc...@apache.org> 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 <lmc...@apache.org> > 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 > >>> >><sumit.gu...@hortonworks.com> > >>> >> wrote: > >>> >> > >>> >>> +1. I’m good with this. > >>> >>> > >>> >>> > >>> >>> On 1/7/16, 12:42 PM, "larry mccay" <larry.mc...@gmail.com> 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 < > >>> >>> sumit.gu...@hortonworks.com> > >>> >>> >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" <lmc...@apache.org> 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 > >>> >>> >> ><kevin.min...@hortonworks.com> > >>> >>> >> >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 > >>><lmc...@apache.org<mailto:lmc...@apache.org>> > >>> >>> >> >> Date: Wednesday, January 6, 2016 at 11:06 PM > >>> >>> >> >> To: Kevin Minder <kevin.min...@hortonworks.com<mailto: > >>> >>> >> >> kevin.min...@hortonworks.com>> > >>> >>> >> >> Cc: "dev@knox.apache.org<mailto:dev@knox.apache.org>" > >>> >>> >> >><dev@knox.apache.org > >>> >>> >> >> <mailto:dev@knox.apache.org>> > >>> >>> >> >> 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 < > >>> >>> >> >> > >>> >>>kevin.min...@hortonworks.com<mailto:kevin.min...@hortonworks.com>> > >>> >>> >> >>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" > >>> >>><lars.fran...@gmail.com<mailto: > >>> >>> >> >> lars.fran...@gmail.com>> 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 > >>> >>> >><lars.fran...@gmail.com > >>> >>> >> >> <mailto:lars.fran...@gmail.com>> > >>> >>> >> >> >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. > >>> >>> >> >> >> > >>> >>> >> >> > >>> >>> >> >> > >>> >>> >> > >>> >>> >> > >>> >>> > >>> >>> > >>> >> > >>> > >>> > > >
