I think the wiki update adequately represents what we’ve concluded in this thread.
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. >>> >> >> >> >>> >> >> >>> >> >> >>> >> >>> >> >>> >>> >>
