Add Protected Branches API support #996

ryangribble · 2015-12-13T12:55:17Z

This Work-In-Progress PR adds support for the protected branches API, which is currently in PREVIEW

Notes

This is our first contribution to octokit.net so just let us know if anything isn't as desired etc... I've tried to comply with existing similar implementations, coding/comment styles, unit and integration tests and so on, so hopefully it isn't too far off the mark

Since the protected branches API is in preview, I wasn't sure if it's OK to be added, but we need this support for tooling we are writing and would rather contribute to upstream than use a local fork, unless we have to! I did notice a couple of other preview features in the codebase. I also saw that octokit.rb has implemented the protected branches functions, so hopefully this is OK

Implementation Details

At a high level the change involves:

Create new models for Branch Protection details
Add Protection element to existing Branch class
Existing GetBranch() and GetAllBranches() calls specify new preview accepts header so this data is returned
Added a new EditBranch() call, similar to other Edit() methods, that takes a BranchUpdate object specifying the protection settings/fields to change. This is Patched into the Branch object/URI as per other similar examples
Modified existing tests for GetBranch() and GetAllBranches() calls to handle the accepts parameter now being specified
Added unit and integration tests for the branch protection features, which are all passing

To Do List

~~Haven't yet looked at what is required on the Octokit.Reactive project~~ Added new function, not sure if anything more to do?

shiftkey · 2015-12-13T20:24:08Z

@ryangribble welcome aboard, just had a quick look at this and it looks really good! Thanks for the hard work kicking this off!

Added new function, not sure if anything more to do?

The Octokit.Reactive project is intended to be API-equivalent to the Octokit project, but returning IObservable<T> instead of Task<T>. I think what you've done is just enough, however CI isn't kicking in so I need to look at that...

shiftkey · 2015-12-13T23:07:53Z

Octokit/Models/Response/BranchProtection.cs

+        /// <summary>
+        /// Should this branch be protected or not
+        /// </summary>
+        public bool Enabled { get; set; }


As this is a response object, this will currently fail our checks for public setters.

For serialization purposes, protected set will work fine - but we also define ctors (like in Branch above) for scenarios where you may need to set it up in a certain way.

You can see these failures by running this from the repo root:

.\build BuildApp
.\build ConventionTests

Done: TattsGroup@37df149

shiftkey · 2015-12-13T23:15:27Z

Integration tests are passing on my end 👍

@haacked how do you feel about the API design here? Anything stand out for feedback?

ryangribble · 2015-12-14T00:04:54Z

No worries Ill fix up those protected set and parameterised ctor's shortly

Regarding API design at first (and you can see on my earlier commits) I was following the octokit.rb implementation of having ProtectBranch() and UnprotectBranch() calls, however this didnt really suit the design of octokit.net which is more focused on having an Edit() method for an entity, and passing in the xxxUpdate model object with the changes... so I re-implemented as a single EditBranch() function

shiftkey · 2015-12-14T01:51:29Z

@ryangribble Pro Tip™: don't forget to add your work account to your GitHub profile, so those last commits are linked to your profile. Or just amend the author info and push -f to your branch.

ryangribble · 2015-12-14T01:57:26Z

@shiftkey good catch thanks, email should be fixed now

ryangribble · 2015-12-14T04:11:26Z

LOL so I was just testing this against our GitHub Enterprise on premises installation, and it looks like the preview API isnt actually in GitHub Enterprise version 2.4.2 (eventhough the actual Branch Protection features are). Will need to wait for Enterprise to include the API feature before we can actually use this stuff in our tooling 😄

khellang · 2015-12-14T08:10:22Z

Octokit.Tests.Conventions/ModelTests.cs

@@ -85,6 +85,12 @@ public void ResponseModelsHaveReadOnlyCollections(Type modelType)
                        continue;
                    }

+                    // Let's skip collections that only have a private Setter
+                    if (property.SetMethod != null && property.SetMethod.IsPrivate)


Why should collection properties with private setters be mutable?

this change makes it so collection properties with private setters are NOT added to the mutable list. Otherwise convention tests were failing saying that these properties werent obeying the Response object rules

That said, this was just a possible fix that seemed to work for me... definitely looking for feedback from main contributors and can easily remove this commit if required (and try to find another way to satisfy the convention tests)

But this is totally out of scope for this test, and it actually makes it incorrect. This test checks that "Response Models Have Read Only Collections". Making it skip properties with private setters also makes it allow mutable collections, which defeats the test in the first place. There's already a test that checks for non-public setters.

Sorry to jump in here before this goes further off the rails, but thanks @khellang for reminding me about the intent of this test.

ICollection<T> has the mutable Add, Remove and Clear - so even if it's a read-only property, it can be manipulated arbitrarily after the object is created. We're trying to discourage that, so we use IReadOnlyList<T> on our collections.

An example of that is CommitActivity - https://github.com/octokit/octokit.net/blob/master/Octokit/Models/Response/CommitActivity.cs - if we could update RequiredStatusChecks.Contexts to be a more specific interface than ICollection<T> and then drop this change we should be able to get back to a green build...

Roger that, I'm the first to admit I haven't quite dealt with this many convention tests etc before, I was definitely hacking my way to success 😀

I'll see what I can do with the Contexts collection and ditch this incorrect convention test modification

ryangribble · 2015-12-14T11:23:14Z

RequiredStatusChecks.Context is now an IReadonlyList, implemented as per CommitActivity example
Removed convention test hack

Also rebased on latest master

ryangribble · 2015-12-14T11:50:33Z

Unsure about the AppVeyor test failure. All unit tests are passing for me locally?

shiftkey · 2015-12-14T20:18:44Z

@ryangribble that test seems to be rather flaky 😞

ryangribble · 2015-12-16T12:17:11Z

Looks like the rerun tests all passed etc

Is there anything further needed from me?

naveensrinivasan · 2015-12-16T13:26:05Z

Octokit/Clients/RepositoriesClient.cs

@@ -502,7 +521,8 @@ public Task<Branch> GetBranch(string owner, string repositoryName, string branch
            Ensure.ArgumentNotNullOrEmptyString(repositoryName, "repositoryName");
            Ensure.ArgumentNotNullOrEmptyString(branchName, "branchName");

-            return ApiConnection.Get<Branch>(ApiUrls.RepoBranch(owner, repositoryName, branchName));
+            const string previewAcceptsHeader = "application/vnd.github.loki-preview+json";


This const string is duplicated in 3 methods. Could we declare it at the class level and use it?

I was following an example I saw in another part of the code base where the preview header was defined each time, but I can change it to a class level.

Should the unit tests use the same define or their own class level define or specify string each time?

The licenses API? Yeah, there's two usages in MiscellaneousClient which could use a bit of 💄 to tidy up at some stage.

Let's define a field on the test class which gets reused here, and then look at the other usages separately.

- Create BranchProtection models - Add Protection element to Branch model - Adjust RepositoryClient GetBranch and GetBranches calls to set the special preview accepts header to enable Protected Branches API (currently in preview) - Adjust tests to include preview accepts header - Add test for Protection element

- Create BranchUpdate model - Add ProtectBranch and UnprotectBranch calls to RepositoryClient - Add tests

Add tests for various protected/unprotected scenarios

…erve alphabetical ordering

Rework integration test examples

…ericList

…ist passed into the ctor

… on IGenericList" This reverts commit eb01067.

Tidy up to be consistent with other model classes

…PI header in one place, change calls to use this

ryangribble · 2015-12-19T10:15:00Z

I really dont like the idea of referring to the test projects from the implementation classes (if that's what you meant) so what Ive done is created a AcceptHeaders helper class with the preview header defined, and changed the imlpementation to use that.

The tests are left as is, as I believe it's good practice to have each test specify what it's success criteria are, rather than use some commonly defined thing (that could be changed later unaware of the implication to tests that were using it).

Ive also got a separate branch and will follow this up with a new PR (after this one is accepted) where Ive gone and cleaned up all uses of Accepts headers (eg the licenses API as well as the standard json and html payloads etc) to use the new helper class, and remove the hardcoded strings from multiple places in implementation classes. Here's the commit in question: TattsGroup@44eb171
Let's get this PR merged first though, then I'll follow up with the general Accepts Headers tidy ups for all usages...

Also rebased on latest master

shiftkey · 2015-12-20T04:11:57Z

@ryangribble that's fine, happy to merge this in after the Travis build goes green and iterate on this further...

Add Protected Branches API support

shiftkey · 2015-12-20T05:22:17Z

Thanks @ryangribble!

ryangribble changed the title ~~Add Protected Branches support~~ [WIP] Add Protected Branches API support Dec 13, 2015

ryangribble force-pushed the protected-branches branch from 1b86935 to a2bc7f0 Compare December 13, 2015 13:11

shiftkey reviewed Dec 13, 2015
View reviewed changes

khellang reviewed Dec 14, 2015
View reviewed changes

ryangribble force-pushed the protected-branches branch from 24ff60e to a54a67b Compare December 14, 2015 11:20

naveensrinivasan reviewed Dec 16, 2015
View reviewed changes

shiftkey changed the title ~~[WIP] Add Protected Branches API support~~ Add Protected Branches API support Dec 16, 2015

ryangribble and others added 11 commits December 19, 2015 19:58

Add Protect/Unprotect Branch functionality

d7472d6

- Create BranchUpdate model - Add ProtectBranch and UnprotectBranch calls to RepositoryClient - Add tests

Add DebuggerDisplay property to model classes

ef82706

Change ProtectBranch/UnprotectBranch into a single EditBranch function

1af5f3b

Add tests for various protected/unprotected scenarios

Synchronize Mono projects

56e4f15

Make variable names consistent and move EditBranch() function to pres…

34bb670

…erve alphabetical ordering

Add EditBranch() to Octokit.Reactive

7033108

Add new Protection field to Branch constructor for consistency

b1542e2

Request object can have public setter

894a6bc

Response objects need protected setters and ctors with parameters

6b83122

Rework integration test examples

potential fix to convention test problems with private setter on IGen…

7cf55f3

…ericList

ryangribble and others added 5 commits December 19, 2015 19:58

Remove AddContext() and ClearContexts() in favour of just using the L…

87bb466

…ist passed into the ctor

Added EditBranch integration test for Octokit.Reactive project

053248d

Revert "potential fix to convention test problems with private setter…

7b2bf36

… on IGenericList" This reverts commit eb01067.

Make Contexts IReadonlyList implemented as per CommitActivity

7be9dc0

Tidy up to be consistent with other model classes

Add AcceptHeaders helper class to efine the ProtectedBranch preview A…

489cfdb

…PI header in one place, change calls to use this

ryangribble force-pushed the protected-branches branch from 8bb5d3c to 489cfdb Compare December 19, 2015 09:58

Fix mono projects

a6dd0af

shiftkey added a commit that referenced this pull request Dec 20, 2015

Merge pull request #996 from TattsGroup/protected-branches

a3871ef

Add Protected Branches API support

shiftkey merged commit a3871ef into octokit:master Dec 20, 2015

ryangribble mentioned this pull request Dec 23, 2015

Move all HTTP headers into AcceptHeaders helper class #1041

Merged

ryangribble deleted the protected-branches branch February 15, 2016 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Protected Branches API support #996

Add Protected Branches API support #996

ryangribble commented Dec 13, 2015

shiftkey commented Dec 13, 2015

shiftkey Dec 13, 2015

ryangribble Dec 14, 2015

shiftkey commented Dec 13, 2015

ryangribble commented Dec 14, 2015

shiftkey commented Dec 14, 2015

ryangribble commented Dec 14, 2015

ryangribble commented Dec 14, 2015

khellang Dec 14, 2015

ryangribble Dec 14, 2015

khellang Dec 14, 2015

shiftkey Dec 14, 2015

ryangribble Dec 14, 2015

ryangribble commented Dec 14, 2015

ryangribble commented Dec 14, 2015

shiftkey commented Dec 14, 2015

ryangribble commented Dec 16, 2015

naveensrinivasan Dec 16, 2015

shiftkey Dec 16, 2015

ryangribble Dec 17, 2015

shiftkey Dec 17, 2015

ryangribble commented Dec 19, 2015

shiftkey commented Dec 20, 2015

shiftkey commented Dec 20, 2015

Add Protected Branches API support #996

Add Protected Branches API support #996

Conversation

ryangribble commented Dec 13, 2015

Notes

Implementation Details

To Do List

shiftkey commented Dec 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiftkey commented Dec 13, 2015

ryangribble commented Dec 14, 2015

shiftkey commented Dec 14, 2015

ryangribble commented Dec 14, 2015

ryangribble commented Dec 14, 2015

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

ryangribble commented Dec 14, 2015

ryangribble commented Dec 14, 2015

shiftkey commented Dec 14, 2015

ryangribble commented Dec 16, 2015

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

ryangribble commented Dec 19, 2015

shiftkey commented Dec 20, 2015

shiftkey commented Dec 20, 2015