As it's getting a bit exhaustive to repeat these bits on GitHub
over and over again,
I though I summarize a couple of notes that hopefully are
interesting for the DLang maintainers.
Please take this as a friendly summary and personal advice of
most GH-related process improvements that have happened over the
last three months. It's (mostly) not intended as a rule book.
There's a (new) formal "Approve" button
=======================================
- GH formalized the review process
- Please use a the "approve" feature instead of LGTM comment
This is important, because all PRs require an approval!
So please approve before you auto-merge (it won't work otherwise).
In the same way GH also allows to attach a "request for changes"
on a PR.
-> If you have a serious remark, please use the "request a
change" feature instead of a plain comment as these "request"
will be nicely shown as warning at the end of a PR (GH will even
block the merge of a PR until the criticized bits are fixed).
Moreover "changes requested" will also be shown on the summary of
all PRs and helps others when they browse the PR list.
Review workflow (squashed commits & write access to PRs)
========================================================
The ideal workflow is that a PR gets commits appended until its
final approval, s.t. you only need to review the added changes.
GitHub has two new features to help us here:
1) Commit squashing
-------------------
- All commits get squashed into one commit before the merge
- This is enabled for all DLang repos
- "auto-merge-squash" does squashing as auto-merge behavior
More infos: https://github.com/blog/2141-squash-your-commits
(Before you ask: Yes, CyberShadow had some initial concerns [1]
about this feature, but we were able to address them and digger
didn't break)
[1]
http://forum.dlang.org/post/[email protected]
2) Write access to PRs
----------------------
- This is an _awesome_ feature that hasn't been used much so far
- It allows maintainer to do those nitpicks themselves (squashing
all commits, fixing typos, ...) instead of going with the usual
ping-pong cycle
- It's enabled by default for new PRs
- If someone turned it accidentally off, it's really okay to ask
him/her as this is a massive time saver
I can only recommend to add the following alias to your
`~/.gitconfig`:
```
[alias]
pr = "!f() { git fetch -fu ${2:-upstream}
refs/pull/$1/head:pr/$1 && git checkout pr/$1; }; f"
```
With this you can checkout a PR like this:
git pr 5150
And in case you are as lazy as I am and don't want to enter the
branch to push manually, you can use my small snippet:
```
#!/bin/bash
tmpfile=$(mktemp)
repoSlug=$(git remote -v | grep '^upstream' | head -n1 | perl
-lne 's/github.com:?\/?(.*)\/([^.]*)([.]git| )// or next; print
$1,"/",$2')
prNumber=$(git rev-parse --abbrev-ref HEAD | cut -d/ -f 2)
curl -s
https://api.github.com/repos/${repoSlug}/pulls/${prNumber} >
$tmpfile
trap "{ rm -f $tmpfile; }" EXIT
headRef=$(cat $tmpfile | jq -r '.head.ref')
headSlug=$(cat $tmpfile | jq -r '.head.repo.full_name')
git push -f [email protected]:${headSlug} HEAD:${headRef}
```
For more details on pushing changes to a PR, please see this
article:
More infos:
https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork
@all: I recently discovered the nice pattern to prepend sth. like
"[SQUASH]" to appended commit messages. This helps maintainers as
well ;-)
Auto-Merge
==========
In case you were wondering about the new "auto-merge" and
"auto-merge-squash" labels.
This is the new auto-merge system that takes the status of all
required CIs into account (the auto-tester tries the merge after
its test passed, but doesn't look out for other CIs.)
You can toggle a PR for auto-merge by simply adding this label or
for the keyboard-enthusiasts:
press "l", press "a" and hit enter.
Warning: this new auto-merge system is officially still WIP
because:
- "auto-merge"-labelled PRs aren't yet prioritized on the
auto-tester [1]
- We would like to set _all_ CIs to enforced
For more details, please see [2]:
[1] https://github.com/dlang-bots/dlang-bot/pull/50
[2] https://github.com/dlang-bots/dlang-bot#auto-merge-wip
CI status
=========
Martin and I have worked quite a bit on making the CIs more
reliable. If you see a transient error, please let us know!
In any other case, the "red cross" on a CI has a meaning -
surprise!
Here's a small summary of the tasks of the "new" CIs:
1) CircleCi
- Tests are run with code coverage enabled
- Phobos only: Over the last weeks, we have tried to unify Phobos
to share a more common coding style. The CI is in place to ensure
this status quo for the future
- Phobos only: All unittest blocks are separated from the main
file and compiled separately. This helps to ensure that the
examples on dlang.org that are runnable and don't miss any
imports.
It should be fairly trivial to find out the regarding error here.
Just click on the "CircleCi" link and open the red tab that is
marked as failing and scroll down to the error message.
2) ProjectTester
- A couple of selected projects are run to ensure that no
regressions are introduced
- Martin & Dicebot are currently working on a huge improvement of
this system:
Preview:
https://ci2.dawg.eu/blue/organizations/jenkins/dlang/detail/dlang/120/pipeline/
More infos:
http://forum.dlang.org/post/[email protected]
As we keep adding CIs it's also planned to move the CircleCi
tasks to the new CI system once it's ready:
https://github.com/Dicebot/dlangci/issues/18
Code coverage
=============
If you are too lazy to click on the annotated coverage link, you
can install the browser extension which will enrich the PR with
code coverage information:
https://github.com/codecov/browser-extension
Unfortunately `codecov/project` is currently not exact as CodeCov
doesn't handle the `parent_ref` correctly. For details:
https://github.com/dlang/phobos/pull/5197
https://github.com/dlang/phobos/pull/5198
https://github.com/codecov/support/issues/360
Milestones
==========
- They are intended to show which PRs are basically ready to be
shipped OR should be shipped soon
- Please use them whenever you see nothing blocking a PR (except
for a final merge decision)
- Ideally about one week before the close of the merge window
(i.e. the end of the milestone), the focus on the remaining items
of the current milestone should be increased
So far it worked well in the past and present:
2.072: https://github.com/dlang/phobos/milestone/9?closed=1
2.073: https://github.com/dlang/phobos/milestone/11?closed=1
2.074: https://github.com/dlang/phobos/milestone/12
Dlang-Bot
=========
- The bot [1] is getting smarter & smarter every week
- WebDrake and I are working on providing a default greeting
message with some useful pointers [2]
What it does atm:
- Shows whether a PR will be part of the changelog ('X' means NO,
'✔' means YES)
- Auto-merges a PR once all required CI pass
- Closes a Bugzilla issue if the respective PR has been merged
- Moves a Trello card if the respective PR has been merged
- Cancels stale Travis builds (this helps to free the Travis
queue at dlang/dmd)
What is planned for the future:
- Automatically remove "needs work" / "needs rebase" on a push
event [3]
- Recognize common labels in the title (e.g. "[WIP]") [4]
- Automatically tag inactive and unmergable PRs [5]
- Add a "needs review" label to unreviewed PRs with passing CIs
[6]
- Show auto-detectable warnings (e.g. regression PR that isn't
targeted at 'stable') [7]
Some of this features may be subjective. If there's anything you
miss in particular or annoys you, please let us know -> [1].
[1] https://github.com/dlang-bots/dlang-bot
[2] https://github.com/dlang-bots/dlang-bot/pull/44
[3] https://github.com/dlang-bots/dlang-bot/pull/51
[4] https://github.com/dlang-bots/dlang-bot/pull/56
[5] https://github.com/dlang-bots/dlang-bot/pull/55
[6] https://github.com/dlang-bots/dlang-bot/pull/52
[7] https://github.com/dlang-bots/dlang-bot/pull/57
Changelog
=========
There's a new changelog format [1] for all three repos (DMD,
Druntime, Phobos) in place. This format is a file per changelog
entry, which has the advantage that a changelog can be added to a
PR _without_ creating merge conflicts.
Hence, take advantage of this feature and don't merge a PR
without a changelog entry ;-)
Moreover, DAutoTest will soon show a preview of the generated
changelog [2].
Last but not least the separation in the DMD repo between
"compiler changes" and "language changes" has been moved, s.t.
the "changelog" repo at DMD just contains compiler changes and
the changelog at dlang.org [3] is supposed to contain language
changes of the upcoming release.
[1] https://github.com/dlang/phobos/tree/master/changelog
[2] https://github.com/dlang/dlang.org/pull/1549
[3]
https://github.com/dlang/dlang.org/tree/master/language-changelog
