Add Bors configuration and documentation #686

andyleejordan · 2018-09-12T22:33:39Z

No description provided.

andyleejordan · 2018-09-12T22:33:48Z

bors try

oe-bors · 2018-09-12T22:33:52Z

try

Configuration problem

bors.toml: syntax error

andyleejordan · 2018-09-12T22:37:17Z

bors try

johnkord

LGTM

oe-bors · 2018-09-12T22:53:23Z

try

Build succeeded

continuous-integration/jenkins/branch

andyleejordan · 2018-09-12T23:02:39Z

bors try

andyleejordan · 2018-09-12T23:08:13Z

It would appear that the "short form" referenced here for setting the committer is seen as "invalid syntax," but the "long form" worked.

Open question for @CodeMonkeyLeet and @johnkord: The default user/email is oe-bors[bot] and the no-reply email GitHub creates for it. The plus to this is (a) it's the default and (b) it links the commits to the actual bot/app account. I was able to change it to an arbitrary name (though I reused oe-bors[bot]) and set the email to oeciteam@microsoft.com; but by changing the email it links the commits to our OE CI Team GitHub service account. This may be desirable. Your choice!

andyleejordan · 2018-09-12T23:53:51Z

bors try just testing how it makes that commit

oe-bors · 2018-09-12T23:53:52Z

try

Not awaiting review

andyleejordan · 2018-09-12T23:57:22Z

bors try foo

oe-bors · 2018-09-12T23:57:24Z

try

Not awaiting review

andyleejordan · 2018-09-12T23:58:27Z

bors try foo

oe-bors · 2018-09-12T23:58:28Z

try

Not awaiting review

andyleejordan · 2018-09-12T23:58:40Z

bors try

oe-bors · 2018-09-12T23:58:41Z

try

Not awaiting review

andyleejordan · 2018-09-13T00:06:17Z

bors try

oe-bors · 2018-09-13T00:06:18Z

try

Not awaiting review

andyleejordan · 2018-09-13T00:06:41Z

Did I break it? Closing and re-opening...

andyleejordan · 2018-09-13T00:06:49Z

bors try

oe-bors · 2018-09-13T00:06:50Z

try

Not awaiting review

andyleejordan · 2018-09-13T00:09:14Z

Sorry for the spam; deleted the trying branch, trying again.

bors try

andyleejordan · 2018-09-13T00:56:07Z

Woohoo webhook worked.

Sad thing is all this worked in my fork since I was setting it up "fresh" but transferring it turned out to be more work than I expected.

oe-bors · 2018-09-13T01:03:49Z

try

Build succeeded

continuous-integration/jenkins/branch

This commit updates the "DOs" and "Merging Pull Requests" for the new Bors workflow. It also adds a rule to ensure commit authorship is correct, as this appears to be an ongoing problem. Some other formatting inconsistencies were fixed.

andyleejordan · 2018-09-13T18:05:48Z

@johnkord @CodeMonkeyLeet are you guys happy with the configuration (especially the Bors bot name and email)?

andyleejordan · 2018-09-13T18:12:18Z

bors try

andyleejordan · 2018-09-13T18:14:32Z

@johnkord Ah so apparently that's expected; I thought it linked to the bot if it was the default, but I tried in the bors-ng repo too, and it does not link appropriately. That puts me in the custom name/email camp to link to the service account.

oe-bors · 2018-09-13T18:25:49Z

try

Build succeeded

continuous-integration/jenkins/branch

letmaik · 2018-09-13T18:37:00Z

docs/Contributing.md

@@ -65,12 +65,12 @@ Please do:
 * **DO** tag any users that should know about and/or review the change.
 * **DO** ensure each commit successfully builds on all platforms and passes all
  unit tests.
+* **DO** rebase and squash unnecessary commits before opening the PR, so that


Is this really necessary? If PRs get squash merged anyway what's the point? When a PR is created I normally don't look at the commits but just at the changes.

PRs won't get squashed any more. If you want to squash it, do it manually. It is totally up to the author how they want to represent their work.

This isn't to pick on anyone (I didn't even look at the authorship), but history like this is not very useful:

6778b20fc clang break cd153edbe clang build errors c8f11599c Clang build breaks b05bd8cc5 clang break 4aaccb61c fix clang build break 3114f89d7 fix clang build break 6e781d01f fix clang error

@letmaik You okay with that?

andyleejordan · 2018-09-13T18:44:54Z

Draft of instruction email to be sent after this is merged:

The Bors bot is now ready to go online. What this means for you is that existing pull requests should either rebase on top of master, or merge master into your branch. Moving forward, in order to merge a finished PR (and to trigger CI for a PR), the commands bors r+ and bors try will instead be used. The usage is simple: just add a comment to your PR with bors try, and the bot will build and test your branch and report the results back. Similarly, instead of clicking "the big green button" to merge a PR, just add a comment bors r+, and the bot will merge your branch to master (but only after it passes the automatic build and test gate).

More information on the Bors bot command reference is here: https://bors.tech/documentation/

To ensure your branch can use Bors, check that you have the file bors.toml in the root of your repo.

CodeMonkeyLeet

Mostly doc comments suggestions

CodeMonkeyLeet · 2018-09-13T17:04:18Z

docs/Contributing.md

+_only_ approved mechanism of merging code to `master`. When a PR is ready to be
+merged, a maintainer will comment on it with `bors r+`. Bors will automatically
+apply the PR's commits to a staging branch, run the test suite, and if
+everything passes, pushes the commits (and a merge commit) to `master`.


Simplify the sentence structure, probably clearer to enumerate:

Bors will automatically: 1. Apply the commits from the PR to a staging branch based on `master`. 1. Trigger the code integration (CI) system to build and test the staging branch. 1. Push the commits and a merge commit to `master` only if everything passes.

CodeMonkeyLeet · 2018-09-13T17:04:22Z

docs/Contributing.md

+apply the PR's commits to a staging branch, run the test suite, and if
+everything passes, pushes the commits (and a merge commit) to `master`.
+
+At first glance this may appear to be no different then allowing the CI system


Grammar: "different from" (or if you accept colloquial usage, "different than")

CodeMonkeyLeet · 2018-09-13T18:50:59Z

docs/Contributing.md

+everything passes, pushes the commits (and a merge commit) to `master`.
+
+At first glance this may appear to be no different then allowing the CI system
+to test the PR, and then merging with "the big green button." However, allowing


I'd suggest simplifying the paragraph to:

We require the use of Bors because it prevents a race condition that can result from manual merges: two conflicting PRs may both pass CI testing independently while neither is in master, only to break the branch once both are merged. Bors synchronizes the CI testing of PRs and ensures that passing PRs are immediately merged, so that the state of master always reflects the state tested and passed by CI.

CodeMonkeyLeet · 2018-09-13T18:52:17Z

docs/Contributing.md

+
+| Syntax | Description
+|--------|------------
+| bors r+ | Run the test suite, and push to master if it passes.


Nit: remove the comma, it's not a serial (oxford) comma since the "and" is a conjunction. Word flags it.

CodeMonkeyLeet · 2018-09-13T18:53:15Z

docs/Contributing.md

+|--------|------------
+| bors r+ | Run the test suite, and push to master if it passes.
+| bors r- | Cancel a pending r+.
+| bors try | Run the test suite without pushing (trigger CI).


Consider simplifying to just "Run the CI tests but do not merge on success."

CodeMonkeyLeet · 2018-09-13T18:54:32Z

docs/Contributing.md

+| bors try | Run the test suite without pushing (trigger CI).
+| bors delegate+ | Allow the pull request author to r+.
+| bors delegate=[list] | Allow the listed users to r+.
+| bors ping | Will respond if Bors is up.


Consistency in use of direct, active voice: "Check if Bors is responsive. It will comment with pong if it is."

CodeMonkeyLeet · 2018-09-13T19:00:06Z

docs/Contributing.md

@@ -153,10 +186,10 @@ You can read more about [Contribution License Agreements (CLA)](
 http://en.wikipedia.org/wiki/Contributor_License_Agreement) on Wikipedia.

 You don't have to do this up-front. You can simply clone, fork, and submit your
-pull-request as usual. When your pull-request is created, it is classified by a
+Pull Request as usual. When your Pull Request is created, it is classified by a


We should standardize on how GitHub refers to these as simply "pull request" (it's also referred to as such elsewhere in this doc), unhyphenated and uncapitalized (it's not treated as a proper noun).

CodeMonkeyLeet · 2018-09-13T19:07:27Z

docs/Contributing.md

-:
+Messages](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html)),
+and use the present tense and imperative mood when describing your changes. This
+is akin to the commit "giving a command" to the code base, and is used by the


This sentence is a little confusing. It sounds like it should be combined with the previous clause you added … did you mean:

Write your descriptions as imperatives in the active voice, as if you are telling Git what you want it to do to the code base.

CodeMonkeyLeet · 2018-09-13T19:13:36Z

docs/Contributing.md

@@ -143,6 +163,19 @@ Also do your best to factor commits appropriately, not too large with unrelated
 things in the same commit, and not too small with the same small change applied
 _n_ times in _n_ different commits.

+Please ensure the correct name and email are set in the commit. See [initial


I'd suggest presenting direct information before links in general:

Ensure that the correct author name and email are set for the commit. For example, before commiting, ensure that Git is configured with your user name and email:
...
For more details, see [initial setup for Git](https://git-scm.com/...

CodeMonkeyLeet · 2018-09-13T19:18:48Z

docs/Contributing.md

+```
+
+We _will not_ accept commits with incorrect authorship. To fix the authorship of
+commits in a feature branch, use `git rebase`, choose to `edit` the offending


Multi-line instructions are probably best captured as lists:

If you have existing commits with incorrect author information, you can fix them as follows:

git rebase your working branch

Choose to edit the commits with incorrect authorship

For each edit, git commit --amend --reset-author

andyleejordan · 2018-09-13T20:04:08Z

Thanks @CodeMonkeyLeet. I also pushed the table's changes upstream bors-ng/bors-ng.github.io#54 😄

CodeMonkeyLeet

LGTM!

andyleejordan · 2018-09-13T20:45:14Z

bors r+

686: Add Bors configuration and documentation r=andschwa a=andschwa Co-authored-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>

oe-bors · 2018-09-13T20:54:01Z

Build succeeded

continuous-integration/jenkins/branch

andyleejordan · 2018-09-13T21:05:32Z

Woo-hoo everything worked according to plan.

letmaik · 2018-09-13T21:29:12Z

Is this really what people want? What about squash merging?

andyleejordan · 2018-09-14T16:58:27Z

@letmaik Yes... that is to say, as the author of the PR, it's what I wanted: three clean commits for (1) the bors.toml (2) the initial doc update and (3) the review feedback (I'm 50/50 on this one, I prefer to squash review feedback into the relevant commit, but Simon likes separate commits for feedback, and it's reasonable for discerning what the author presented and what the end result was).

If a PR has half a dozen commits that should be squashed, it's a trivial ask that the dev do git rebase -i to squash them. To not do so instead means the author expects the reviewer to recognize their commits are bad, and to choose to use GitHub-squash instead. Applying GitHub-squash as the default means that most authors' work will be thrown away.

That said, here's the relevant issue if you want to help implement it bors-ng/bors-ng#194

andyleejordan requested a review from CodeMonkeyLeet September 12, 2018 22:33

andyleejordan force-pushed the bors branch from a2d8ec5 to 32aa93d Compare September 12, 2018 22:36

johnkord previously approved these changes Sep 12, 2018

View reviewed changes

andyleejordan dismissed johnkord’s stale review via f74da91 September 12, 2018 23:02

andyleejordan force-pushed the bors branch from 32aa93d to f74da91 Compare September 12, 2018 23:02

andyleejordan force-pushed the bors branch from f74da91 to 5a6639d Compare September 12, 2018 23:58

andyleejordan closed this Sep 13, 2018

andyleejordan reopened this Sep 13, 2018

andyleejordan added 2 commits September 13, 2018 11:05

Add Bors configuration

82b178f

Add Bors to contributing docs

d139a80

This commit updates the "DOs" and "Merging Pull Requests" for the new Bors workflow. It also adds a rule to ensure commit authorship is correct, as this appears to be an ongoing problem. Some other formatting inconsistencies were fixed.

andyleejordan force-pushed the bors branch from 5a6639d to d139a80 Compare September 13, 2018 18:05

andyleejordan force-pushed the bors branch from 92e55ec to d139a80 Compare September 13, 2018 18:20

letmaik reviewed Sep 13, 2018

View reviewed changes

johnkord approved these changes Sep 13, 2018

View reviewed changes

CodeMonkeyLeet reviewed Sep 13, 2018

View reviewed changes

Incorporate review feedback

95a227d

CodeMonkeyLeet approved these changes Sep 13, 2018

View reviewed changes

CodeMonkeyLeet added the engineering Issue is related to tools and processes necessary for maintaining the Open Enclave repo label Sep 13, 2018

CodeMonkeyLeet added this to the 2018.09 milestone Sep 13, 2018

oe-bors bot pushed a commit that referenced this pull request Sep 13, 2018

Merge #686

b0a6adc

686: Add Bors configuration and documentation r=andschwa a=andschwa Co-authored-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>

oe-bors bot merged commit 95a227d into master Sep 13, 2018

oe-bors bot deleted the bors branch September 13, 2018 20:54

Add Bors configuration and documentation #686

Add Bors configuration and documentation #686

Conversation

andyleejordan commented Sep 12, 2018

andyleejordan commented Sep 12, 2018

oe-bors bot commented Sep 12, 2018

try

Configuration problem

andyleejordan commented Sep 12, 2018

johnkord left a comment

Choose a reason for hiding this comment

oe-bors bot commented Sep 12, 2018

try

Build succeeded

andyleejordan commented Sep 12, 2018

andyleejordan commented Sep 12, 2018

andyleejordan commented Sep 12, 2018

oe-bors bot commented Sep 12, 2018

try

andyleejordan commented Sep 12, 2018

oe-bors bot commented Sep 12, 2018

try

andyleejordan commented Sep 12, 2018

oe-bors bot commented Sep 12, 2018

try

andyleejordan commented Sep 12, 2018

oe-bors bot commented Sep 12, 2018

try

andyleejordan commented Sep 13, 2018

oe-bors bot commented Sep 13, 2018

try

andyleejordan commented Sep 13, 2018

andyleejordan commented Sep 13, 2018

oe-bors bot commented Sep 13, 2018

try

andyleejordan commented Sep 13, 2018

andyleejordan commented Sep 13, 2018

oe-bors bot commented Sep 13, 2018

try

Build succeeded

andyleejordan commented Sep 13, 2018

andyleejordan commented Sep 13, 2018

andyleejordan commented Sep 13, 2018

oe-bors bot commented Sep 13, 2018

try

Build succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Sep 13, 2018

CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Sep 13, 2018

CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

andyleejordan commented Sep 13, 2018

oe-bors bot commented Sep 13, 2018

Build succeeded

andyleejordan commented Sep 13, 2018

letmaik commented Sep 13, 2018

andyleejordan commented Sep 14, 2018