Hi Johan,

Your points all make sense to me. In that case, we need to make the policy for approving PRs such that it meets the code review criteria (which is still something we need to formalize) -- meaning that a "Reviewer" (or more in the case of larger changes) needs to review it and not just any "Committer". If we do that, then I there should be no cause for rejected reviews, since it will have already been "Reviewed".

-- Kevin


Johan Vos wrote:
That is the difficult point indeed.
But why would a PR to OpenJFX be rejected after it was approved in the github mirror? I would assume the main reason for this is because the PR did not what it was supposed to do. In that case, it makes sense to remove the commits from the github mirror as well.

I think the main thing here is that we need to be very serious about reviewing and accepting PR's in the github mirror. Accepting a PR in github does not require the *formal* process of creating webrevs etc, but it requires discussion about the issue with reviewers of OpenJFX. We have to minimize the number of times an edge case occurs, in which the discussion was pro PR first, but after it got merged into "development" new arguments are brought up against the PR. I think it would be good to have sort of a post-mortem analysis in case this happens, in order to prevent it from happening again. But as I said, if it does happen, it probably has good reasons and in that case we have to remove it from the development branch as well.

I think the more common case would be that an issue is fixed on the github mirror, but not yet accepted (nor rejected) in OpenJFX, so there will be some time lag between the PR acceptance and the integration in OpenJFX. But this should not be a problem, as long as the following scenario is the main flow:

The github master branch is always synced with OpenJFX, and never gets modified by other commits. The github "development" branch is where we accept PR's, that can then be send upstream. Changes from "master" are regularly merged into "development". The moment an accepted PR makes it into OpenJFX, it will be synced into "master" and merged into "development" where the merge happens silently as there are no conflicts (since development already has this code).

Does that make sense?

- Johan

On Wed, Feb 28, 2018 at 12:51 AM Kevin Rushforth <kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>> wrote:



    Nir Lisker wrote:

        Johan's thinking was to allow Committers to approve the PR on
        GitHub -- meaning they could be merged on GitHub before an
        actual Review has happened. Are you proposing to change that?


    What if the PR is rejected at review? We'll end up with conflicts
    between the repos. And supposed someone works on a different fix
    and uses the rejected PR code, how will that be committed?

    Good questions; maybe Johan has some thoughts as to how to
    mitigate this?


    -- Kevin



    On Wed, Feb 28, 2018 at 12:25 AM, Kevin Rushforth
    <kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com>>
    wrote:

        This seems a good start in formalizing the process. It will
        need a little tweaking in a couple of areas.

        Regarding JBS access, even though I want to relax the
        requirement to become an Author (get a JBS account), it will
        likely end up somewhere between "an intention to contribute"
        and "two sponsored contributions, already reviewed and
        committed". Even without this, there will necessarily be a
        gap in time between "I want to work on a bug" and getting a
        JBS account. So there is value in encouraging people to clone
        the GitHub sandbox, "kick the tires", make a PR to get
        feedback, etc., before they can access JBS directly (or even
        while waiting for their OCA to be processed, but no PRs in
        that case). Something to take into account.

        Regarding review, we will need a bit more discussion on that.
        I like the idea of the PR being logged in JBS once it is
        ready to be reviewed. Johan's thinking was to allow
        Committers to approve the PR on GitHub -- meaning they could
        be merged on GitHub before an actual Review has happened. Are
        you proposing to change that? It might have some advantages,
        but it could also make it harder in other areas. I'd like to
        hear from Johan on this. This reminds me that we need to
        continue the discussion on the general "Review" policy, as it
        is relevant here.

        As for whether it is merged into GitHub, I don't have a
        strong opinion on that. As you say it will be pulled into the
        mirror anyway (along with changes from reviews happening in
        JBS that don't first go through the sandbox), so maybe it
        doesn't matter? On the other hand there might be advantages
        to getting it into the mainline of the sandbox early? Hard to
        say.

        -- Kevin


        Nir Lisker wrote:
        Iv'e given the pipeline some thought. I'm purposely ignoring
        current role names (Author, Contributor...). My suggestions:

        Potential contributor wants to contribute...

        1. Formal process
          a. If the issue is not in the JBS, they submit it via
        bugreport.
          b. They send an email on the mailing list regarding the
        issue (a plan, question on how to approach etc.)
          c. If the above effort is "deemed worthy" (whatever that
        means), and they have signed the OCA, and they then they get
        access to JBS. If they've given a GitHub account, they get
        access to GitHub PRs.
          d. Discussion from the mailing list is copied/linked to
        the JBS issue. Maybe if it's their issue (step a) then the
        Reporter field can change to them.

        This ensures that:
        * There's 1 entry point.
        * GitHub and JBS identities are linked (GitHub identity is
        verified).
        * Being able to comment on JBS is easier - instead of
        requiring 2 commits it requires good intentions(?)
        * Not every person on the planet has access to JBS.

        2. Work process
          a. They fork the GitHub repo.
          b. They create a PR with a 2-way link to/from JBS (similar
        to  current webrevs - JBS links).
          c. Discussion specifically on the patch should happen in
        the PR thread. General info on the bug (affected versions
        etc.) still happens in JBS.
          d. After the patch had been reviewed, it is committed to
        the Oracle repo. Since GitHub mirrors Oracle I don't think
        it matters if the patch is merged into GitHub.

        This ensures that:
        * It's easier to start working because the GiutHub repo is
        more convenient than the Oracle repo currently.
        * PRs and JBS issues are mutually aware.
        * The submit -> review -> commit process is streamlined.

        We pay a synchronization price for having 2 repos and 2 bug
        trackers. This is what I could come up with.

        - Nir

        On Fri, Feb 16, 2018 at 1:14 AM, Kevin Rushforth
        <kevin.rushfo...@oracle.com
        <mailto:kevin.rushfo...@oracle.com>> wrote:



            Johan Vos wrote:


            On Thu, Feb 15, 2018 at 4:09 AM Kevin Rushforth
            <kevin.rushfo...@oracle.com
            <mailto:kevin.rushfo...@oracle.com>> wrote:

            A global reference in JBS would indeed be very good to
            track back the work in a PR to a real issue. It can
            also be very useful as there are many existing issues
            in JBS that can be referred to in future work.

            The only issue I see is that in order to create an
            issue in JBS, you need to have "author" status, so not
            everyone can do this? Given the idea that developers
            who want to create a PR also need to sign an OCA, it
            might make sense to somehow combine the administration?

            I don't think we can combine this, but I hope to be able
            to relax the requirements to become an Author a little.
            The current guidelines are 2 sponsored contributions [1].

            Pending appointment as an Author, it isn't hard to
            submit a bug via http://bugreport.java.com/ . If there
            is a test case, it usually gets moved to the JDK project
            within a day or so (and I can move them sooner, if
            needed). The bigger bother is that you can't comment in
            JBS on a bug you didn't create, but once the bug is
            there, you can work on it in GutHub and/or send email to
            the list. I'll also add any comments from contributors
            who are not yet Authors to any bug report.

            -- Kevin

            [1] http://openjdk.java.net/projects/#project-author


- Johan



Reply via email to