Protected branches plugin

[jwsloan]

Fixes #3 .

[jwsloan]

@bkeepers I didn't actually test this manually. I couldn't figure out how to get everything setup correctly. My creds were failing. But I figured I'd go ahead and get this submitted anyway.

[jwsloan]

@bkeepers I have successfully tested this in a custom app. Any idea when you can take a look?

[patcon]

Is your custom app still live? I can do a review, but my JS chops are such that I'd feel best just using it for a week on a non-critical project :)

[jwsloan]

@patcon hopefully this works... I just made it public. https://github.com/apps/probotsettingsprotectedbranches

GitHub

Build software better, together

GitHub is where people build software. More than 27 million people use GitHub to discover, fork, and contribute to over 80 million projects.

[patcon]

Maybe there's room for a larger discussion on whether we could create two apps (prod and staging), and have a pipeline that deploys to prod on master merge. The idea being that @bkeepers can merge master a tiny bit more liberally, knowing that a small crew (maybe technical, but maybe less technical) is on the bleeding edge and will give feedback and/or exercise new features

Happy to open another issue if that sounds plausible

[patcon]

@jwsloan Thanks! 🎉 Playing with it here: https://github.com/patcon/sample-civictech-app

(I'll prob fully exercise it tomorrow -- and it will also help me create a best-practice project starter template for civictech projects for the weekly hacknights I co-organize! so thanks so much!)

[patcon]

totes minor, but quoted strings aren't used anywhere else in config :)

[patcon]

I'm pretty familiar with this page, and some of my recollection of settings has to do with placement:

Can we put them in the order of the settings screen as much as possible?

[jwsloan]

We certainly can. That makes a lot of sense. I was ordering it based on the API docs. But basing it on what people are most likely to be familiar with is the right call.

[bkeepers]

This looks great so far! I'll pull it down soon and try it out.

What happens if I have protected branches and I remove the protection key from the config? I'm assuming it stays configured? Is that the right behavior, or do you thin people would expect to see it removed?

[jwsloan]

Thanks for taking a look, @bkeepers. Currently removing the key means you no longer want to manage Protected Branches via this file. It will no longer make any changes. If you leave the protection key, but it is empty, it will remove protection.

[patcon]

Great point @bkeepers. I like @jwsloan's approach, that removing the key completely should leave protection as-is.

Should we treat null and [] as empty, just like we do with {}? That would seem to be forgiving, even if perhaps not totally consistent. If we do this, would be great to have it reflected in tests :)

[ci-reporter]

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of 3708fbb. Here's the output:

npm test

> [email protected] test /home/travis/build/probot/settings
> jest && standard

 PASS  test/lib/plugins/repository.test.js
 PASS  test/lib/plugins/branches.test.js
 PASS  test/lib/plugins/teams.test.js
 PASS  test/lib/plugins/collaborators.test.js
 PASS  test/lib/plugins/labels.test.js
 PASS  test/index.test.js

Test Suites: 6 passed, 6 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        6.401s
Ran all test suites.
standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /home/travis/build/probot/settings/lib/plugins/branches.js:26:10: Missing space before function parentheses.

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!

---

This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

[jwsloan]

@patcon I went ahead and implemented that. What do you think?

[jwsloan]

@patcon, @bkeepers have either of you had a chance to try it out?

[patcon]

Yep, I've used it! It's pretty rad. One thought is that the config is pretty complex and nested, and it might make sense for it to be more forgiving, since probot fails silently.

I know you already extended null and {} to be falsey, but what about treating all these the same? [], {}, null, false.

(That might be a larger convo, so happy to defer this into a different issue, as I think it works fine as-if if the person configuring is careful :)

[jwsloan]

Thanks for the feedback, @patcon! [] should already work. It is definitely reasonable for false to be treated as falsey... :) I'll create a test for that.

[jwsloan]

@patcon, I've pushed a commit adding tests for [] and false. They show that it already works as expected. Any other feedback?

[patcon]

Hm. Odd. This was the commit, before which seemed to be was silently failing?
CivicTechTO/sample-civictech-app@6bcbc41

I assume dismissal_restrictions: null was borking it? I can try to investigate later

[jwsloan]

@patcon, sorry. I thought we were just discussing the top-level protection key. I'm not sure it makes sense for all of those to be accepted in every situation. I need to look closer at what the API expects.

[jwsloan]

@patcon According to the API documentation, it looks like it is not valid to send null for dismissal_restrictions. If it is present, it needs to be an object. It seems like this API may not be 100% consistent in how it expects each key to be set to turn it off.

It seems like an answer is to not have probot fail silently. Is there something we can do about that? It would definitely not be good for someone to think they've successfully protected a branch, but not had it work.

[jwsloan]

@bkeepers, @patcon are you guys willing to go ahead and merge this? We could see if people run into pain with the configuration, and go from there?

[jwsloan]

@bkeepers the issue this came from just got marked stale. Do you think we could get it merged soon? Thanks!

[patcon]

@travi's comment:

i see a number of the more complex options for protected branches detailed out above, but my most common use case for simple repos is to simply mark it as protected so no one can accidentally force push. what might that simple configuration look like?

replying here since it has to do with code here :)

My quick impression is that this would work. Perhaps worth having a larger comment code block giving some simple examples though -- if @jwsloan confirms, perhaps @travi you could suggest what would have helped you have clarity on how it works 🎉

branches:
  master:
    protection: {}

[travi]

sorry for the delayed response. somehow i had overlooked that this PR existed for the issue that I commented on. looks like there is a fair amount of info included here that I just hadn't found. mainly just wanted to make sure that the simple cases had been considered, and it sounds like they have. I'll be able to better confirm once this gets merged.

my simples use cases are usually to enable "Protect this branch" to prevent force pushes over master and almost always also "Include administrators"

[davidjdixon]

@bkeepers I'd like to also see this PR merged. Could we get a yes/no either way?

[jwsloan]

Mr. @bkeepers, any chance we can move forward with this?

[bkeepers]

Thanks @jwsloan and everyone else for your contributions here! Sorry for the delay in merging the changes, but thanks for your patience.

This is available in production. Let me know if you see any issues or have any feedback.

[travi]

thanks @bkeepers, @jwsloan, and everyone for getting this in place. i'm excited to be able to automate this one away

[travi]

unfortunately, i'm not seeing this setting take effect.

i initially tried just setting what i hoped would be the minimum. when that didn't work, i paid more attention to the "Required" properties. since that didn't enable protection for the branch either, i tried enabling one of the rules i didnt need, just to see if more was necessary, but still had no luck. am i missing something obvious?

also, just a side note, but the new required properties somewhat conflict with the note under Usage that states:

All settings are optional.

I realize that they are only required if the higher-level settings are provided, but some clarification might be valuable there.

[jwsloan]

@travi Since it is live now, I'll give it a shot myself. It could be that the Protected Branches API has changed since I submitted the PR, and I didn't do a good job of keeping track of that.

[jwsloan]

@travi would you put in an issue with the settings you're using?

[travi]

I can add more detail tonight, but just in my phone for now. The links above show the variations I've tried

[travi]

opened a separate issue for what i mentioned above: #87

[jwsloan]

@patcon does your install of my test app still work? As mentioned in #87, the feature doesn't appear to be working anymore.