Project:GitHub/Pull requests

From Gentoo Wiki
Jump to: navigation, search

GitHub labels explained

Since GitHub does not provide for detailed descriptions for labels, here's a quick run-down.

Assignment status

assigned -- at least one assignee (package maintainer) for each altered package was found on GitHub and mentioned in the assignment comment. GitHub team does not need to look into assigning the pull request.

need assignment -- the pull request could not be fully assigned automatically (inverse of the above). Someone needs to look into pinging the appropriate maintainer.

Additional assignment information

maintainer-needed -- the pull request alters at least one package that does not have a maintainer. Any developer can look into merging that part. It is a good idea to ask the user about proxy-maintaining it.

new package -- the pull request adds a new package. Either the user should proxy-maintain it, or another developer needs to take it.

self-maintained -- the pull request author is one of the maintainers of each of the packages altered in the pull request. Any proxy-maint member may review it without having to obtain permission from other developers.

Pull request status (progress)

do not merge -- the pull request is not supposed to be merged (or reviewed, unless specifically asked to). This is intended for early RFCs and CI runs. The automated assignment for this pull request is disabled.

help needed -- the author has requested help from other developers in fixing the pull request. If you have a minute, please look into it.

test request -- the pull request is waiting for testing, usually by the submitter. If otherwise approved, the pull request can be merged as soon as the issues are confirmed to be fixed.

work in progress -- the work on the pull request is still ongoing. It will be assigned, and the maintainer are free to provide early review. However, it should be merged until the label is removed.

Merging notes

fix on merge -- the pull request is mostly good, except for a few minor issues. If the submitter did not fix them yet, the developer can merge the pull request provided that he fixes them himself while doing it.

revbump on merge -- the pull request is mostly good, except that the package(s) need(s) to be revbumped. If the submitter did not do that yet, the developer can merge the pull request provided that he introduces the necessary revbump (+ ~arch keywording, potentially keeping the old version if it is stable).

Pull request assignment

The new pull requests are automatically assigned using a cronjob run every 4 minutes. The assignment script leaves a comment @-mentioning appropriate developers/teams and setting labels appropriately.

The assignment is performed based on the list of changed files. The script reads the current maintainers from the metadata.xml files, and maps them to GitHub developers/teams. It should be noted that the files from the current ::gentoo version are used rather than ones from the pull request.

The following additional rules apply:

  • New and unmaintained packages are assigned to Project:Proxy Maintainers as candidates for proxy maintenance.
  • Eclass and profile changes are not assigned automatically, and instead assumed to be handled by other assignees of the pull request (if any).
  • Pull requests involving packages with more than 5 unique sets of maintainers are not assigned automatically (to prevent spam).
  • Packages whose maintainers have no GitHub account (as in, none of the maintainers do have one) are marked as non-assignable.

If the script was able to find at least one assignee for each of the modified packages, the pull request is marked assigned. Otherwise it is marked can't assign.

In order to prevent automatic assignment, please set the assigned label or add at least one explicit assignee. In order to have the pull request re-assigned, remove all assignees (if any) and the appropriate label. The script will automatically remove the previous assignment comment.

Maintainer address verification

The assignment script additionally verifies that the e-mail addresses of the maintainers are registered on the Gentoo Bugzilla. If an unregistered address is found, the assignment comment contains a verbose warning about it.

When the submitter fixes the issue, or the package list changes, you can remove the assigned label in order to force reassignment. This will also repeat the verification.

Pull request CI

All pull requests are subject to CI runs that run pkgcheck on the whole repository. The CI script is run every 20 minutes, and processes a single pull request (and leaves ETAs on all remaining pull requests).

The CI run consists of pregenerating caches and running pkgcheck on the whole repository. This makes it possible to catch indirect dependency breakages resulting from the changes. If any issues are found, the result is compared to the repository state on which the PR was last rebased. The report indicates appropriately which issues are new to the pull request, and which are persisting (i.e. could be fixed by rebasing).

In order to rerun the CI, force push new commits over the pull requests. The script will detect changed commit identifier, and rerun the checks. it is currently not possible to disable CI on a pull request.

Developers are encouraged to file pull requests in order to CI-test their package removals.

Warning
The CI checks only report severe problems. Furthermore, pkgcheck is imperfect (yet very fast) and may fail to notice some subtle breakages (especially if USE flag masking/forcing) is involved. CI is not an excuse not to run repoman full, at least for the packages being committed.

Pull request reviews

Who is eligible to review?

Everyone can review pull requests. However, please make sure that your comments are correct and on topic. It is a bad experience for our users if they are confused by incorrect suggestions, and then asked to revert them.

Pull request reviews are non-binding. They are supposed to improve the pull request and make it more likely to be accepted as-is by the assignee. The assignee may choose not to accept a pull request independent of positive or negative reviews.

How to review?

A different levels of review are appropriate to the different kinds of commits. In particular:

  • if a pull request affects a small part of ebuild (adding a patch, fixing a small bug), don't request the author to fix irrelevant bugs (unless he explicitly wants to).
  • If a pull request does a version bump, don't ask the author to fix minor bugs that are unchanged since the previous version (unless he explicitly wants to).
  • If a pull request does an EAPI bump, it might be reasonable to request changes semi-relevant to the EAPI bump.
  • If a pull request submits a new ebuild, it is highly desirable to bring it to top-quality before being committed.
  • If a pull request requests proxied maintenance of an existing ebuild, it is desirable to ask the new maintainer to fix some bugs, however not necessarily all of them at once.

Be clear in your comments. Avoid unnecessary nitpicking, and even more importantly — avoid wars between developers over ebuild style. If something is purely your preference or recommendation, state that clearly and explain why. If you're not sure, say that, suggest how to check. Feel free to @-mention other developers or teams (QA for technical issues, licenses for license doubts, python for Python doubts, etc.) to get help.

For simple additional changes (revbump, typo fix), don't request the submitter to update the pull request. Instead, leave a comment stating what is wrong and that it will be fixed by the person merging it, so that the submitters knows how to avoid the problem in the future. Add a 'revbump on merge' or 'fix on merge' label appropriately, so that people won't merge it accidentally.

How to submit reviews?

Preferably, use the GitHub review feature to submit reviews. This makes it possible to combine all comments into a single mail rather than sending one mail for each comment.

You can either review the final result of the pull request (Files changed tab) or single commits separately. In order to add a comment, click a line to be commented on, write the comment and use Start a review. This will cause this and your further review comments to be marked pending — which also allows you to modify or remove them before sending.

Please note that GitHub stores your pending, so you can freely navigate through multiple commits and tabs, or even leave the pull request without losing your input. Once ready to submit, navigate to the Files changed tab and use the Review changes button on top.

You can write the general comments here (e.g. applying to the whole pull request, if necessary) and additionally decide if you approve or request changes to the pull request. Neither is obligatory — you can also use the review feature to leave a few neutral comments in a single mail.

Merging pull requests

Warning
Do not use the big green Merge pull request button. The repository is a mirror, so the changes will be discarded. You need to merge the pull request into git.gentoo.org instead.

Pull request approval

Please remember that pull requests follow the same rules as other commits to the Gentoo repository. Therefore, you can only merge a pull request if you would be permitted to commit the changes to the Gentoo repository. Furthermore, you take responsibility for the changes you are merging.

In particular, changes affecting packages maintained by other developers/projects may require their permission to be merged. If the maintainers do not use GitHub, you may need to use other media to contact them and obtain approval for the merge.

Common tips

Adding the git remote

Most of the methods listed here require you to fetch pull requests from GitHub. In order to do that most conveniently, create a new remote for GitHub and enable fetching of pull request refs:

user $git remote add github https://github.com/gentoo/gentoo.git
user $git config --add remote.github.fetch '+refs/pull/*/head:refs/remotes/github/pull/*'

Afterwards, each call to:

user $git fetch github

will fetch new pull requests and expose them as github/pull/NNNN refs.

Closing pull requests

There are three ways of closing a pull request:

  1. via a merge commit,
  2. via a Closes tag,
  3. manually.

If you do a merge commit involving unmodified upstream commits, GitHub will detect commit id match and close the pull request automatically.

Alternatively, you could add a Closes tag to your commit message to make GitHub close the pull request:

CODE Example git commit message
dev-foo/bar: fix fnord, #mmmmmm

Closes: https://github.com/gentoo/gentoo/pull/NNNN

Finally, if you did neither of that you need to close the pull request manually.

Inline merges

Using git cherry-pick

In order to cherry-pick all new commits from the pull request, do:

user $git cherry-pick master..github/pull/NNNN

Afterwards, edit the commits as necessary, rebase and sign them and push.

Using git am

It is also possible to use GitHub generated patches to avoid adding the GitHub repository locally. It should be noted, however, that git am is weaker than git cherry-pick at conflict resolution.

user $wget -O - https://github.com/gentoo/gentoo/pull/NNNN.patch | git am

Doing a true merge

Warning
True merges should be only used whenever there is a good reason to do so. Expect a long bikeshed following each one.

In order to create a true merge commit for a pull request, do:

user $git merge -S github/pull/NNNN

Please do not use the default merge commit message. Instead, modify it to adhere to the Gentoo commit message rules, indicating the most important changes included in the merged branch and include the pull request URL in it.

Remember that you need to explicitly make git preserve merge commits when rebasing, e.g.:

user $git pull --rebase=preserve -S