From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web09.394.1575394830950596083 for ; Tue, 03 Dec 2019 09:40:31 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VlMYmuxu; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575394830; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=mNZ7N27AplfY7kTNEHEtnrFIblwBFUtlGsF+p+ONuJM=; b=VlMYmuxuqSyEFENfuP4EEY0yJjH3SKmhWSxdFZUI2osM8iHwBf2Dgvay6IAmFGq67bCH60 9XoB63yYAPFKpgL1SuAVqtF/2h5vatwA3F5OptA6kO9DZji8akCVgwoHigsla5RK4C4WO7 e8m1VQHmGnUgM4XLhKyGjp1xI/1WWSE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-413-VrsOk4gWMSuYh54iVzoDDw-1; Tue, 03 Dec 2019 12:40:26 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BD598477; Tue, 3 Dec 2019 17:40:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-183.ams2.redhat.com [10.36.117.183]) by smtp.corp.redhat.com (Postfix) with ESMTP id ABE1C1001B07; Tue, 3 Dec 2019 17:40:22 +0000 (UTC) Subject: Re: [edk2-devel] [Patch wiki v2] EDK II Dev Process: Change push to GitHub pull request To: devel@edk2.groups.io, michael.d.kinney@intel.com Cc: Sean Brogan , Bret Barkelew , Liming Gao , Andrew Fish , Leif Lindholm References: <20191202180131.876-1-michael.d.kinney@intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 3 Dec 2019 18:40:21 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191202180131.876-1-michael.d.kinney@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: VrsOk4gWMSuYh54iVzoDDw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 12/02/19 19:01, Michael D Kinney via Groups.Io wrote: > https://bugzilla.tianocore.org/show_bug.cgi?id=2315 > > Update wiki pages to remove instructions that directly > push to edk2/master. Update these instructions to create > a GitHub pull request from an EDK II Maintainers fork of I suggest "Maintainer's" > the edk2 repository. > > Cc: Sean Brogan > Cc: Bret Barkelew > Cc: Liming Gao > Cc: Andrew Fish > Cc: Laszlo Ersek > Cc: Leif Lindholm > Signed-off-by: Michael D Kinney > --- > EDK-II-Development-Process.md | 104 ++++++++++++++++-- > ...e-for-edk2-contributors-and-maintainers.md | 90 ++++++++++++--- > images/CreateGitHubPullRequest.png | Bin 0 -> 112984 bytes > images/CreateGitHubPullRequest2.png | Bin 0 -> 177099 bytes > 4 files changed, 171 insertions(+), 23 deletions(-) > create mode 100644 images/CreateGitHubPullRequest.png > create mode 100644 images/CreateGitHubPullRequest2.png > > diff --git a/EDK-II-Development-Process.md b/EDK-II-Development-Process.md > index a735e21..a205e83 100644 > --- a/EDK-II-Development-Process.md > +++ b/EDK-II-Development-Process.md > @@ -108,14 +108,102 @@ The maintainer process for the EDK II project > - Edit lines to have an 'r' to 'reword' the commit. This will > allow you to add the Reviewed-by attributions. > > -6. Push changes to the EDK II project repository > - - Pushing the integration branch directly to origin/master is > - preferred > - > - `$ git push origin HEAD:master` > - > - - Using the `--dry-run` parameter will show exactly what is going > - to be updated. Use this if you are paranoid. :) > +6. Push changes to the EDK II maintainer's fork of the EDK II project > + repository. > + - How to create a [GitHub fork](https://help.github.com/en/github/getting-started-with-github/fork-a-repo) > + - **NOTE:** A GitHub fork can also be created using the command line > + utility called [`hub`](https://github.com/github/hub/releases). The > + `hub` usage information can be found [here](https://hub.github.com/hub.1.html). > + > + - Add remote to the EDK II maintainer's fork of the EDK II project > + > + `$ git remote add https://github.com//edk2` I suggest adding a hyphen into "". (Note: if you agree, occurs in other locations in this patch, so please update those too.) Furthermore, I believe the remote reference (in the HTTPS scheme) carries a ".git" suffix. (It's not enforced by github.com but it's the pattern it copies to your clipboard you when you use the "clone this repo" feature on the webui.) IIRC > + > + - Push the integration branch to `/master`. Add the > + `--dry-run` parameter to show exactly what is going to be updated before > + pushing to a public branch. Use this if you are paranoid. :) > + > + `$ git push ` > + I think this is not fully consistent -- with the above "git push" command, the name will refer to both the local branch name and the new remote branch name. That's fine per se (and it does match point #7 below), but it does not match the paragraph just above. (Which tells us to push to /master.) I think it's simplest if we just drop that paragraph -- I think we should only refer to the integration branch in this article, and not to the fork's master branch. I think the "--dry-run" reference is no longer necessary either -- with the new process, the maintainer cannot directly push to the core repo, so the risk to mess up things has been greatly redudced. > +7. Create a GitHub pull request from the EDK II maintainer's > + `` to `edk2/master` setting the **`push`** label. > + If all checks pass, the changes are strict rebase merged. If the again, s/rebase/fast-forward/ -- but perhaps "strict rebase merged" is established github.com lingo. I'm unsure. > + **`push`** label is not set, then the checks are run, but no changes are > + merged. > + > + - How to create a [GitHub pull request](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request) > + - **NOTE:** A GitHub pull request can also be created using the command > + line utility called [`hub`](https://github.com/github/hub/releases). > + The `hub` usage information can be found [here](https://hub.github.com/hub.1.html). > + > + - Add the [TianoCore Bugzilla](https://bugzilla.tianocore.org/) issue > + number(s) resolved by the pull request to the pull request title. Hm, OK. That's useful. Thanks. > + > + - If `` is a patch series, then copy the patch #0 > + summary commit message into the pull request description. I suggest removing "commit message"; just "summary" sounds great. > + > + - If the EDK II Maintainer wants the patches from `` > + to be merged into `edk2/master` using strict rebase, then the **`push`** s/rebase/fast-forward/? > + label must be set. By default, pull requests to `edk2/master` are > + personal builds that perform checks and provide results, but no commits > + are automatically made to `edk2/master`. An EDK II Maintainer has the > + option of requesting a personal build first. If all checks pass, then > + the **`push`** label can be set and the pull request can be re-opened to > + request the patches to be committed to `edk2/master`. I think this encourages a suboptimal workflow. Based on your recent explanation, I do accept and agree that reopening and *then* setting the "push" label may make sense. But, that's only useful if there is a force-push in-between, with updates to the patches -- trivial updates by the maintainer, or more significant (and repeatedly reviewed!) updates by the original submitter / contributor. If there are no intervening updates, then the above pattern runs the checks twice, on the exact same codebase. I think that's not useful. So I'd propose dropping the last two sentences, or else fleshing out the workflow (which you described very well in the other email) in more detail. > + > + - Email notifications for pull requests, pushes, and check status results > + are enabled by watching the EDK II repository (https://github.com/tianocore/edk2). > + How to [Watch a GitHub repository](https://help.github.com/en/github/receiving-notifications-about-activity-on-github/watching-and-unwatching-repositories) > + > + - The figure below shows an example creating a GitHub pull request. The > + red box at the top shows that the edk2 repository is being watched. It > + also shows that GitHub observed that a new EDK II Maintainer branch > + `Bug_2315_Example_GitHub_Pull_Request` was pushed and is now available to > + create a pull request against `edk2/master` by selecting the button labeled > + `Compare & pull request`. > + > + ![Create GitHub Pull Request](images/CreateGitHubPullRequest.png) > + > + - The figure below shows the title and description of the pull request as > + well as the `Labels` button on the right that is used to enable/disable > + the **`push`** label. > + > + ![Create GitHub Pull Request Details](images/CreateGitHubPullRequest2.png) > + > +8. Resolve GitHub pull request issues. A pull request to `edk2/master` may > + fail for the following conditions: > + > + - A merge conflict is detected. The pull request remains open. The > + EDK II maintainer resolves all merge conflicts locally and does a forced > + push to ``. The pull request checks are > + automatically restarted. Please state that this is only acceptable if the conflict resolution is trivial. Otherwise the maintainer has to contact the contributor and ask the contributor to rebase the work on top of current master, and resubmit the patches (v2) to the list. (You may have implied all that in "locally", i.e. "outside of github.com", but I'd rather see it spelled out.) > + > + - The pull request fails the `PatchCheck.py` check. The pull request > + remains open. The EDK II maintainer resolves the `PatchCheck.py` issues > + locally and does a forced push to ``. The > + pull request checks are automatically restarted. Follow links to > + Azure Pipelines results to view the `PatchCheck.py` issues. Same as above. > + - The pull request fails Windows or Ubuntu checks. The pull request > + remains open. The EDK II maintainer resolves the checks locally and does > + a forced push to ``. The pull request checks > + are automatically restarted. Follow links to Azure Pipelines results > + to view the test results for checks that failed. Same as above. > + > + - The pull request submitter is not a member of the TianoCore EDK II > + Maintainers team and the **`push`** label is set. The pull request is > + ignored and automatically closed. > + > +9. Update [TianoCore Bugzilla](https://bugzilla.tianocore.org/) issue(s) > + resolved by the commit(s). > + > + - Add a pointer to the GitHub pull request (e.g. > + https://github.com/tianocore/edk2/pull/153). > + > + - Add the SHA hash range pushed to `edk2/master` (e.g. Pushed as > + cc6854506c..f8dd7c7018). > + > + - Mark BZ issues as Resolved/Fixed. Great. > > Updating your master branch > --------------------------- > diff --git a/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md b/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md Ah, thanks for updating this article too! > index 445461b..c7d0310 100644 > --- a/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md > +++ b/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers.md > @@ -907,27 +907,88 @@ Maintainer workflow > 11](#maint-11) accordingly. > > 14. § > - Time to push the patches to upstream master. Take a big breath :), > - and run > + Time to push the patches to upstream master. Take a big breath :) > > - ``` > - git push origin REVIEW_implement_foo_for_bar_vN:master > - ``` > - > - This will *attempt* to push the commits from your local > + The steps below *attempt* to push the commits from your local > `REVIEW_implement_foo_for_bar_vN` branch -- which is based off of > - your local master branch -- to the main github repo, *and* move the > - upstream master branch forward to the final commit among those. > + your local master branch -- to the main github repo using a GitHub > + pull request, *and* move the upstream master branch forward to the > + final commit among those. > > If it succeeds, you deserve an alcoholic (or non-alcoholic) drink > of your choice, you're done. > > - If it fails, then the reason is that *another maintainer* executed > - these steps in parallel, and moved forward the upstream master > + If a merge conflict is detected, then the reason is that *another maintainer* > + executed these steps in parallel, and moved forward the upstream master > branch *after* your [maintainer step 6](#maint-06), but before your > - [maintainer step 14](#maint-14). If github accepted your push in > - this case, that would cause the *other* maintainer's push to go > - lost. So, proceed to the next step. > + [maintainer step 14](#maint-14). If github accepted your pull request in > + this case, that may cause the *other* maintainer's pull request to fail. > + So, proceed to the next step. > + > + Create a GitHub pull request from the EDK II maintainer's > + `REVIEW_implement_foo_for_bar_vN` branch to `edk2/master` setting the > + **`push`** label. If all checks pass, the changes are strict rebase > + merged. If the **`push`** label is not set, then the checks are run, > + but no changes are merged. > + > + - How to create a [GitHub pull request](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/creating-a-pull-request) > + - **NOTE:** A GitHub pull request can also be created using the command > + line utility called [`hub`](https://github.com/github/hub/releases). > + The `hub` usage information can be found [here](https://hub.github.com/hub.1.html). > + > + Add the [TianoCore Bugzilla](https://bugzilla.tianocore.org/) issue > + number(s) resolved by the pull request to the pull request title. > + > + If `REVIEW_implement_foo_for_bar_vN` is a patch series, then copy the > + patch #0 summary commit message into the pull request description. > + > + If the EDK II Maintainer wants the patches from `REVIEW_implement_foo_for_bar_vN` > + to be merged into `edk2/master` using strict rebase, then the **`push`** > + label must be set. By default, pull requests to `edk2/master` are > + personal builds that perform checks and provide results, but no commits > + are automatically made to `edk2/master`. An EDK II Maintainer has the > + option of requesting a personal build first. If all checks pass, then > + the **`push`** label can be set and the pull request can be re-opened to > + request the patches to be committed to `edk2/master`. > + > + - Email notifications for pull requests, pushes, and check status results > + are enabled by watching the EDK II repository (https://github.com/tianocore/edk2). > + How to [Watch a GitHub repository](https://help.github.com/en/github/receiving-notifications-about-activity-on-github/watching-and-unwatching-repositories) > + > + Resolve GitHub pull request issues. A pull request to `edk2/master` may > + fail for the following conditions: > + > + - A merge conflict is detected. The pull request remains open. The > + EDK II maintainer resolves all merge conflicts locally and does a forced > + push to `REVIEW_implement_foo_for_bar_vN`. The pull request checks are > + automatically restarted. > + > + - The pull request fails the `PatchCheck.py` check. The pull request > + remains open. The EDK II maintainer resolves the `PatchCheck.py` issues > + locally and does a forced push to `REVIEW_implement_foo_for_bar_vN`. The > + pull request checks are automatically restarted. Follow links to > + Azure Pipelines results to view the `PatchCheck.py` issues. > + > + - The pull request fails Windows or Ubuntu checks. The pull request > + remains open. The EDK II maintainer resolves the checks locally and does > + a forced push to `REVIEW_implement_foo_for_bar_vN`. The pull request checks > + are automatically restarted. Follow links to Azure Pipelines results > + to view the test results for checks that failed. > + > + - The pull request submitter is not a member of the TianoCore EDK II > + Maintainers team and the **`push`** label is set. The pull request is > + ignored and automatically closed. > + > + Update [TianoCore Bugzilla](https://bugzilla.tianocore.org/) issue(s) > + resolved by the commit(s). > + > + - Add a pointer to the GitHub pull request (e.g. > + https://github.com/tianocore/edk2/pull/153). > + > + - Add the SHA hash range pushed to `edk2/master` (e.g. Pushed as > + cc6854506c..f8dd7c7018). > + > + - Mark BZ issues as Resolved/Fixed. > Instead of duplicating this information, can we: - somehow highlight or factor out the new sections added to the "EDK-II-Development-Process.md" article, - briefly reference that new passage, in maintainer step 14 of the unkempt guide? I think the only "extra" help we need to provide here is, "please substitute `REVIEW_implement_foo_for_bar_vN` for ``". > 15. § > Repeat the following steps: > @@ -949,4 +1010,3 @@ Maintainer workflow > > - [maintainer step 12](#maint-12) through [maintainer step > 14](#maint-14) -- rebuild, optionally retest, try pushing again. > - Is this empty line being removed on purpose? Hmm, yes, that makes sense, it's the last line in the file; it should not be empty. I think the only non-trivial request I've made is to highlight or otherwise make "well-addressible" the new procedure in the official workflow article (EDK-II-Development-Process.md), and then add a "shallow" pointer towards that section to the unkempt guide. With a hint for the branch substitution. Please update what you agree with from my points; I'm already happy with this version. Reviewed-by: Laszlo Ersek Thanks! Laszlo