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. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >
