You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Joe Bowser <bo...@gmail.com> on 2013/01/21 19:41:34 UTC
Can we talk about large features before they arrive in master?
Recently we've been noticing that there's been a lot of new features
going into Cordova this release, especially in Android. Now, I know
that I've been guilty of this in the past, which is partly why I'm
saying this now.
I think that we need to talk about features and make sure they work
before we push them into the master repository. We can't add new
methods that break how Cordova works for our older users without
having a really good reason for it. We have time to actually review
things before they arrive in the master branch, and it's trickier for
us to pull a feature out of master once it's in there, especially when
people commit to the branch after. That's why I think we should have
a review process in place.
What are people's thoughts on this?
Joe
Re: Can we talk about large features before they arrive in master?
Posted by Shazron <sh...@gmail.com>.
I always thought silence is assent, especially if the ML was given enough
notice. Per the Apache Voting Process:
http://www.apache.org/foundation/voting.html (Consensus Gauging through
Silence -- since we don't have a review then commit policy like WebKit)
On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org> wrote:
> One tough thing about this is knowing when you have agreement or not. E.g.
> for adding File.slice(), Braden sent out an email on the 7th saying that
> he'd like to add it for iOS & Android. Simon gave it a +1, and no one else
> responded. He then implemented the feature in a feature branch and then
> merged it when it was working. Since there was an email with few replies, I
> gathered that it had been discussed, and that no one really had anything to
> say about it.
>
> Joe - How do you propose that we make sure that things work? Or are you
> suggesting that a code-review serves this purpose?
>
> As for code reviews:
>
> I'd certainly be interested in more code-reviews. I think it's really
> useful to get feedback on changes. The only time when it becomes a burden
> is when turn-around time gets too long (e.g. you submit for review and no
> one looks at it for over a day).
>
> Up until now, we've been using the github pull-request interface to have
> others review our changes, but this isn't done very frequently. I also
> don't love this approach because comments through it don't get posted back
> to the cordova mailing-list.
>
> Another example is just using feature branches:
> Last week I tried out the branch approach for the CB-2227. I sent an email
> saying what I wanted to do, created a branch, and sent my first "I've made
> progress" out on Wednesday. On Thursday, I responded to the thread saying
> that the initial work was done and asked for feedback. I got no feedback,
> so today I merged into Master and updated the bug. I did this because I
> feel pretty confident about the change, but also because it will force
> everyone to test it out, and we're still early in the current
> release-cycle.
>
> Maybe there's a problem with people not noticing when feedback is being
> requested? Should we have a more structured way of asking for feedback on a
> branch? E.g. Start a new thread on the ML with the subject "Review Request:
> ..." or something like that? And if you're proposing a new API, say "API
> Review: ...". WDYT?
>
>
>
>
> On Mon, Jan 21, 2013 at 2:12 PM, Jesse MacFadyen <purplecabbage@gmail.com
> >wrote:
>
> > Agree with both of you. Also of note is the ripple effect of adding a
> > feature to one platform, see the slice() discussion for an example.
> > Any new feature should be explored and discussed further than just
> > ios/android.
> >
> >
> >
> > Cheers,
> > Jesse
> >
> > Sent from my iPhone5
> >
> > On 2013-01-21, at 10:50 AM, Brian LeRoux <b...@brian.io> wrote:
> >
> > Agree, but this should be just the regular flow rather than a formal
> > check in. Anything big should be in a branch and ideally ppl should
> > socialize branches on the list before the merge.
> >
> > On Mon, Jan 21, 2013 at 12:41 PM, Joe Bowser <bo...@gmail.com> wrote:
> > > Recently we've been noticing that there's been a lot of new features
> > > going into Cordova this release, especially in Android. Now, I know
> > > that I've been guilty of this in the past, which is partly why I'm
> > > saying this now.
> > >
> > > I think that we need to talk about features and make sure they work
> > > before we push them into the master repository. We can't add new
> > > methods that break how Cordova works for our older users without
> > > having a really good reason for it. We have time to actually review
> > > things before they arrive in the master branch, and it's trickier for
> > > us to pull a feature out of master once it's in there, especially when
> > > people commit to the branch after. That's why I think we should have
> > > a review process in place.
> > >
> > > What are people's thoughts on this?
> > >
> > > Joe
> >
>
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Grieve <ag...@chromium.org>.
Filed an INFRA ticket for ReviewBoard:
https://issues.apache.org/jira/browse/INFRA-5889
On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <ag...@chromium.org>wrote:
> ReviewBoard seems like a great fit to me! Let's try it out!
>
>
> On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org> wrote:
>
>> On 01/21/2013 01:24 PM, Joe Bowser wrote:
>> > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org>
>> wrote:
>> >> As for code reviews:
>> >>
>> >> I'd certainly be interested in more code-reviews. I think it's really
>> >> useful to get feedback on changes. The only time when it becomes a
>> burden
>> >> is when turn-around time gets too long (e.g. you submit for review and
>> no
>> >> one looks at it for over a day).
>> >>
>> >> Up until now, we've been using the github pull-request interface to
>> have
>> >> others review our changes, but this isn't done very frequently. I also
>> >> don't love this approach because comments through it don't get posted
>> back
>> >> to the cordova mailing-list.
>> >
>> > I'm not super thrilled by this either, because our GitHub pull request
>> > system is completely broken since we can't actually close requests and
>> > indicate when we think things are a good idea or not. I think we
>> > should do what Android does with Gerrit (see
>> > https://android-review.googlesource.com) , but that'll involve
>> > additional infrastructure and another war with INFRA about whether
>> > it's the Apache way or whatever.
>>
>> An instance of ReviewBoard [1] exists at Apache [2], so I don't think it
>> means war about the Apache way. Is that something that could fill this
>> need?
>>
>> Brian
>>
>> [1] https://reviews.apache.org/
>> [2]
>> https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
>>
>
>
Re: Can we talk about large features before they arrive in master?
Posted by Filip Maj <fi...@adobe.com>.
Only runs against master but there is always work to be done :)
On 1/22/13 11:34 AM, "Michael Brooks" <mi...@michaelbrooks.ca> wrote:
>>
>> Here's a draft for a new "Committing Your Own Changes:" section on
>> http://wiki.apache.org/cordova/CommitterWorkflow
>
>
>Great draft Andrew. It reads well.
>
>Outsider observation/note: probably useful for step 4 to tie into the CI -
>> getting the pull request to notify Fil's CI project.
>
>
>It would be nice to run a smoke test against the CI server. I'm unsure
>whether medic currently supports branches or only runs against master.
>
>On Tue, Jan 22, 2013 at 10:57 AM, Andrew Lunny <al...@gmail.com> wrote:
>
>> Outsider observation/note: probably useful for step 4 to tie into the
>>CI -
>> getting the pull request to notify Fil's CI project.
>>
>> On 22 January 2013 10:53, Andrew Grieve <ag...@chromium.org> wrote:
>>
>> > Here's a draft for a new "Committing Your Own Changes:" section on
>> > http://wiki.apache.org/cordova/CommitterWorkflow
>> >
>> > Step 1: Mail the Mailing-list
>> > - This is required if:
>> > - Your change will add/remove/change any public Cordova APIs.
>> > - You suspect that your change has a chance of being controversial
>> > - You would like feedback before you begin.
>> >
>> > When possible, try to phrase things in the form of a proposal. If no
>>one
>> > objects (within a workday or two), then consider yourself to have Lazy
>> > Consensus
>><http://www.apache.org/foundation/glossary.html#LazyConsensus
>> >.
>> >
>> > Step 2: Ensure there is a JIRA issue.
>> > - JIRA issues are used for both new features and for bugs.
>> > - The "Fix For" field is used for the purpose of Release Notes.
>> > - The issues are also used to track which commits / topic branches
>>are
>> > related to them.
>> >
>> > Step 3: Create a topic branch
>> > - Using a public topic branch is necessary only when either:
>> > 1. you would like to collaborate on the feature
>> > 2. you would like feedback on your code before committing
>> > - For small bugfixes, public topic branches are not required.
>> > - Note: You should never rebase a public topic branch!
>> >
>> > Step 4: Ask for a code review
>> > - If you are using a public topic branch, then you should ask for a
>> code
>> > review when you consider it to be complete.
>> > - For now, use a github pull request. Soon, use reviews.apache.org.
>> > - Email the ML so that anyone who is available can have a look at
>>your
>> > code. If you have someone in particular that you would like approval
>> from,
>> > be sure to add them in the To: of your email.
>> > - Again, sometimes this will end with a Lazy
>> >
>>Consensus<http://www.apache.org/foundation/glossary.html#LazyConsensus>
>> > .
>> >
>> > Step 5: Merge your change
>> > - Once your topic branch is tested & working, it's time to merge it.
>> Use
>> > the following workflow:
>> >
>> > git checkout master
>> > git pull apache master
>> > git checkout topic_branch
>> > git checkout -b to_be_merged
>> > git rebase master -i
>> > ...
>> > git checkout master
>> > git merge --ff-only to_be_merged
>> > git push apache master
>> > git branch -d to_be_merged
>> > git branch -D topic_branch
>> > git push apache :topic_branch
>> >
>> > The rebase -i step is your chance to clean up the commit messages and
>>to
>> > combine small commits when appropriate. For example:
>> > Commit A: Implemented RockOn feature (CB-1234)
>> > Commit B: Added tests for RockOn (CB-1234)
>> > Commit C: Fixed RockOn not working with empty strings
>> > Commit D: Renamed RockOn to JustRock
>> > Commit E: Refactor MainFeature to make use of JustRock.
>> >
>> > In this case, it would be appropriate to combine commits A-D into a
>> single
>> > commit, or at least commits A & C. Having a smaller number of commits
>> when
>> > merging makes it easier for others to comprehend the diff commits, and
>> also
>> > makes it easier to roll-back commits should the need arise. For JS
>> commits,
>> > prefix the message with [platform] so that it's clear who should take
>> > interest in the commit. For all commits, be sure to include the JIRA
>> issue
>> > number / URL.
>> >
>> >
>> > On Tue, Jan 22, 2013 at 1:19 PM, Braden Shepherdson
>><braden@chromium.org
>> > >wrote:
>> >
>> > > Code reviews will generally sound good to Googlers, so long as we
>>can
>> > keep
>> > > the turnaround down. It definitely keeps our code quality high on
>> > internal
>> > > projects, even if it is sometimes a pain to have to wait for a
>>response
>> > and
>> > > do your own reviews. I've asked Michal and Andrew for
>>over-the-shoulder
>> > iOS
>> > > reviews in the past, since I'm new to that platform.
>> > >
>> > > I also want to apologize for the trouble with the ArrayBuffers on
>> > Android.
>> > > I was running into the bug with navigating in mobile-spec causing
>> > > deviceready not to fire, and had just changed my start page to the
>> binary
>> > > echo test Michal wrote. It started working, so I cleaned up my
>> debugging
>> > > and pushed. That was premature, since I broke some of the tests and
>> > hadn't
>> > > run the automatic tests. Gomen nasai.
>> > >
>> > >
>> > > Braden
>> > >
>> > > On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve
>><agrieve@chromium.org
>> > > >wrote:
>> > >
>> > > > ReviewBoard seems like a great fit to me! Let's try it out!
>> > > >
>> > > >
>> > > > On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org>
>> > wrote:
>> > > >
>> > > > > On 01/21/2013 01:24 PM, Joe Bowser wrote:
>> > > > > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <
>> > agrieve@chromium.org
>> > > >
>> > > > > wrote:
>> > > > > >> As for code reviews:
>> > > > > >>
>> > > > > >> I'd certainly be interested in more code-reviews. I think
>>it's
>> > > really
>> > > > > >> useful to get feedback on changes. The only time when it
>> becomes a
>> > > > > burden
>> > > > > >> is when turn-around time gets too long (e.g. you submit for
>> review
>> > > and
>> > > > > no
>> > > > > >> one looks at it for over a day).
>> > > > > >>
>> > > > > >> Up until now, we've been using the github pull-request
>>interface
>> > to
>> > > > have
>> > > > > >> others review our changes, but this isn't done very
>>frequently.
>> I
>> > > also
>> > > > > >> don't love this approach because comments through it don't
>>get
>> > > posted
>> > > > > back
>> > > > > >> to the cordova mailing-list.
>> > > > > >
>> > > > > > I'm not super thrilled by this either, because our GitHub pull
>> > > request
>> > > > > > system is completely broken since we can't actually close
>> requests
>> > > and
>> > > > > > indicate when we think things are a good idea or not. I think
>>we
>> > > > > > should do what Android does with Gerrit (see
>> > > > > > https://android-review.googlesource.com) , but that'll involve
>> > > > > > additional infrastructure and another war with INFRA about
>> whether
>> > > > > > it's the Apache way or whatever.
>> > > > >
>> > > > > An instance of ReviewBoard [1] exists at Apache [2], so I don't
>> think
>> > > it
>> > > > > means war about the Apache way. Is that something that could
>>fill
>> > this
>> > > > > need?
>> > > > >
>> > > > > Brian
>> > > > >
>> > > > > [1] https://reviews.apache.org/
>> > > > > [2]
>> > > > >
>> > >
>> https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
>> > > > >
>> > > >
>> > >
>> >
>>
Re: Can we talk about large features before they arrive in master?
Posted by Michael Brooks <mi...@michaelbrooks.ca>.
>
> Here's a draft for a new "Committing Your Own Changes:" section on
> http://wiki.apache.org/cordova/CommitterWorkflow
Great draft Andrew. It reads well.
Outsider observation/note: probably useful for step 4 to tie into the CI -
> getting the pull request to notify Fil's CI project.
It would be nice to run a smoke test against the CI server. I'm unsure
whether medic currently supports branches or only runs against master.
On Tue, Jan 22, 2013 at 10:57 AM, Andrew Lunny <al...@gmail.com> wrote:
> Outsider observation/note: probably useful for step 4 to tie into the CI -
> getting the pull request to notify Fil's CI project.
>
> On 22 January 2013 10:53, Andrew Grieve <ag...@chromium.org> wrote:
>
> > Here's a draft for a new "Committing Your Own Changes:" section on
> > http://wiki.apache.org/cordova/CommitterWorkflow
> >
> > Step 1: Mail the Mailing-list
> > - This is required if:
> > - Your change will add/remove/change any public Cordova APIs.
> > - You suspect that your change has a chance of being controversial
> > - You would like feedback before you begin.
> >
> > When possible, try to phrase things in the form of a proposal. If no one
> > objects (within a workday or two), then consider yourself to have Lazy
> > Consensus <http://www.apache.org/foundation/glossary.html#LazyConsensus
> >.
> >
> > Step 2: Ensure there is a JIRA issue.
> > - JIRA issues are used for both new features and for bugs.
> > - The "Fix For" field is used for the purpose of Release Notes.
> > - The issues are also used to track which commits / topic branches are
> > related to them.
> >
> > Step 3: Create a topic branch
> > - Using a public topic branch is necessary only when either:
> > 1. you would like to collaborate on the feature
> > 2. you would like feedback on your code before committing
> > - For small bugfixes, public topic branches are not required.
> > - Note: You should never rebase a public topic branch!
> >
> > Step 4: Ask for a code review
> > - If you are using a public topic branch, then you should ask for a
> code
> > review when you consider it to be complete.
> > - For now, use a github pull request. Soon, use reviews.apache.org.
> > - Email the ML so that anyone who is available can have a look at your
> > code. If you have someone in particular that you would like approval
> from,
> > be sure to add them in the To: of your email.
> > - Again, sometimes this will end with a Lazy
> > Consensus<http://www.apache.org/foundation/glossary.html#LazyConsensus>
> > .
> >
> > Step 5: Merge your change
> > - Once your topic branch is tested & working, it's time to merge it.
> Use
> > the following workflow:
> >
> > git checkout master
> > git pull apache master
> > git checkout topic_branch
> > git checkout -b to_be_merged
> > git rebase master -i
> > ...
> > git checkout master
> > git merge --ff-only to_be_merged
> > git push apache master
> > git branch -d to_be_merged
> > git branch -D topic_branch
> > git push apache :topic_branch
> >
> > The rebase -i step is your chance to clean up the commit messages and to
> > combine small commits when appropriate. For example:
> > Commit A: Implemented RockOn feature (CB-1234)
> > Commit B: Added tests for RockOn (CB-1234)
> > Commit C: Fixed RockOn not working with empty strings
> > Commit D: Renamed RockOn to JustRock
> > Commit E: Refactor MainFeature to make use of JustRock.
> >
> > In this case, it would be appropriate to combine commits A-D into a
> single
> > commit, or at least commits A & C. Having a smaller number of commits
> when
> > merging makes it easier for others to comprehend the diff commits, and
> also
> > makes it easier to roll-back commits should the need arise. For JS
> commits,
> > prefix the message with [platform] so that it's clear who should take
> > interest in the commit. For all commits, be sure to include the JIRA
> issue
> > number / URL.
> >
> >
> > On Tue, Jan 22, 2013 at 1:19 PM, Braden Shepherdson <braden@chromium.org
> > >wrote:
> >
> > > Code reviews will generally sound good to Googlers, so long as we can
> > keep
> > > the turnaround down. It definitely keeps our code quality high on
> > internal
> > > projects, even if it is sometimes a pain to have to wait for a response
> > and
> > > do your own reviews. I've asked Michal and Andrew for over-the-shoulder
> > iOS
> > > reviews in the past, since I'm new to that platform.
> > >
> > > I also want to apologize for the trouble with the ArrayBuffers on
> > Android.
> > > I was running into the bug with navigating in mobile-spec causing
> > > deviceready not to fire, and had just changed my start page to the
> binary
> > > echo test Michal wrote. It started working, so I cleaned up my
> debugging
> > > and pushed. That was premature, since I broke some of the tests and
> > hadn't
> > > run the automatic tests. Gomen nasai.
> > >
> > >
> > > Braden
> > >
> > > On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <agrieve@chromium.org
> > > >wrote:
> > >
> > > > ReviewBoard seems like a great fit to me! Let's try it out!
> > > >
> > > >
> > > > On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org>
> > wrote:
> > > >
> > > > > On 01/21/2013 01:24 PM, Joe Bowser wrote:
> > > > > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <
> > agrieve@chromium.org
> > > >
> > > > > wrote:
> > > > > >> As for code reviews:
> > > > > >>
> > > > > >> I'd certainly be interested in more code-reviews. I think it's
> > > really
> > > > > >> useful to get feedback on changes. The only time when it
> becomes a
> > > > > burden
> > > > > >> is when turn-around time gets too long (e.g. you submit for
> review
> > > and
> > > > > no
> > > > > >> one looks at it for over a day).
> > > > > >>
> > > > > >> Up until now, we've been using the github pull-request interface
> > to
> > > > have
> > > > > >> others review our changes, but this isn't done very frequently.
> I
> > > also
> > > > > >> don't love this approach because comments through it don't get
> > > posted
> > > > > back
> > > > > >> to the cordova mailing-list.
> > > > > >
> > > > > > I'm not super thrilled by this either, because our GitHub pull
> > > request
> > > > > > system is completely broken since we can't actually close
> requests
> > > and
> > > > > > indicate when we think things are a good idea or not. I think we
> > > > > > should do what Android does with Gerrit (see
> > > > > > https://android-review.googlesource.com) , but that'll involve
> > > > > > additional infrastructure and another war with INFRA about
> whether
> > > > > > it's the Apache way or whatever.
> > > > >
> > > > > An instance of ReviewBoard [1] exists at Apache [2], so I don't
> think
> > > it
> > > > > means war about the Apache way. Is that something that could fill
> > this
> > > > > need?
> > > > >
> > > > > Brian
> > > > >
> > > > > [1] https://reviews.apache.org/
> > > > > [2]
> > > > >
> > >
> https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
> > > > >
> > > >
> > >
> >
>
Re: Can we talk about large features before they arrive in master?
Posted by Filip Maj <fi...@adobe.com>.
Not a bad idea, noted
On 1/22/13 10:57 AM, "Andrew Lunny" <al...@gmail.com> wrote:
>Outsider observation/note: probably useful for step 4 to tie into the CI -
>getting the pull request to notify Fil's CI project.
>
>On 22 January 2013 10:53, Andrew Grieve <ag...@chromium.org> wrote:
>
>> Here's a draft for a new "Committing Your Own Changes:" section on
>> http://wiki.apache.org/cordova/CommitterWorkflow
>>
>> Step 1: Mail the Mailing-list
>> - This is required if:
>> - Your change will add/remove/change any public Cordova APIs.
>> - You suspect that your change has a chance of being controversial
>> - You would like feedback before you begin.
>>
>> When possible, try to phrase things in the form of a proposal. If no one
>> objects (within a workday or two), then consider yourself to have Lazy
>> Consensus
>><http://www.apache.org/foundation/glossary.html#LazyConsensus>.
>>
>> Step 2: Ensure there is a JIRA issue.
>> - JIRA issues are used for both new features and for bugs.
>> - The "Fix For" field is used for the purpose of Release Notes.
>> - The issues are also used to track which commits / topic branches are
>> related to them.
>>
>> Step 3: Create a topic branch
>> - Using a public topic branch is necessary only when either:
>> 1. you would like to collaborate on the feature
>> 2. you would like feedback on your code before committing
>> - For small bugfixes, public topic branches are not required.
>> - Note: You should never rebase a public topic branch!
>>
>> Step 4: Ask for a code review
>> - If you are using a public topic branch, then you should ask for a
>>code
>> review when you consider it to be complete.
>> - For now, use a github pull request. Soon, use reviews.apache.org.
>> - Email the ML so that anyone who is available can have a look at your
>> code. If you have someone in particular that you would like approval
>>from,
>> be sure to add them in the To: of your email.
>> - Again, sometimes this will end with a Lazy
>> Consensus<http://www.apache.org/foundation/glossary.html#LazyConsensus>
>> .
>>
>> Step 5: Merge your change
>> - Once your topic branch is tested & working, it's time to merge it.
>>Use
>> the following workflow:
>>
>> git checkout master
>> git pull apache master
>> git checkout topic_branch
>> git checkout -b to_be_merged
>> git rebase master -i
>> ...
>> git checkout master
>> git merge --ff-only to_be_merged
>> git push apache master
>> git branch -d to_be_merged
>> git branch -D topic_branch
>> git push apache :topic_branch
>>
>> The rebase -i step is your chance to clean up the commit messages and to
>> combine small commits when appropriate. For example:
>> Commit A: Implemented RockOn feature (CB-1234)
>> Commit B: Added tests for RockOn (CB-1234)
>> Commit C: Fixed RockOn not working with empty strings
>> Commit D: Renamed RockOn to JustRock
>> Commit E: Refactor MainFeature to make use of JustRock.
>>
>> In this case, it would be appropriate to combine commits A-D into a
>>single
>> commit, or at least commits A & C. Having a smaller number of commits
>>when
>> merging makes it easier for others to comprehend the diff commits, and
>>also
>> makes it easier to roll-back commits should the need arise. For JS
>>commits,
>> prefix the message with [platform] so that it's clear who should take
>> interest in the commit. For all commits, be sure to include the JIRA
>>issue
>> number / URL.
>>
>>
>> On Tue, Jan 22, 2013 at 1:19 PM, Braden Shepherdson <braden@chromium.org
>> >wrote:
>>
>> > Code reviews will generally sound good to Googlers, so long as we can
>> keep
>> > the turnaround down. It definitely keeps our code quality high on
>> internal
>> > projects, even if it is sometimes a pain to have to wait for a
>>response
>> and
>> > do your own reviews. I've asked Michal and Andrew for
>>over-the-shoulder
>> iOS
>> > reviews in the past, since I'm new to that platform.
>> >
>> > I also want to apologize for the trouble with the ArrayBuffers on
>> Android.
>> > I was running into the bug with navigating in mobile-spec causing
>> > deviceready not to fire, and had just changed my start page to the
>>binary
>> > echo test Michal wrote. It started working, so I cleaned up my
>>debugging
>> > and pushed. That was premature, since I broke some of the tests and
>> hadn't
>> > run the automatic tests. Gomen nasai.
>> >
>> >
>> > Braden
>> >
>> > On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <agrieve@chromium.org
>> > >wrote:
>> >
>> > > ReviewBoard seems like a great fit to me! Let's try it out!
>> > >
>> > >
>> > > On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org>
>> wrote:
>> > >
>> > > > On 01/21/2013 01:24 PM, Joe Bowser wrote:
>> > > > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <
>> agrieve@chromium.org
>> > >
>> > > > wrote:
>> > > > >> As for code reviews:
>> > > > >>
>> > > > >> I'd certainly be interested in more code-reviews. I think it's
>> > really
>> > > > >> useful to get feedback on changes. The only time when it
>>becomes a
>> > > > burden
>> > > > >> is when turn-around time gets too long (e.g. you submit for
>>review
>> > and
>> > > > no
>> > > > >> one looks at it for over a day).
>> > > > >>
>> > > > >> Up until now, we've been using the github pull-request
>>interface
>> to
>> > > have
>> > > > >> others review our changes, but this isn't done very
>>frequently. I
>> > also
>> > > > >> don't love this approach because comments through it don't get
>> > posted
>> > > > back
>> > > > >> to the cordova mailing-list.
>> > > > >
>> > > > > I'm not super thrilled by this either, because our GitHub pull
>> > request
>> > > > > system is completely broken since we can't actually close
>>requests
>> > and
>> > > > > indicate when we think things are a good idea or not. I think we
>> > > > > should do what Android does with Gerrit (see
>> > > > > https://android-review.googlesource.com) , but that'll involve
>> > > > > additional infrastructure and another war with INFRA about
>>whether
>> > > > > it's the Apache way or whatever.
>> > > >
>> > > > An instance of ReviewBoard [1] exists at Apache [2], so I don't
>>think
>> > it
>> > > > means war about the Apache way. Is that something that could fill
>> this
>> > > > need?
>> > > >
>> > > > Brian
>> > > >
>> > > > [1] https://reviews.apache.org/
>> > > > [2]
>> > > >
>> >
>>https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
>> > > >
>> > >
>> >
>>
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Lunny <al...@gmail.com>.
Outsider observation/note: probably useful for step 4 to tie into the CI -
getting the pull request to notify Fil's CI project.
On 22 January 2013 10:53, Andrew Grieve <ag...@chromium.org> wrote:
> Here's a draft for a new "Committing Your Own Changes:" section on
> http://wiki.apache.org/cordova/CommitterWorkflow
>
> Step 1: Mail the Mailing-list
> - This is required if:
> - Your change will add/remove/change any public Cordova APIs.
> - You suspect that your change has a chance of being controversial
> - You would like feedback before you begin.
>
> When possible, try to phrase things in the form of a proposal. If no one
> objects (within a workday or two), then consider yourself to have Lazy
> Consensus <http://www.apache.org/foundation/glossary.html#LazyConsensus>.
>
> Step 2: Ensure there is a JIRA issue.
> - JIRA issues are used for both new features and for bugs.
> - The "Fix For" field is used for the purpose of Release Notes.
> - The issues are also used to track which commits / topic branches are
> related to them.
>
> Step 3: Create a topic branch
> - Using a public topic branch is necessary only when either:
> 1. you would like to collaborate on the feature
> 2. you would like feedback on your code before committing
> - For small bugfixes, public topic branches are not required.
> - Note: You should never rebase a public topic branch!
>
> Step 4: Ask for a code review
> - If you are using a public topic branch, then you should ask for a code
> review when you consider it to be complete.
> - For now, use a github pull request. Soon, use reviews.apache.org.
> - Email the ML so that anyone who is available can have a look at your
> code. If you have someone in particular that you would like approval from,
> be sure to add them in the To: of your email.
> - Again, sometimes this will end with a Lazy
> Consensus<http://www.apache.org/foundation/glossary.html#LazyConsensus>
> .
>
> Step 5: Merge your change
> - Once your topic branch is tested & working, it's time to merge it. Use
> the following workflow:
>
> git checkout master
> git pull apache master
> git checkout topic_branch
> git checkout -b to_be_merged
> git rebase master -i
> ...
> git checkout master
> git merge --ff-only to_be_merged
> git push apache master
> git branch -d to_be_merged
> git branch -D topic_branch
> git push apache :topic_branch
>
> The rebase -i step is your chance to clean up the commit messages and to
> combine small commits when appropriate. For example:
> Commit A: Implemented RockOn feature (CB-1234)
> Commit B: Added tests for RockOn (CB-1234)
> Commit C: Fixed RockOn not working with empty strings
> Commit D: Renamed RockOn to JustRock
> Commit E: Refactor MainFeature to make use of JustRock.
>
> In this case, it would be appropriate to combine commits A-D into a single
> commit, or at least commits A & C. Having a smaller number of commits when
> merging makes it easier for others to comprehend the diff commits, and also
> makes it easier to roll-back commits should the need arise. For JS commits,
> prefix the message with [platform] so that it's clear who should take
> interest in the commit. For all commits, be sure to include the JIRA issue
> number / URL.
>
>
> On Tue, Jan 22, 2013 at 1:19 PM, Braden Shepherdson <braden@chromium.org
> >wrote:
>
> > Code reviews will generally sound good to Googlers, so long as we can
> keep
> > the turnaround down. It definitely keeps our code quality high on
> internal
> > projects, even if it is sometimes a pain to have to wait for a response
> and
> > do your own reviews. I've asked Michal and Andrew for over-the-shoulder
> iOS
> > reviews in the past, since I'm new to that platform.
> >
> > I also want to apologize for the trouble with the ArrayBuffers on
> Android.
> > I was running into the bug with navigating in mobile-spec causing
> > deviceready not to fire, and had just changed my start page to the binary
> > echo test Michal wrote. It started working, so I cleaned up my debugging
> > and pushed. That was premature, since I broke some of the tests and
> hadn't
> > run the automatic tests. Gomen nasai.
> >
> >
> > Braden
> >
> > On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <agrieve@chromium.org
> > >wrote:
> >
> > > ReviewBoard seems like a great fit to me! Let's try it out!
> > >
> > >
> > > On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org>
> wrote:
> > >
> > > > On 01/21/2013 01:24 PM, Joe Bowser wrote:
> > > > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <
> agrieve@chromium.org
> > >
> > > > wrote:
> > > > >> As for code reviews:
> > > > >>
> > > > >> I'd certainly be interested in more code-reviews. I think it's
> > really
> > > > >> useful to get feedback on changes. The only time when it becomes a
> > > > burden
> > > > >> is when turn-around time gets too long (e.g. you submit for review
> > and
> > > > no
> > > > >> one looks at it for over a day).
> > > > >>
> > > > >> Up until now, we've been using the github pull-request interface
> to
> > > have
> > > > >> others review our changes, but this isn't done very frequently. I
> > also
> > > > >> don't love this approach because comments through it don't get
> > posted
> > > > back
> > > > >> to the cordova mailing-list.
> > > > >
> > > > > I'm not super thrilled by this either, because our GitHub pull
> > request
> > > > > system is completely broken since we can't actually close requests
> > and
> > > > > indicate when we think things are a good idea or not. I think we
> > > > > should do what Android does with Gerrit (see
> > > > > https://android-review.googlesource.com) , but that'll involve
> > > > > additional infrastructure and another war with INFRA about whether
> > > > > it's the Apache way or whatever.
> > > >
> > > > An instance of ReviewBoard [1] exists at Apache [2], so I don't think
> > it
> > > > means war about the Apache way. Is that something that could fill
> this
> > > > need?
> > > >
> > > > Brian
> > > >
> > > > [1] https://reviews.apache.org/
> > > > [2]
> > > >
> > https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
> > > >
> > >
> >
>
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Grieve <ag...@chromium.org>.
Here's a draft for a new "Committing Your Own Changes:" section on
http://wiki.apache.org/cordova/CommitterWorkflow
Step 1: Mail the Mailing-list
- This is required if:
- Your change will add/remove/change any public Cordova APIs.
- You suspect that your change has a chance of being controversial
- You would like feedback before you begin.
When possible, try to phrase things in the form of a proposal. If no one
objects (within a workday or two), then consider yourself to have Lazy
Consensus <http://www.apache.org/foundation/glossary.html#LazyConsensus>.
Step 2: Ensure there is a JIRA issue.
- JIRA issues are used for both new features and for bugs.
- The "Fix For" field is used for the purpose of Release Notes.
- The issues are also used to track which commits / topic branches are
related to them.
Step 3: Create a topic branch
- Using a public topic branch is necessary only when either:
1. you would like to collaborate on the feature
2. you would like feedback on your code before committing
- For small bugfixes, public topic branches are not required.
- Note: You should never rebase a public topic branch!
Step 4: Ask for a code review
- If you are using a public topic branch, then you should ask for a code
review when you consider it to be complete.
- For now, use a github pull request. Soon, use reviews.apache.org.
- Email the ML so that anyone who is available can have a look at your
code. If you have someone in particular that you would like approval from,
be sure to add them in the To: of your email.
- Again, sometimes this will end with a Lazy
Consensus<http://www.apache.org/foundation/glossary.html#LazyConsensus>
.
Step 5: Merge your change
- Once your topic branch is tested & working, it's time to merge it. Use
the following workflow:
git checkout master
git pull apache master
git checkout topic_branch
git checkout -b to_be_merged
git rebase master -i
...
git checkout master
git merge --ff-only to_be_merged
git push apache master
git branch -d to_be_merged
git branch -D topic_branch
git push apache :topic_branch
The rebase -i step is your chance to clean up the commit messages and to
combine small commits when appropriate. For example:
Commit A: Implemented RockOn feature (CB-1234)
Commit B: Added tests for RockOn (CB-1234)
Commit C: Fixed RockOn not working with empty strings
Commit D: Renamed RockOn to JustRock
Commit E: Refactor MainFeature to make use of JustRock.
In this case, it would be appropriate to combine commits A-D into a single
commit, or at least commits A & C. Having a smaller number of commits when
merging makes it easier for others to comprehend the diff commits, and also
makes it easier to roll-back commits should the need arise. For JS commits,
prefix the message with [platform] so that it's clear who should take
interest in the commit. For all commits, be sure to include the JIRA issue
number / URL.
On Tue, Jan 22, 2013 at 1:19 PM, Braden Shepherdson <br...@chromium.org>wrote:
> Code reviews will generally sound good to Googlers, so long as we can keep
> the turnaround down. It definitely keeps our code quality high on internal
> projects, even if it is sometimes a pain to have to wait for a response and
> do your own reviews. I've asked Michal and Andrew for over-the-shoulder iOS
> reviews in the past, since I'm new to that platform.
>
> I also want to apologize for the trouble with the ArrayBuffers on Android.
> I was running into the bug with navigating in mobile-spec causing
> deviceready not to fire, and had just changed my start page to the binary
> echo test Michal wrote. It started working, so I cleaned up my debugging
> and pushed. That was premature, since I broke some of the tests and hadn't
> run the automatic tests. Gomen nasai.
>
>
> Braden
>
> On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <agrieve@chromium.org
> >wrote:
>
> > ReviewBoard seems like a great fit to me! Let's try it out!
> >
> >
> > On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org> wrote:
> >
> > > On 01/21/2013 01:24 PM, Joe Bowser wrote:
> > > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <agrieve@chromium.org
> >
> > > wrote:
> > > >> As for code reviews:
> > > >>
> > > >> I'd certainly be interested in more code-reviews. I think it's
> really
> > > >> useful to get feedback on changes. The only time when it becomes a
> > > burden
> > > >> is when turn-around time gets too long (e.g. you submit for review
> and
> > > no
> > > >> one looks at it for over a day).
> > > >>
> > > >> Up until now, we've been using the github pull-request interface to
> > have
> > > >> others review our changes, but this isn't done very frequently. I
> also
> > > >> don't love this approach because comments through it don't get
> posted
> > > back
> > > >> to the cordova mailing-list.
> > > >
> > > > I'm not super thrilled by this either, because our GitHub pull
> request
> > > > system is completely broken since we can't actually close requests
> and
> > > > indicate when we think things are a good idea or not. I think we
> > > > should do what Android does with Gerrit (see
> > > > https://android-review.googlesource.com) , but that'll involve
> > > > additional infrastructure and another war with INFRA about whether
> > > > it's the Apache way or whatever.
> > >
> > > An instance of ReviewBoard [1] exists at Apache [2], so I don't think
> it
> > > means war about the Apache way. Is that something that could fill this
> > > need?
> > >
> > > Brian
> > >
> > > [1] https://reviews.apache.org/
> > > [2]
> > >
> https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
> > >
> >
>
Re: Can we talk about large features before they arrive in master?
Posted by Braden Shepherdson <br...@chromium.org>.
Code reviews will generally sound good to Googlers, so long as we can keep
the turnaround down. It definitely keeps our code quality high on internal
projects, even if it is sometimes a pain to have to wait for a response and
do your own reviews. I've asked Michal and Andrew for over-the-shoulder iOS
reviews in the past, since I'm new to that platform.
I also want to apologize for the trouble with the ArrayBuffers on Android.
I was running into the bug with navigating in mobile-spec causing
deviceready not to fire, and had just changed my start page to the binary
echo test Michal wrote. It started working, so I cleaned up my debugging
and pushed. That was premature, since I broke some of the tests and hadn't
run the automatic tests. Gomen nasai.
Braden
On Tue, Jan 22, 2013 at 10:59 AM, Andrew Grieve <ag...@chromium.org>wrote:
> ReviewBoard seems like a great fit to me! Let's try it out!
>
>
> On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org> wrote:
>
> > On 01/21/2013 01:24 PM, Joe Bowser wrote:
> > > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org>
> > wrote:
> > >> As for code reviews:
> > >>
> > >> I'd certainly be interested in more code-reviews. I think it's really
> > >> useful to get feedback on changes. The only time when it becomes a
> > burden
> > >> is when turn-around time gets too long (e.g. you submit for review and
> > no
> > >> one looks at it for over a day).
> > >>
> > >> Up until now, we've been using the github pull-request interface to
> have
> > >> others review our changes, but this isn't done very frequently. I also
> > >> don't love this approach because comments through it don't get posted
> > back
> > >> to the cordova mailing-list.
> > >
> > > I'm not super thrilled by this either, because our GitHub pull request
> > > system is completely broken since we can't actually close requests and
> > > indicate when we think things are a good idea or not. I think we
> > > should do what Android does with Gerrit (see
> > > https://android-review.googlesource.com) , but that'll involve
> > > additional infrastructure and another war with INFRA about whether
> > > it's the Apache way or whatever.
> >
> > An instance of ReviewBoard [1] exists at Apache [2], so I don't think it
> > means war about the Apache way. Is that something that could fill this
> > need?
> >
> > Brian
> >
> > [1] https://reviews.apache.org/
> > [2]
> > https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
> >
>
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Grieve <ag...@chromium.org>.
ReviewBoard seems like a great fit to me! Let's try it out!
On Mon, Jan 21, 2013 at 8:43 PM, Brian M Dube <bd...@apache.org> wrote:
> On 01/21/2013 01:24 PM, Joe Bowser wrote:
> > On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
> >> As for code reviews:
> >>
> >> I'd certainly be interested in more code-reviews. I think it's really
> >> useful to get feedback on changes. The only time when it becomes a
> burden
> >> is when turn-around time gets too long (e.g. you submit for review and
> no
> >> one looks at it for over a day).
> >>
> >> Up until now, we've been using the github pull-request interface to have
> >> others review our changes, but this isn't done very frequently. I also
> >> don't love this approach because comments through it don't get posted
> back
> >> to the cordova mailing-list.
> >
> > I'm not super thrilled by this either, because our GitHub pull request
> > system is completely broken since we can't actually close requests and
> > indicate when we think things are a good idea or not. I think we
> > should do what Android does with Gerrit (see
> > https://android-review.googlesource.com) , but that'll involve
> > additional infrastructure and another war with INFRA about whether
> > it's the Apache way or whatever.
>
> An instance of ReviewBoard [1] exists at Apache [2], so I don't think it
> means war about the Apache way. Is that something that could fill this
> need?
>
> Brian
>
> [1] https://reviews.apache.org/
> [2]
> https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
>
Re: Can we talk about large features before they arrive in master?
Posted by Brian M Dube <bd...@apache.org>.
On 01/21/2013 01:24 PM, Joe Bowser wrote:
> On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org> wrote:
>> As for code reviews:
>>
>> I'd certainly be interested in more code-reviews. I think it's really
>> useful to get feedback on changes. The only time when it becomes a burden
>> is when turn-around time gets too long (e.g. you submit for review and no
>> one looks at it for over a day).
>>
>> Up until now, we've been using the github pull-request interface to have
>> others review our changes, but this isn't done very frequently. I also
>> don't love this approach because comments through it don't get posted back
>> to the cordova mailing-list.
>
> I'm not super thrilled by this either, because our GitHub pull request
> system is completely broken since we can't actually close requests and
> indicate when we think things are a good idea or not. I think we
> should do what Android does with Gerrit (see
> https://android-review.googlesource.com) , but that'll involve
> additional infrastructure and another war with INFRA about whether
> it's the Apache way or whatever.
An instance of ReviewBoard [1] exists at Apache [2], so I don't think it
means war about the Apache way. Is that something that could fill this need?
Brian
[1] https://reviews.apache.org/
[2] https://blogs.apache.org/infra/entry/reviewboard_instance_running_at_the
Re: Can we talk about large features before they arrive in master?
Posted by Joe Bowser <bo...@gmail.com>.
On Mon, Jan 21, 2013 at 2:13 PM, Filip Maj <fi...@adobe.com> wrote:
>
>>Braden just used a private branch and didn't ask for review. Maybe the
>>take-away from this is that we it would be good to have the chance for a
>>review for anything that was big enough to warrant a branch? I'll add this
>>to the committer's wiki page.
>
> Yeah let's summarize our discussion points / rules of thumb down in the
> wiki page in case we were missing anything there.
>
> TBH we haven't done many code reviews in the past. I believe
> CordovaWebView was one of the biggest features we landed since about
> Cordova 1.0 that we had actually organized a code review, set up a
> conference call and screenshare to join in, etc. It lived in a branch I
> believe through two or three point releases just because it was a giant
> feature, needed a couple rebases/merges, and failed the first couple times
> we tried to merge it in.
>
I feel that this was to excessive, and that it actually hurt us in the
long-run having CordovaWebView in review for so long. That being
said, it did teach us what's important to watch out for when
introducing new features, and what features will cause our users to
want our heads on sticks. I think an informal review is good enough.
I usually wait 24 to 48 hours before merging things in. That being
said, I'm able to amuse myself with weird side projects like Purity
(https://github.com/infil00p/cordova-android/tree/puritytool) and
other tasks while I wait. :P
> In general, committers are trusted to be working on features that are
> important to the project and will land high quality code eventually into
> master. We just need that extra step of discipline of running mobile-spec
> and running the native tests for affected platforms on a few devices for
> sanity's sake. And as always, if you can't do this for whatever reason,
> you can always reach out to the list and ask for other committers to test
> out feature x on device y, or even just a smoke/sanity test on another
> committer's machine, etc. before merging into master.
>
Agreed! We have a lack of old Android 2.3 devices in the Vancouver
office, and a lot of Android 4.x devices. I'll usually need to get
things tested on the original Samsung Galaxy S or some other Samsung
Galaxy phone that's not the top of the line, or is ruggedized or
something (i.e. Samsung Galaxy Rugby).
Re: Can we talk about large features before they arrive in master?
Posted by Filip Maj <fi...@adobe.com>.
>Braden just used a private branch and didn't ask for review. Maybe the
>take-away from this is that we it would be good to have the chance for a
>review for anything that was big enough to warrant a branch? I'll add this
>to the committer's wiki page.
Yeah let's summarize our discussion points / rules of thumb down in the
wiki page in case we were missing anything there.
TBH we haven't done many code reviews in the past. I believe
CordovaWebView was one of the biggest features we landed since about
Cordova 1.0 that we had actually organized a code review, set up a
conference call and screenshare to join in, etc. It lived in a branch I
believe through two or three point releases just because it was a giant
feature, needed a couple rebases/merges, and failed the first couple times
we tried to merge it in.
In general, committers are trusted to be working on features that are
important to the project and will land high quality code eventually into
master. We just need that extra step of discipline of running mobile-spec
and running the native tests for affected platforms on a few devices for
sanity's sake. And as always, if you can't do this for whatever reason,
you can always reach out to the list and ask for other committers to test
out feature x on device y, or even just a smoke/sanity test on another
committer's machine, etc. before merging into master.
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Grieve <ag...@chromium.org>.
On Mon, Jan 21, 2013 at 4:24 PM, Joe Bowser <bo...@gmail.com> wrote:
> On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
> > One tough thing about this is knowing when you have agreement or not.
> E.g.
> > for adding File.slice(), Braden sent out an email on the 7th saying that
> > he'd like to add it for iOS & Android. Simon gave it a +1, and no one
> else
> > responded. He then implemented the feature in a feature branch and then
> > merged it when it was working. Since there was an email with few
> replies, I
> > gathered that it had been discussed, and that no one really had anything
> to
> > say about it.
> >
> > Joe - How do you propose that we make sure that things work? Or are you
> > suggesting that a code-review serves this purpose?
> >
>
> I do think a code review serves this purpose. I need to see a
> published branch somewhere that I can test to make sure that things
> don't break on my end before I can say "Hey, this looks good!", and
> I'd prefer if mobile-spec was updated as part of the feature so I
> don't have to figure out what's being done myself.
>
> > As for code reviews:
> >
> > I'd certainly be interested in more code-reviews. I think it's really
> > useful to get feedback on changes. The only time when it becomes a burden
> > is when turn-around time gets too long (e.g. you submit for review and no
> > one looks at it for over a day).
> >
> > Up until now, we've been using the github pull-request interface to have
> > others review our changes, but this isn't done very frequently. I also
> > don't love this approach because comments through it don't get posted
> back
> > to the cordova mailing-list.
>
> I'm not super thrilled by this either, because our GitHub pull request
> system is completely broken since we can't actually close requests and
> indicate when we think things are a good idea or not. I think we
> should do what Android does with Gerrit (see
> https://android-review.googlesource.com) , but that'll involve
> additional infrastructure and another war with INFRA about whether
> it's the Apache way or whatever.
>
> >
> > Another example is just using feature branches:
> > Last week I tried out the branch approach for the CB-2227. I sent an
> email
> > saying what I wanted to do, created a branch, and sent my first "I've
> made
> > progress" out on Wednesday. On Thursday, I responded to the thread saying
> > that the initial work was done and asked for feedback. I got no feedback,
> > so today I merged into Master and updated the bug. I did this because I
> > feel pretty confident about the change, but also because it will force
> > everyone to test it out, and we're still early in the current
> release-cycle.
> >
>
> I think this is fine and is exactly what needs to happen. It didn't
> seem to break anything ,and there was a branch that we could look at.
> The thing is that I don't remember seeing a branch for ArrayBuffer
> that we could look at. I'll dig through my e-mail to find it, but I
> don't remember seeing it.
>
Braden just used a private branch and didn't ask for review. Maybe the
take-away from this is that we it would be good to have the chance for a
review for anything that was big enough to warrant a branch? I'll add this
to the committer's wiki page.
>
> > Maybe there's a problem with people not noticing when feedback is being
> > requested? Should we have a more structured way of asking for feedback
> on a
> > branch? E.g. Start a new thread on the ML with the subject "Review
> Request:
> > ..." or something like that? And if you're proposing a new API, say "API
> > Review: ...". WDYT?
> >
>
> I think that having a proper header on the e-mail would be a good plan.
>
Re: Can we talk about large features before they arrive in master?
Posted by Filip Maj <fi...@adobe.com>.
In my opinion the last few "omfg shit's broken" emails comes mainly from
lack of testing. If a committer puts something into master, they need to
do "due diligence" for it, and that boils down to one question: do tests
pass on devices+platforms affected by the commit?
iOS this is easier (per se) than Android as there are less devices to
worry about. Android is a wild west. I suppose the "gimme feedback on this
feature" threads are a good opportunity to reach out to the rest of the
committers and ask for a sanity test across more devices (and should be
done at a minimum if you feel you don't have good device coverage on your
own).
Here are some rules of thumb I follow:
- if I want to check something into cordova-js that is common across
platforms, I need to put in extra work to do the testing. That means I
grab a couple devices from a couple different platforms and make sure the
tests pass at a reasonable rate (at or around the previous passing % for
that platform). And sometimes issue come up that are not caught by the
cordova-js unit tests (esp. if they are run on node). Sometimes these
issues are platform/device-specific.
- if I'm checking something into platform code, I make sure to test
getting the sample app (or better yet mobile-spec) running using the
getting started instructions.
- for iOS, I usually run a smoke test on an iPod and since I got an
iPhone5 to test on, on that too.
- on Android I run tests on either a galaxy nexus or nexus 7 (both run
4.2.1) and a nexus S (2.3.6).
That said, there are certainly some issues uncovered. As you brought up
Andrew, I cannot find a "hey lets tag next week" email to the list for
2.4.0rc1 either. I think the Adobe team was well aware internally that we
wanted to cut a release now-ish. But, clearly, we failed to communicate
that on the list. Definitely won't happen again and that's our bad.
As long as everyone runs tests accordingly, this isn't a huge issue. It is
on its way to being resolved and we'll release the rc1 of 2.4.0 in the
coming days.
On 1/21/13 1:24 PM, "Joe Bowser" <bo...@gmail.com> wrote:
>On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org>
>wrote:
>> One tough thing about this is knowing when you have agreement or not.
>>E.g.
>> for adding File.slice(), Braden sent out an email on the 7th saying that
>> he'd like to add it for iOS & Android. Simon gave it a +1, and no one
>>else
>> responded. He then implemented the feature in a feature branch and then
>> merged it when it was working. Since there was an email with few
>>replies, I
>> gathered that it had been discussed, and that no one really had
>>anything to
>> say about it.
>>
>> Joe - How do you propose that we make sure that things work? Or are you
>> suggesting that a code-review serves this purpose?
>>
>
>I do think a code review serves this purpose. I need to see a
>published branch somewhere that I can test to make sure that things
>don't break on my end before I can say "Hey, this looks good!", and
>I'd prefer if mobile-spec was updated as part of the feature so I
>don't have to figure out what's being done myself.
>
>> As for code reviews:
>>
>> I'd certainly be interested in more code-reviews. I think it's really
>> useful to get feedback on changes. The only time when it becomes a
>>burden
>> is when turn-around time gets too long (e.g. you submit for review and
>>no
>> one looks at it for over a day).
>>
>> Up until now, we've been using the github pull-request interface to have
>> others review our changes, but this isn't done very frequently. I also
>> don't love this approach because comments through it don't get posted
>>back
>> to the cordova mailing-list.
>
>I'm not super thrilled by this either, because our GitHub pull request
>system is completely broken since we can't actually close requests and
>indicate when we think things are a good idea or not. I think we
>should do what Android does with Gerrit (see
>https://android-review.googlesource.com) , but that'll involve
>additional infrastructure and another war with INFRA about whether
>it's the Apache way or whatever.
>
>>
>> Another example is just using feature branches:
>> Last week I tried out the branch approach for the CB-2227. I sent an
>>email
>> saying what I wanted to do, created a branch, and sent my first "I've
>>made
>> progress" out on Wednesday. On Thursday, I responded to the thread
>>saying
>> that the initial work was done and asked for feedback. I got no
>>feedback,
>> so today I merged into Master and updated the bug. I did this because I
>> feel pretty confident about the change, but also because it will force
>> everyone to test it out, and we're still early in the current
>>release-cycle.
>>
>
>I think this is fine and is exactly what needs to happen. It didn't
>seem to break anything ,and there was a branch that we could look at.
>The thing is that I don't remember seeing a branch for ArrayBuffer
>that we could look at. I'll dig through my e-mail to find it, but I
>don't remember seeing it.
>
>> Maybe there's a problem with people not noticing when feedback is being
>> requested? Should we have a more structured way of asking for feedback
>>on a
>> branch? E.g. Start a new thread on the ML with the subject "Review
>>Request:
>> ..." or something like that? And if you're proposing a new API, say "API
>> Review: ...". WDYT?
>>
>
>I think that having a proper header on the e-mail would be a good plan.
Re: Can we talk about large features before they arrive in master?
Posted by Joe Bowser <bo...@gmail.com>.
On Mon, Jan 21, 2013 at 1:06 PM, Andrew Grieve <ag...@chromium.org> wrote:
> One tough thing about this is knowing when you have agreement or not. E.g.
> for adding File.slice(), Braden sent out an email on the 7th saying that
> he'd like to add it for iOS & Android. Simon gave it a +1, and no one else
> responded. He then implemented the feature in a feature branch and then
> merged it when it was working. Since there was an email with few replies, I
> gathered that it had been discussed, and that no one really had anything to
> say about it.
>
> Joe - How do you propose that we make sure that things work? Or are you
> suggesting that a code-review serves this purpose?
>
I do think a code review serves this purpose. I need to see a
published branch somewhere that I can test to make sure that things
don't break on my end before I can say "Hey, this looks good!", and
I'd prefer if mobile-spec was updated as part of the feature so I
don't have to figure out what's being done myself.
> As for code reviews:
>
> I'd certainly be interested in more code-reviews. I think it's really
> useful to get feedback on changes. The only time when it becomes a burden
> is when turn-around time gets too long (e.g. you submit for review and no
> one looks at it for over a day).
>
> Up until now, we've been using the github pull-request interface to have
> others review our changes, but this isn't done very frequently. I also
> don't love this approach because comments through it don't get posted back
> to the cordova mailing-list.
I'm not super thrilled by this either, because our GitHub pull request
system is completely broken since we can't actually close requests and
indicate when we think things are a good idea or not. I think we
should do what Android does with Gerrit (see
https://android-review.googlesource.com) , but that'll involve
additional infrastructure and another war with INFRA about whether
it's the Apache way or whatever.
>
> Another example is just using feature branches:
> Last week I tried out the branch approach for the CB-2227. I sent an email
> saying what I wanted to do, created a branch, and sent my first "I've made
> progress" out on Wednesday. On Thursday, I responded to the thread saying
> that the initial work was done and asked for feedback. I got no feedback,
> so today I merged into Master and updated the bug. I did this because I
> feel pretty confident about the change, but also because it will force
> everyone to test it out, and we're still early in the current release-cycle.
>
I think this is fine and is exactly what needs to happen. It didn't
seem to break anything ,and there was a branch that we could look at.
The thing is that I don't remember seeing a branch for ArrayBuffer
that we could look at. I'll dig through my e-mail to find it, but I
don't remember seeing it.
> Maybe there's a problem with people not noticing when feedback is being
> requested? Should we have a more structured way of asking for feedback on a
> branch? E.g. Start a new thread on the ML with the subject "Review Request:
> ..." or something like that? And if you're proposing a new API, say "API
> Review: ...". WDYT?
>
I think that having a proper header on the e-mail would be a good plan.
Re: Can we talk about large features before they arrive in master?
Posted by Andrew Grieve <ag...@chromium.org>.
One tough thing about this is knowing when you have agreement or not. E.g.
for adding File.slice(), Braden sent out an email on the 7th saying that
he'd like to add it for iOS & Android. Simon gave it a +1, and no one else
responded. He then implemented the feature in a feature branch and then
merged it when it was working. Since there was an email with few replies, I
gathered that it had been discussed, and that no one really had anything to
say about it.
Joe - How do you propose that we make sure that things work? Or are you
suggesting that a code-review serves this purpose?
As for code reviews:
I'd certainly be interested in more code-reviews. I think it's really
useful to get feedback on changes. The only time when it becomes a burden
is when turn-around time gets too long (e.g. you submit for review and no
one looks at it for over a day).
Up until now, we've been using the github pull-request interface to have
others review our changes, but this isn't done very frequently. I also
don't love this approach because comments through it don't get posted back
to the cordova mailing-list.
Another example is just using feature branches:
Last week I tried out the branch approach for the CB-2227. I sent an email
saying what I wanted to do, created a branch, and sent my first "I've made
progress" out on Wednesday. On Thursday, I responded to the thread saying
that the initial work was done and asked for feedback. I got no feedback,
so today I merged into Master and updated the bug. I did this because I
feel pretty confident about the change, but also because it will force
everyone to test it out, and we're still early in the current release-cycle.
Maybe there's a problem with people not noticing when feedback is being
requested? Should we have a more structured way of asking for feedback on a
branch? E.g. Start a new thread on the ML with the subject "Review Request:
..." or something like that? And if you're proposing a new API, say "API
Review: ...". WDYT?
On Mon, Jan 21, 2013 at 2:12 PM, Jesse MacFadyen <pu...@gmail.com>wrote:
> Agree with both of you. Also of note is the ripple effect of adding a
> feature to one platform, see the slice() discussion for an example.
> Any new feature should be explored and discussed further than just
> ios/android.
>
>
>
> Cheers,
> Jesse
>
> Sent from my iPhone5
>
> On 2013-01-21, at 10:50 AM, Brian LeRoux <b...@brian.io> wrote:
>
> Agree, but this should be just the regular flow rather than a formal
> check in. Anything big should be in a branch and ideally ppl should
> socialize branches on the list before the merge.
>
> On Mon, Jan 21, 2013 at 12:41 PM, Joe Bowser <bo...@gmail.com> wrote:
> > Recently we've been noticing that there's been a lot of new features
> > going into Cordova this release, especially in Android. Now, I know
> > that I've been guilty of this in the past, which is partly why I'm
> > saying this now.
> >
> > I think that we need to talk about features and make sure they work
> > before we push them into the master repository. We can't add new
> > methods that break how Cordova works for our older users without
> > having a really good reason for it. We have time to actually review
> > things before they arrive in the master branch, and it's trickier for
> > us to pull a feature out of master once it's in there, especially when
> > people commit to the branch after. That's why I think we should have
> > a review process in place.
> >
> > What are people's thoughts on this?
> >
> > Joe
>
Re: Can we talk about large features before they arrive in master?
Posted by Jesse MacFadyen <pu...@gmail.com>.
Agree with both of you. Also of note is the ripple effect of adding a
feature to one platform, see the slice() discussion for an example.
Any new feature should be explored and discussed further than just
ios/android.
Cheers,
Jesse
Sent from my iPhone5
On 2013-01-21, at 10:50 AM, Brian LeRoux <b...@brian.io> wrote:
Agree, but this should be just the regular flow rather than a formal
check in. Anything big should be in a branch and ideally ppl should
socialize branches on the list before the merge.
On Mon, Jan 21, 2013 at 12:41 PM, Joe Bowser <bo...@gmail.com> wrote:
> Recently we've been noticing that there's been a lot of new features
> going into Cordova this release, especially in Android. Now, I know
> that I've been guilty of this in the past, which is partly why I'm
> saying this now.
>
> I think that we need to talk about features and make sure they work
> before we push them into the master repository. We can't add new
> methods that break how Cordova works for our older users without
> having a really good reason for it. We have time to actually review
> things before they arrive in the master branch, and it's trickier for
> us to pull a feature out of master once it's in there, especially when
> people commit to the branch after. That's why I think we should have
> a review process in place.
>
> What are people's thoughts on this?
>
> Joe
Re: Can we talk about large features before they arrive in master?
Posted by Brian LeRoux <b...@brian.io>.
Agree, but this should be just the regular flow rather than a formal
check in. Anything big should be in a branch and ideally ppl should
socialize branches on the list before the merge.
On Mon, Jan 21, 2013 at 12:41 PM, Joe Bowser <bo...@gmail.com> wrote:
> Recently we've been noticing that there's been a lot of new features
> going into Cordova this release, especially in Android. Now, I know
> that I've been guilty of this in the past, which is partly why I'm
> saying this now.
>
> I think that we need to talk about features and make sure they work
> before we push them into the master repository. We can't add new
> methods that break how Cordova works for our older users without
> having a really good reason for it. We have time to actually review
> things before they arrive in the master branch, and it's trickier for
> us to pull a feature out of master once it's in there, especially when
> people commit to the branch after. That's why I think we should have
> a review process in place.
>
> What are people's thoughts on this?
>
> Joe