C H E C K S A P I S U P P O R T 🤩 #79

hiimbex · 2018-07-05T23:46:29Z

What this PR does:

Upgrades to Probot 7️⃣. 0️⃣
Eliminates use of status API ❌
Uses Checks API aka best API 🎆
Restructures lib/dco.js to return an array of error messages objects like:

[{ 
  "author": "Bex Warner",
  "committer": "Bex Warner",
  "message": "The sign-off is missing.",
  "sha": "fe9f33191f3a87c86843292920decab97b5b71b",
  "url": "https://github.com/probot/dco/pull/78/commits/fe9f33191f3a87c86843292920decab97b5b71b"
}]

Restructures test/dco.test.js to support that format 🛠
Adds guidance for ammending your commits 🆘
- supports git commit --amend --signoff for a mistake in the most recent commit
- supports git rebase Head~$numOfCommits --signoff for mistakes in multiple commits
- then instructs git push --force origin $your-branch
Also includes a list of full length errors for each commit 🔢
Adds a hard override of the status for anyone with write permissions ✍️

LOOK AT HOW PRETTY IT IS

All commits are signed off:

One commit incorrectly signed off:

Multiple commits incorrectly signed off:

Override button (only visible for those with repo write access):

Todo:

Properly convert test/index.test.js to v7 and update tests. (tbh I don't fully understand how this test worked before, so not sure how to convert it!)
Maybe improve code readability/structure because at the moment it's rather hacky

cc/ @probot/maintainers @kytrinyx for 👀& 💭's

caniszczyk · 2018-07-06T05:58:37Z

omg I’m in love!

bkeepers

At a quick glance this looks awesome. I haven't had a chance to review the code yet, but love the functionality.

Is there any way we could "staff ship" this big of a change to allow people to opt in to the new behavior?

Could we separate out the upgrade to Probot 7.0 from the changes in functionality?

bkeepers · 2018-07-09T16:11:54Z

index.js

+        },
+        actions: [{
+          label: 'Set DCO to pass',
+          description: 'would set status to passing',


I'd love to get eyes on this from a legal perspective to make sure that a this is worded properly. Something like "Accept DCO" and "I certify that my contributions fall under the Developer Certificate of Origin: https://developercertificate.org/"

@caniszczyk @mlinksva: could you recommend someone to help here?

Also, I wonder if there should be a separate "Dismiss" vs "Accept" buttons

So it's important to note this option is only accessible for users with write access to the repo.

We get quite a few requests for new users to not have to use git so painfully, so if the maintainers of a repo are happy with the user's comments that they "accept" the DCO, might as well give them a free status pass button.

Maybe I'm missing something, but this looks to me like a way for a maintainer to ignore the test, not for a contributor to agree their contributions are covered by the DCO.

Added: as @hiimbex wrote faster. 😳

Doh, sorry, I totally wasn't thinking about who would be the one clicking the button.

Signed-off-by: Bex Warner <[email protected]>

Signed-off-by: Brandon Keepers <[email protected]> Signed-off-by: Bex Warner <[email protected]>

Signed-off-by: Bex Warner <[email protected]>

hiimbex · 2018-07-18T03:36:44Z

Tests are now working! @bkeepers I'm blaming you 100% for the DCO failing (also a good motivator to implement #75 at some point).

Is there any way we could "staff ship" this big of a change to allow people to opt in to the new behavior?

I'm wondering if you still think this is necessary given that the 'set DCO to passing' otpion is only for those with write access. If you still this is something we should pursue, I'm not sure the best path forward for something like this. Maybe an additional .env variable?

I personally don't think a staff ship is necessary since the only functionality change is giving maintainers a button and using checks not statuses (it should still work in the same way). The other big update that goes with this is updating the permissions the app uses; however, now we get to write a note explaining why! I'm happy to craft one explaining this PR super in depth, so our users know why this change is happening.

But whenever you have ⏳ this is ready for 👀!

bkeepers

Testing this out locally now and will have more feedback, but first thing I noticed is that there a bunch of undefineds in the tests. Is that just bad test data, or is there something broken?

bkeepers · 2018-07-18T03:46:53Z

test/dco.test.js

+      const dcoObject = await getDCOStatus([{commit, author: { login: 'hiimbex' }, parents: []}], alwaysRequireSignoff, prInfo)
+
+      expect(dcoObject).toEqual([{
+        url: 'https://github.com/hiimbex/testing-things/pull/1/commits/undefined',


undefined?

@bkeepers I honestly thought I updated this. All that needs to change is the fake data passed into each test needs a sha value, for example:

const dcoObject = await getDCOStatus([{commit, author: { login: 'bkeepers' }, parents: []}], alwaysRequireSignoff, prInfo)

should be:

const dcoObject = await getDCOStatus([{commit, author: { login: 'bkeepers' }, parents: [], sha: '318g9hnf02b49uthisisasha'}], alwaysRequireSignoff, prInfo)

The code that actually makes this change is here

bkeepers

Gave this a try and it looks pretty good. Eventually I'd love to chat about refactoring some of this code, but it can wait until after this feature lands.

bkeepers · 2018-08-02T19:10:50Z

index.js

+}
+
+function handleMultipleCommits (pr, commitLength, dcoFailed) {
+  return `You only have ${dcoFailed.length} commits incorrectly signed off! To fix, head to your local branch and run: \n\`\`\`bash\ngit rebase HEAD~${commitLength} --signoff\n\`\`\`\n Now your commits will have your sign off. Next run \n\`\`\`bash\ngit push --force origin ${pr.head.ref}\n\`\`\``


This assumes the commits missing the DCO are always the last n commits. What if I have 5 commits, but the one in the middle properly has the DCO?

Could we just have the user rebase based on the base of the pull request?

Correct me if I'm wrong, butgit push --force origin ${pr.head.ref} does exactly what you're describing. I was originally trying to do something fancy with commitLength but I think it's actually dead code.

Ok, so I forgot what I actually did. So commitLength actually is the number of total commits in the PR, so you always rebase and sign off all of those commits, no matter which ones were wrong via:
git rebase HEAD~${commitLength} --signoff which utilizes this rebase option. And then does:
git push --force origin ${pr.head.ref}

bkeepers · 2018-08-02T19:12:48Z

lib/dco.js

@@ -22,23 +17,34 @@ module.exports = async function (commits, isRequiredFor) {
    }
    const match = regex.exec(commit.message)

+    const commitInfo = {
+      sha,
+      url: `https://github.com/${prInfo.owner}/${prInfo.repo}/pull/${prInfo.number}/commits/${sha}`,


What about using context.payload.pull_request.html_url as the base for this?

Signed-off-by: Bex Warner <[email protected]>

hiimbex · 2018-08-03T14:42:16Z

Just confirmed via hiimbex/testing-things#71 that the existing PRs that swap mid-way from the old statuses to checks don't need any special work or support. The re-written check will overwrite the status.

hiimbex · 2018-08-03T17:57:52Z

Tests are failing due to jestjs/jest#6769 and will I think be fixed in a new Jest release? But it's compeltley unrelated to my changes.

Would love a re-review based on the updates whenever you've got a chance @bkeepers 🙂

Signed-off-by: Bex Warner <[email protected]>

Update docs to reflect swap to checks api

hiimbex · 2018-08-22T19:09:33Z

Consider it 🚢 d!! 🎉

caniszczyk · 2018-08-22T19:19:06Z

THANK YOU!

…

On Wed, Aug 22, 2018 at 2:09 PM, Bex Warner ***@***.***> wrote: Consider it 🚢 d!! 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD5ITSi2pbiUE9DCM8BlkCEvim5CCV8ks5uTaxtgaJpZM4VEr_o> .

-- Cheers, Chris Aniszczyk http://aniszczyk.org +1 512 961 6719

github-actions · 2022-01-18T05:39:52Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

bkeepers reviewed Jul 9, 2018

View reviewed changes

bkeepers self-requested a review July 9, 2018 16:15

hiimbex mentioned this pull request Jul 9, 2018

update probot to v7 #80

Merged

hiimbex and others added 9 commits July 14, 2018 11:42

implement checks api support 🤩

18aebfa

Signed-off-by: Bex Warner <[email protected]>

update probot to v7

df54955

Signed-off-by: Brandon Keepers <[email protected]> Signed-off-by: Bex Warner <[email protected]>

Use Probot instead of Application directly

e8de4ff

Signed-off-by: Brandon Keepers <[email protected]> Signed-off-by: Bex Warner <[email protected]>

Relax version requirements

a165b32

Signed-off-by: Brandon Keepers <[email protected]> Signed-off-by: Bex Warner <[email protected]>

fix regression from #77 and add test

8f38194

Signed-off-by: Bex Warner <[email protected]>

add tests

cae6249

Signed-off-by: Bex Warner <[email protected]>

rm unnecessary verified fields in tests

fb013ba

Signed-off-by: Bex Warner <[email protected]>

finish writing nock for failing and successful checks

e3cabb4

Signed-off-by: Bex Warner <[email protected]>

add back accidnetally deleted .env.example

075cc5f

Signed-off-by: Bex Warner <[email protected]>

hiimbex force-pushed the checks-api branch from 219bc67 to 075cc5f Compare July 14, 2018 18:47

hiimbex added 2 commits July 14, 2018 11:50

Merge branch 'master' into checks-api

21bd20f

at this point git is just trolling me 😤

89571b1

Signed-off-by: Bex Warner <[email protected]>

hiimbex mentioned this pull request Jul 27, 2018

Allow signed off via comment #83

Closed

hiimbex assigned bkeepers Aug 2, 2018

bkeepers suggested changes Aug 2, 2018

View reviewed changes

bkeepers reviewed Aug 2, 2018

View reviewed changes

bkeepers removed their assignment Aug 2, 2018

hiimbex added 2 commits August 3, 2018 07:12

add mock sha values in tests

dec6419

Signed-off-by: Bex Warner <[email protected]>

swap to url from the payload

a646a75

Signed-off-by: Bex Warner <[email protected]>

hiimbex assigned bkeepers Aug 3, 2018

fix jest issue

4779571

Signed-off-by: Bex Warner <[email protected]>

bkeepers approved these changes Aug 15, 2018

View reviewed changes

bkeepers temporarily deployed to probot-dco August 15, 2018 18:07 Inactive

bkeepers deployed to staging August 15, 2018 19:51 Active

bkeepers deployed to staging August 15, 2018 19:54 Active

hiimbex added 4 commits August 20, 2018 17:53

add backwards compatibility for statuses

df1886e

Signed-off-by: Bex Warner <[email protected]>

lintin

2991458

Signed-off-by: Bex Warner <[email protected]>

Update README.md

30a652e

Signed-off-by: Bex Warner <[email protected]>

Merge pull request #85 from probot/hiimbex-patch-1-1

2d02575

Update docs to reflect swap to checks api

hiimbex deployed to production August 22, 2018 18:53 Active

hiimbex mentioned this pull request Aug 22, 2018

Checks API Feedback #86

Closed

hiimbex merged commit 35f7202 into master Aug 22, 2018

hiimbex deleted the checks-api branch August 22, 2018 19:09

github-actions bot added the released label Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C H E C K S A P I S U P P O R T 🤩 #79

C H E C K S A P I S U P P O R T 🤩 #79

hiimbex commented Jul 5, 2018 •

edited

Loading

caniszczyk commented Jul 6, 2018 via email •

edited by hiimbex

Loading

bkeepers left a comment

bkeepers Jul 9, 2018

hiimbex Jul 9, 2018

mlinksva Jul 9, 2018 •

edited

Loading

bkeepers Jul 18, 2018

hiimbex commented Jul 18, 2018 •

edited

Loading

bkeepers left a comment

bkeepers Jul 18, 2018

hiimbex Aug 2, 2018

bkeepers left a comment

bkeepers Aug 2, 2018

hiimbex Aug 2, 2018

hiimbex Aug 3, 2018

bkeepers Aug 2, 2018

hiimbex commented Aug 3, 2018

hiimbex commented Aug 3, 2018

hiimbex commented Aug 22, 2018

caniszczyk commented Aug 22, 2018 via email

github-actions bot commented Jan 18, 2022

C H E C K S A P I S U P P O R T 🤩 #79

C H E C K S A P I S U P P O R T 🤩 #79

Conversation

hiimbex commented Jul 5, 2018 • edited Loading

What this PR does:

LOOK AT HOW PRETTY IT IS

Todo:

caniszczyk commented Jul 6, 2018 via email • edited by hiimbex Loading

bkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlinksva Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiimbex commented Jul 18, 2018 • edited Loading

bkeepers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkeepers 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

hiimbex commented Aug 3, 2018

hiimbex commented Aug 3, 2018

hiimbex commented Aug 22, 2018

caniszczyk commented Aug 22, 2018 via email

github-actions bot commented Jan 18, 2022

hiimbex commented Jul 5, 2018 •

edited

Loading

caniszczyk commented Jul 6, 2018 via email •

edited by hiimbex

Loading

mlinksva Jul 9, 2018 •

edited

Loading

hiimbex commented Jul 18, 2018 •

edited

Loading