[DISCUSS] PR Based Contributor Workflow

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[DISCUSS] PR Based Contributor Workflow

John D. Ament
All,

I put together a first pass PR on an improved contributor workflow that can
leverage github PRs.  This is in addition to our existing patch approach.

You can find the PR here, with the changes:
https://github.com/apache/deltaspike/pull/61/files

Using PRs gives us a bit of an advantage:

- We don't lose the original author in the commit
- We can run automated tests prior to the commit being merged in

Please take a look, I'm happy to adjust as needed.  I also took the liberty
to replace some of the to-be-retired links (e.g. people.a.o is retiring
soon, mail archives are being moved to pony, ICLA is now PDF based)

John
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

Daniel Cunha
Hi John,

Greate job. I think that we really need to have that. It's much more easy
and cool to work with PR.
Easy way to review, easy way to fix changes, the contributor does not need
to attach a new patch just need to update the PR and we'll have feedbacks
more fast with PR Builder Plugin and comments by line on PR.

I prefer this way, totally agree with your PR.

+1 :)

On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament <[hidden email]>
wrote:

> All,
>
> I put together a first pass PR on an improved contributor workflow that can
> leverage github PRs.  This is in addition to our existing patch approach.
>
> You can find the PR here, with the changes:
> https://github.com/apache/deltaspike/pull/61/files
>
> Using PRs gives us a bit of an advantage:
>
> - We don't lose the original author in the commit
> - We can run automated tests prior to the commit being merged in
>
> Please take a look, I'm happy to adjust as needed.  I also took the liberty
> to replace some of the to-be-retired links (e.g. people.a.o is retiring
> soon, mail archives are being moved to pony, ICLA is now PDF based)
>
> John
>



--
Daniel Cunha
https://twitter.com/dvlc_
http://www.tomitribe.com
http://www.tomitribe.io
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

Christian Kaltepoth
Hey John,

Great work!

+1 ;)

Christian

2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:

> Hi John,
>
> Greate job. I think that we really need to have that. It's much more easy
> and cool to work with PR.
> Easy way to review, easy way to fix changes, the contributor does not need
> to attach a new patch just need to update the PR and we'll have feedbacks
> more fast with PR Builder Plugin and comments by line on PR.
>
> I prefer this way, totally agree with your PR.
>
> +1 :)
>
> On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament <[hidden email]>
> wrote:
>
> > All,
> >
> > I put together a first pass PR on an improved contributor workflow that
> can
> > leverage github PRs.  This is in addition to our existing patch approach.
> >
> > You can find the PR here, with the changes:
> > https://github.com/apache/deltaspike/pull/61/files
> >
> > Using PRs gives us a bit of an advantage:
> >
> > - We don't lose the original author in the commit
> > - We can run automated tests prior to the commit being merged in
> >
> > Please take a look, I'm happy to adjust as needed.  I also took the
> liberty
> > to replace some of the to-be-retired links (e.g. people.a.o is retiring
> > soon, mail archives are being moved to pony, ICLA is now PDF based)
> >
> > John
> >
>
>
>
> --
> Daniel Cunha
> https://twitter.com/dvlc_
> http://www.tomitribe.com
> http://www.tomitribe.io
>



--
Christian Kaltepoth
Blog: http://blog.kaltepoth.de/
Twitter: http://twitter.com/chkal
GitHub: https://github.com/chkal
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

Jason Porter
+1 PRs are much easier to work with, imo.

On Sun, Jul 24, 2016 at 1:40 AM, Christian Kaltepoth <[hidden email]
> wrote:

> Hey John,
>
> Great work!
>
> +1 ;)
>
> Christian
>
> 2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:
>
> > Hi John,
> >
> > Greate job. I think that we really need to have that. It's much more easy
> > and cool to work with PR.
> > Easy way to review, easy way to fix changes, the contributor does not
> need
> > to attach a new patch just need to update the PR and we'll have feedbacks
> > more fast with PR Builder Plugin and comments by line on PR.
> >
> > I prefer this way, totally agree with your PR.
> >
> > +1 :)
> >
> > On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament <[hidden email]>
> > wrote:
> >
> > > All,
> > >
> > > I put together a first pass PR on an improved contributor workflow that
> > can
> > > leverage github PRs.  This is in addition to our existing patch
> approach.
> > >
> > > You can find the PR here, with the changes:
> > > https://github.com/apache/deltaspike/pull/61/files
> > >
> > > Using PRs gives us a bit of an advantage:
> > >
> > > - We don't lose the original author in the commit
> > > - We can run automated tests prior to the commit being merged in
> > >
> > > Please take a look, I'm happy to adjust as needed.  I also took the
> > liberty
> > > to replace some of the to-be-retired links (e.g. people.a.o is retiring
> > > soon, mail archives are being moved to pony, ICLA is now PDF based)
> > >
> > > John
> > >
> >
> >
> >
> > --
> > Daniel Cunha
> > https://twitter.com/dvlc_
> > http://www.tomitribe.com
> > http://www.tomitribe.io
> >
>
>
>
> --
> Christian Kaltepoth
> Blog: http://blog.kaltepoth.de/
> Twitter: http://twitter.com/chkal
> GitHub: https://github.com/chkal
>



--
Jason Porter
http://en.gravatar.com/lightguardjp
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

Mark Struberg-3
We should add a section that the person who applies the PR to our canonical repo have to verify that the PR only contains commits from the contributor himself. He basically needs to make sure that the contributor doesn't ship too much in the pull request (and thus trashing our code provenance chain).

ASF committers should not use PRs but directly commit to canonical repo themselves.
Of course it's fine to showcase ideas etc on github first. But you can simply cherry pick that over to master and push that to our repo yourself.

just my .02

txs and LieGrue,
strub





> On Tuesday, 26 July 2016, 1:26, Jason Porter <[hidden email]> wrote:
> > +1 PRs are much easier to work with, imo.
>
>
> On Sun, Jul 24, 2016 at 1:40 AM, Christian Kaltepoth <[hidden email]
>>  wrote:
>
>>  Hey John,
>>
>>  Great work!
>>
>>  +1 ;)
>>
>>  Christian
>>
>>  2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:
>>
>>  > Hi John,
>>  >
>>  > Greate job. I think that we really need to have that. It's much
> more easy
>>  > and cool to work with PR.
>>  > Easy way to review, easy way to fix changes, the contributor does not
>>  need
>>  > to attach a new patch just need to update the PR and we'll have
> feedbacks
>>  > more fast with PR Builder Plugin and comments by line on PR.
>>  >
>>  > I prefer this way, totally agree with your PR.
>>  >
>>  > +1 :)
>>  >
>>  > On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament
> <[hidden email]>
>>  > wrote:
>>  >
>>  > > All,
>>  > >
>>  > > I put together a first pass PR on an improved contributor
> workflow that
>>  > can
>>  > > leverage github PRs.  This is in addition to our existing patch
>>  approach.
>>  > >
>>  > > You can find the PR here, with the changes:
>>  > > https://github.com/apache/deltaspike/pull/61/files
>>  > >
>>  > > Using PRs gives us a bit of an advantage:
>>  > >
>>  > > - We don't lose the original author in the commit
>>  > > - We can run automated tests prior to the commit being merged in
>>  > >
>>  > > Please take a look, I'm happy to adjust as needed.  I also
> took the
>>  > liberty
>>  > > to replace some of the to-be-retired links (e.g. people.a.o is
> retiring
>>  > > soon, mail archives are being moved to pony, ICLA is now PDF
> based)
>>  > >
>>  > > John
>>  > >
>>  >
>>  >
>>  >
>>  > --
>>  > Daniel Cunha
>>  > https://twitter.com/dvlc_
>>  > http://www.tomitribe.com
>>  > http://www.tomitribe.io
>>  >
>>
>>
>>
>>  --
>>  Christian Kaltepoth
>>  Blog: http://blog.kaltepoth.de/
>>  Twitter: http://twitter.com/chkal
>>  GitHub: https://github.com/chkal
>>
>
>
>
> --
> Jason Porter
> http://en.gravatar.com/lightguardjp
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

Gerhard Petracek
Administrator
@mark: +1

regards,
gerhard



2016-07-27 10:53 GMT+02:00 Mark Struberg <[hidden email]>:

> We should add a section that the person who applies the PR to our
> canonical repo have to verify that the PR only contains commits from the
> contributor himself. He basically needs to make sure that the contributor
> doesn't ship too much in the pull request (and thus trashing our code
> provenance chain).
>
> ASF committers should not use PRs but directly commit to canonical repo
> themselves.
> Of course it's fine to showcase ideas etc on github first. But you can
> simply cherry pick that over to master and push that to our repo yourself.
>
> just my .02
>
> txs and LieGrue,
> strub
>
>
>
>
>
> > On Tuesday, 26 July 2016, 1:26, Jason Porter <[hidden email]>
> wrote:
> > > +1 PRs are much easier to work with, imo.
> >
> >
> > On Sun, Jul 24, 2016 at 1:40 AM, Christian Kaltepoth <
> [hidden email]
> >>  wrote:
> >
> >>  Hey John,
> >>
> >>  Great work!
> >>
> >>  +1 ;)
> >>
> >>  Christian
> >>
> >>  2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:
> >>
> >>  > Hi John,
> >>  >
> >>  > Greate job. I think that we really need to have that. It's much
> > more easy
> >>  > and cool to work with PR.
> >>  > Easy way to review, easy way to fix changes, the contributor does not
> >>  need
> >>  > to attach a new patch just need to update the PR and we'll have
> > feedbacks
> >>  > more fast with PR Builder Plugin and comments by line on PR.
> >>  >
> >>  > I prefer this way, totally agree with your PR.
> >>  >
> >>  > +1 :)
> >>  >
> >>  > On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament
> > <[hidden email]>
> >>  > wrote:
> >>  >
> >>  > > All,
> >>  > >
> >>  > > I put together a first pass PR on an improved contributor
> > workflow that
> >>  > can
> >>  > > leverage github PRs.  This is in addition to our existing patch
> >>  approach.
> >>  > >
> >>  > > You can find the PR here, with the changes:
> >>  > > https://github.com/apache/deltaspike/pull/61/files
> >>  > >
> >>  > > Using PRs gives us a bit of an advantage:
> >>  > >
> >>  > > - We don't lose the original author in the commit
> >>  > > - We can run automated tests prior to the commit being merged in
> >>  > >
> >>  > > Please take a look, I'm happy to adjust as needed.  I also
> > took the
> >>  > liberty
> >>  > > to replace some of the to-be-retired links (e.g. people.a.o is
> > retiring
> >>  > > soon, mail archives are being moved to pony, ICLA is now PDF
> > based)
> >>  > >
> >>  > > John
> >>  > >
> >>  >
> >>  >
> >>  >
> >>  > --
> >>  > Daniel Cunha
> >>  > https://twitter.com/dvlc_
> >>  > http://www.tomitribe.com
> >>  > http://www.tomitribe.io
> >>  >
> >>
> >>
> >>
> >>  --
> >>  Christian Kaltepoth
> >>  Blog: http://blog.kaltepoth.de/
> >>  Twitter: http://twitter.com/chkal
> >>  GitHub: https://github.com/chkal
> >>
> >
> >
> >
> > --
> > Jason Porter
> > http://en.gravatar.com/lightguardjp
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

John D. Ament
In reply to this post by Mark Struberg-3
Mark,

Good points.

On Wed, Jul 27, 2016 at 4:54 AM Mark Struberg <[hidden email]>
wrote:

> We should add a section that the person who applies the PR to our
> canonical repo have to verify that the PR only contains commits from the
> contributor himself. He basically needs to make sure that the contributor
> doesn't ship too much in the pull request (and thus trashing our code
> provenance chain).
>

This note should be added to both PR and Patch sections IMHO.  Same issue
could exist for patches.


>
> ASF committers should not use PRs but directly commit to canonical repo
> themselves.
> Of course it's fine to showcase ideas etc on github first. But you can
> simply cherry pick that over to master and push that to our repo yourself.
>

To me this is described in the "discussion" workflow, but I can call it out
a bit clearer.  Same should be true of patches though.  This would also be
for DS committers, not necessarily ASF committers (e.g. may be a committer
on other projects, just not DS, then a PR should be fine as well as a
patch).


>
> just my .02
>
> txs and LieGrue,
> strub
>
>
>
>
>
> > On Tuesday, 26 July 2016, 1:26, Jason Porter <[hidden email]>
> wrote:
> > > +1 PRs are much easier to work with, imo.
> >
> >
> > On Sun, Jul 24, 2016 at 1:40 AM, Christian Kaltepoth <
> [hidden email]
> >>  wrote:
> >
> >>  Hey John,
> >>
> >>  Great work!
> >>
> >>  +1 ;)
> >>
> >>  Christian
> >>
> >>  2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:
> >>
> >>  > Hi John,
> >>  >
> >>  > Greate job. I think that we really need to have that. It's much
> > more easy
> >>  > and cool to work with PR.
> >>  > Easy way to review, easy way to fix changes, the contributor does not
> >>  need
> >>  > to attach a new patch just need to update the PR and we'll have
> > feedbacks
> >>  > more fast with PR Builder Plugin and comments by line on PR.
> >>  >
> >>  > I prefer this way, totally agree with your PR.
> >>  >
> >>  > +1 :)
> >>  >
> >>  > On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament
> > <[hidden email]>
> >>  > wrote:
> >>  >
> >>  > > All,
> >>  > >
> >>  > > I put together a first pass PR on an improved contributor
> > workflow that
> >>  > can
> >>  > > leverage github PRs.  This is in addition to our existing patch
> >>  approach.
> >>  > >
> >>  > > You can find the PR here, with the changes:
> >>  > > https://github.com/apache/deltaspike/pull/61/files
> >>  > >
> >>  > > Using PRs gives us a bit of an advantage:
> >>  > >
> >>  > > - We don't lose the original author in the commit
> >>  > > - We can run automated tests prior to the commit being merged in
> >>  > >
> >>  > > Please take a look, I'm happy to adjust as needed.  I also
> > took the
> >>  > liberty
> >>  > > to replace some of the to-be-retired links (e.g. people.a.o is
> > retiring
> >>  > > soon, mail archives are being moved to pony, ICLA is now PDF
> > based)
> >>  > >
> >>  > > John
> >>  > >
> >>  >
> >>  >
> >>  >
> >>  > --
> >>  > Daniel Cunha
> >>  > https://twitter.com/dvlc_
> >>  > http://www.tomitribe.com
> >>  > http://www.tomitribe.io
> >>  >
> >>
> >>
> >>
> >>  --
> >>  Christian Kaltepoth
> >>  Blog: http://blog.kaltepoth.de/
> >>  Twitter: http://twitter.com/chkal
> >>  GitHub: https://github.com/chkal
> >>
> >
> >
> >
> > --
> > Jason Porter
> > http://en.gravatar.com/lightguardjp
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: [DISCUSS] PR Based Contributor Workflow

John D. Ament
All,

I've taken the input from Mark and applied it to the changes.

Please review at your convenience.  Assuming we're still settled on the
change, I can push this week. (e.g. since most people already voted +1 I'd
like to get a nod from Mark that he's good with the changes).

John

On Wed, Jul 27, 2016 at 6:37 AM John D. Ament <[hidden email]> wrote:

> Mark,
>
> Good points.
>
> On Wed, Jul 27, 2016 at 4:54 AM Mark Struberg <[hidden email]>
> wrote:
>
>> We should add a section that the person who applies the PR to our
>> canonical repo have to verify that the PR only contains commits from the
>> contributor himself. He basically needs to make sure that the contributor
>> doesn't ship too much in the pull request (and thus trashing our code
>> provenance chain).
>>
>
> This note should be added to both PR and Patch sections IMHO.  Same issue
> could exist for patches.
>
>
>>
>> ASF committers should not use PRs but directly commit to canonical repo
>> themselves.
>> Of course it's fine to showcase ideas etc on github first. But you can
>> simply cherry pick that over to master and push that to our repo yourself.
>>
>
> To me this is described in the "discussion" workflow, but I can call it
> out a bit clearer.  Same should be true of patches though.  This would also
> be for DS committers, not necessarily ASF committers (e.g. may be a
> committer on other projects, just not DS, then a PR should be fine as well
> as a patch).
>
>
>>
>> just my .02
>>
>> txs and LieGrue,
>> strub
>>
>>
>>
>>
>>
>> > On Tuesday, 26 July 2016, 1:26, Jason Porter <[hidden email]>
>> wrote:
>> > > +1 PRs are much easier to work with, imo.
>> >
>> >
>> > On Sun, Jul 24, 2016 at 1:40 AM, Christian Kaltepoth <
>> [hidden email]
>> >>  wrote:
>> >
>> >>  Hey John,
>> >>
>> >>  Great work!
>> >>
>> >>  +1 ;)
>> >>
>> >>  Christian
>> >>
>> >>  2016-07-23 18:14 GMT+02:00 Daniel Cunha <[hidden email]>:
>> >>
>> >>  > Hi John,
>> >>  >
>> >>  > Greate job. I think that we really need to have that. It's much
>> > more easy
>> >>  > and cool to work with PR.
>> >>  > Easy way to review, easy way to fix changes, the contributor does
>> not
>> >>  need
>> >>  > to attach a new patch just need to update the PR and we'll have
>> > feedbacks
>> >>  > more fast with PR Builder Plugin and comments by line on PR.
>> >>  >
>> >>  > I prefer this way, totally agree with your PR.
>> >>  >
>> >>  > +1 :)
>> >>  >
>> >>  > On Sat, Jul 23, 2016 at 1:04 PM, John D. Ament
>> > <[hidden email]>
>> >>  > wrote:
>> >>  >
>> >>  > > All,
>> >>  > >
>> >>  > > I put together a first pass PR on an improved contributor
>> > workflow that
>> >>  > can
>> >>  > > leverage github PRs.  This is in addition to our existing patch
>> >>  approach.
>> >>  > >
>> >>  > > You can find the PR here, with the changes:
>> >>  > > https://github.com/apache/deltaspike/pull/61/files
>> >>  > >
>> >>  > > Using PRs gives us a bit of an advantage:
>> >>  > >
>> >>  > > - We don't lose the original author in the commit
>> >>  > > - We can run automated tests prior to the commit being merged in
>> >>  > >
>> >>  > > Please take a look, I'm happy to adjust as needed.  I also
>> > took the
>> >>  > liberty
>> >>  > > to replace some of the to-be-retired links (e.g. people.a.o is
>> > retiring
>> >>  > > soon, mail archives are being moved to pony, ICLA is now PDF
>> > based)
>> >>  > >
>> >>  > > John
>> >>  > >
>> >>  >
>> >>  >
>> >>  >
>> >>  > --
>> >>  > Daniel Cunha
>> >>  > https://twitter.com/dvlc_
>> >>  > http://www.tomitribe.com
>> >>  > http://www.tomitribe.io
>> >>  >
>> >>
>> >>
>> >>
>> >>  --
>> >>  Christian Kaltepoth
>> >>  Blog: http://blog.kaltepoth.de/
>> >>  Twitter: http://twitter.com/chkal
>> >>  GitHub: https://github.com/chkal
>> >>
>> >
>> >
>> >
>> > --
>> > Jason Porter
>> > http://en.gravatar.com/lightguardjp
>> >
>>
>