osdir.com


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Revisit the proposal to use github PR


To clarify my position: Github PRs are great for *reviewing* code, and the
commentary is much easier to follow imo. But for *merging* code, esp into
our multi-branch strategy, PRs don't fit well, unless there's some
technique I and perhaps others are unaware of.

On Thu, Dec 13, 2018 at 9:47 AM Ariel Weisberg <ariel@xxxxxxxxxxx> wrote:

> Hi,
>
> I'm not clear on what github makes worse. It preserves more history then
> the JIRA approach. When people invitably force push their branches you
> can't tell from the link to a branch on JIRA. Github preserves the comments
> and force push history so you know what version of the code each comment
> applied to. Github also tracks when requests for changes are acknowledged
> and resolved.  I have had to make the same change request many times and
> keep track independently whether it was resolved. This has also resulting
> in mistakes getting merged when I missed a comment that was ignored.
>
> Now that github can CC JIRA that also CCs to commits@. It's better then
> JIRA comments because each comment includes a small diff of the code the
> comment applies to. To do that in JIRA I have to manually link to the code
> the PR and most people don't do that for every comment so some of them are
> inscrutable after the fact. Also manually created links sometimes refer to
> references that disappear or get force pushed. It's a bit tricky to get
> right.
>
> To me arguing against leveraging a better code review workflow (whether
> github or some other tool) is like arguing against using source control
> tools. Sure the filesystem and home grown scripts can be used to work
> around lack of source control, but why would you?
>
> I see two complaints so far. One is that github PRs encourage nitpicking.
> I don't see tool based solution to that off the cuff. Another is that
> github doesn't by default CC JIRA. Maybe we can just refuse to accept
> improperly formatted PRs and look into auto rejecting ones that don't refer
> to a ticket?
>
> Ariel
>
> On Thu, Dec 13, 2018, at 12:20 PM, Aleksey Yeschenko wrote:
> > There are some nice benefits to GH PRs, one of them is that we could
> > eventually set up CircleCI hooks that would explicitly prevent commits
> > that don’t pass the tests.
> >
> > But handling multiple branches would indeed be annoying. Would have to
> > either submit 1 PR per branch - which is both tedious and non-atomic -
> > or do a mixed approach, with a PR for the oldest branch, then a manual
> > merge upwards. The latter would be kinda meh, especially when commits
> > for different branches diverge.
> >
> > For me personally, the current setup works quite well, and I mostly
> > share Sylvain’s opinion above, for the same reasons listed.
> >
> > —
> > AY
> >
> > > On 13 Dec 2018, at 08:15, Sylvain Lebresne <lebresne@xxxxxxxxx> wrote:
> > >
> > > Fwiw, I personally find it very useful to have all discussion, review
> > > comments included, in the same place (namely JIRA, since for better or
> > > worse, that's what we use for tracking tickets). Typically, that means
> > > everything gets consistently pushed to the  commits@ mailing list,
> which I
> > > find extremely convenient to keep track of things. I also have a theory
> > > that the inline-comments type of review github PR give you is very
> > > convenient for nitpicks, shallow or spur-of-the-moment comments, but
> > > doesn't help that much for deeper reviews, and that it thus to favor
> the
> > > former kind of review.
> > >
> > > Additionally, and to Benedict's point, I happen to have first hand
> > > experience with a PR-based process for a multi-branch workflow very
> similar
> > > to the one of this project, and suffice to say that I hate it with a
> > > passion.
> > >
> > > Anyway, very much personal opinion here.
> > > --
> > > Sylvain
> > >
> > >
> > > On Thu, Dec 13, 2018 at 2:13 AM dinesh.joshi@xxxxxxxxx.INVALID
> > > <dinesh.joshi@xxxxxxxxx.invalid> wrote:
> > >
> > >> I've been already using github PRs for some time now. Once you
> specify the
> > >> ticket number, the comments and discussion are persisted in Apache
> Jira as
> > >> work log so it can be audited if desired. However, committers usually
> > >> squash and commit the changes once the PR is approved. We don't use
> the
> > >> merge feature in github. I don't believe github we can merge the
> commit
> > >> into multiple branches through the UI. We would need to merge it into
> one
> > >> branch and then manually merge that commit into other branches. The
> big
> > >> upside of using github PRs is that it makes collaborating a lot
> easier.
> > >> Downside is that it makes it very difficult to follow along the
> progress in
> > >> Apache Jira. The messages that github posts back include huge diffs
> and are
> > >> aweful.
> > >> Dinesh
> > >>
> > >>    On Thursday, December 13, 2018, 1:10:12 AM GMT+5:30, Benedict
> Elliott
> > >> Smith <benedict@xxxxxxxxxx> wrote:
> > >>
> > >> Perhaps somebody could summarise the tradeoffs?  I’m a little
> concerned
> > >> about how it would work for our multi-branch workflow.  Would we open
> > >> multiple PRs?
> > >>
> > >> Could we easily link with external CircleCI?
> > >>
> > >> It occurs to me, in JIRA proposal mode, that an extra required field
> for a
> > >> permalink to GitHub for the patch would save a lot of time I spend
> hunting
> > >> for a branch in the comments.
> > >>
> > >>
> > >>
> > >>
> > >>> On 12 Dec 2018, at 19:20, jay.zhuang@xxxxxxxxx.INVALID wrote:
> > >>>
> > >>> It was discussed 1 year's ago:
> > >> https://www.mail-archive.com/dev@xxxxxxxxxxxxxxxxxxxx/msg11810.html
> > >>> As all Apache projects are moving to gitbox:
> > >> https://reference.apache.org/committer/github, should we revisit
> that and
> > >> change our review/commit process to use github PR?A good example is
> > >> Spark:"Changes to Spark source code are proposed, reviewed and
> committed
> > >> via Github pull requests" (https://spark.apache.org/contributing.html
> ).
> > >>> /jay
> > >>
> > >>
> > >> ---------------------------------------------------------------------
> > >> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
> > >> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxxxx
> > >>
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
> > For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxxxx
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxxxx
> For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxxxx
>
>