Protected Branches updates - main methods #1441

ryangribble · 2016-08-10T05:39:58Z

The updates to Protected Branches preview API added a significant number of new endpoints allowing granularity in setting each segment of the branch protection settings separately.

For an MVP (Minimum Viable Product) approach, this PR implements only the top level methods, which will enable octokit.net users to manage all aspects of BranchProtection from that top level. If there is need/desire, the other more granular methods can be added in separate PRs

This PR implements the following methods:

    Task<BranchProtectionSettings> GetBranchProtection(string owner, string name, string branch);
    Task<BranchProtectionSettings> GetBranchProtection(int repositoryId, string branch);
    Task<BranchProtectionSettings> UpdateBranchProtection(string owner, string name, string branch, BranchProtectionSettingsUpdate update);
    Task<BranchProtectionSettings> UpdateBranchProtection(int repositoryId, string branch, BranchProtectionSettingsUpdate update);
    Task<bool> DeleteBranchProtection(string owner, string name, string branch);
    Task<bool> DeleteBranchProtection(int repositoryId, string branch);

So far only non reactive client and unit tests

…pdate request that doesnt take a restrictions parameter

- fix Update method PUT parameters - add [SerializeNull] to various request fields - fix deserialize problem with ProtectedBranchRestrictions ctor not being public - tidy up DebuggerDisplay output

…eRepositoryBranchesClient Add unit tests for Observable methods

hnrkndrssn · 2016-08-10T09:05:28Z

Octokit/Models/Response/BranchProtection.cs

+    }
+
+    [DebuggerDisplay("{DebuggerDisplay,nq}")]
+    public class BranchProtectionRequiredStatusChecks


XMLDocs for consistency?

hnrkndrssn · 2016-08-10T09:07:15Z

Apart from (possibly) a couple of missing XMLDocs, the code looks good to me 👍

hnrkndrssn · 2016-08-11T00:57:35Z

Looks good to me!

ryangribble · 2016-08-12T21:33:29Z

@shiftkey let me know if you want to 👀 this over or if you're happy for me to merge based on @alfhenrik's review

shiftkey · 2016-08-15T05:10:37Z

Octokit.Tests.Integration/Clients/RepositoryBranchesClientTests.cs

@@ -23,6 +25,9 @@ public class TheGetAllMethod
            {
                Assert.NotNull(branch.Protection);
            }
+
+            // Ensure Protected attribute is deserialized (at least master branch should be true)
+            Assert.True(branches.Any(x => x.Protected));


Could we make this test assert the state of master and 🔥 this comment?

shiftkey · 2016-08-15T06:25:02Z

@ryangribble just gave it a look over, a few little comments but nothing serious

👍 merge at your leisure

ryangribble · 2016-08-16T03:36:11Z

Ive fixed the comments and 💄 just waiting for any suggestions on that comment regarding a nicer ctor. But otherwise this can be merged

Change team and user lists to specific classes derived from Collection<T> so we can offer better configuration of BranchProtectionPushRestriction via multiple ctors (teams only, users only, teams and users, etc) Add another ctor to BranchProtectionPushRestrictions for the case where no teams/users are specified (ie admin only) Change organization update tests to use this new ctor and assert we get empty lists rather than no push restrictions

ryangribble · 2016-08-25T21:27:37Z

OK ive implemented the user and team push restrictions settings be classes derived from Collection<string> which means we can now offer nicer ctors to better configure those options, rather than having to pass empty arrays etc

// Only admins can push
new BranchProtectionPushRestrictionsUpdate();

// Specify some teams that can push
new BranchProtectionPushRestrictionsUpdate(
    new BranchProtectionTeamCollection { "my-team1", "my-team2" });

// specify some users that can push
new BranchProtectionPushRestrictionsUpdate(
    new BranchProtectionUserCollection { "user1", "user2" });

// spceify both teams AND users that can push
new BranchProtectionPushRestrictionsUpdate(
    new BranchProtectionTeamCollection { "my-team1", "my-team2" },
    new BranchProtectionUserCollection { "user1", "user2" });

ryangribble · 2016-08-25T21:29:49Z

Also I realise some of the names are pretty verbose (eg BranchProtectionRequiredStatusChecks) but that's because most of the simpler names (eg RequiredStatusChecks) are still taken by the previous implementation which we've flagged as [Obsolete] but still need to keep around for another release or 2...

shiftkey · 2016-08-29T04:04:37Z

@ryangribble that all seems great ✨

ryangribble added 8 commits August 8, 2016 22:09

First cut at adding Get/Update/Delete methods for branch protection

466d076

So far only non reactive client and unit tests

Add comments, move request classes to new file

3bd7323

push restrictions only apply to org repos so provide a ctor for the u…

2c7d6fb

…pdate request that doesnt take a restrictions parameter

add protected field to Branch

8a002b9

debug implementation while writing integration tests

51e2380

- fix Update method PUT parameters - add [SerializeNull] to various request fields - fix deserialize problem with ProtectedBranchRestrictions ctor not being public - tidy up DebuggerDisplay output

not allowed null for users/teams, clarify usage in comments

4ea8c70

Add integration tests for Get, Update, Delete BranchProtection methods

b0bc707

Implement Get Update and Delete BranchProtection methods in Observabl…

998af89

…eRepositoryBranchesClient Add unit tests for Observable methods

ryangribble changed the title ~~Protected Branches preview v2 MVP~~ Protected Branches updaets - main methods Aug 10, 2016

Tidy up a few Xml comments

7bc44d4

hnrkndrssn reviewed Aug 10, 2016
View reviewed changes

ryangribble changed the title ~~Protected Branches updaets - main methods~~ Protected Branches updates - main methods Aug 10, 2016

tweak comments

6ebf193

martinscholz83 mentioned this pull request Aug 12, 2016

Protected Branches Preview API Update - granular methods #1442

Closed

shiftkey reviewed Aug 15, 2016
View reviewed changes

shiftkey mentioned this pull request Aug 15, 2016

v0.22 - Ridgy Didge #1444

Merged

10 tasks

ryangribble added 2 commits August 15, 2016 21:13

Tidy up asserts

0b1280e

Move comment to <remarks> section

c96ee7a

ryangribble added 2 commits August 25, 2016 06:40

Merge branch 'master' into protected-branches-v2

46dc145

Fixup string.Format casing for consistency

0334730

shiftkey merged commit 4808941 into octokit:master Aug 29, 2016

ryangribble mentioned this pull request Aug 29, 2016

Protected branches updates - granular methods #1443

Merged

ryangribble deleted the protected-branches-v2 branch August 30, 2016 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protected Branches updates - main methods #1441

Protected Branches updates - main methods #1441

ryangribble commented Aug 10, 2016 •

edited

Loading

hnrkndrssn Aug 10, 2016

hnrkndrssn commented Aug 10, 2016

hnrkndrssn commented Aug 11, 2016

ryangribble commented Aug 12, 2016

shiftkey Aug 15, 2016

shiftkey commented Aug 15, 2016

ryangribble commented Aug 16, 2016

ryangribble commented Aug 25, 2016 •

edited

Loading

ryangribble commented Aug 25, 2016 •

edited

Loading

shiftkey commented Aug 29, 2016

Protected Branches updates - main methods #1441

Protected Branches updates - main methods #1441

Conversation

ryangribble commented Aug 10, 2016 • edited Loading

hnrkndrssn Aug 10, 2016

Choose a reason for hiding this comment

hnrkndrssn commented Aug 10, 2016

hnrkndrssn commented Aug 11, 2016

ryangribble commented Aug 12, 2016

shiftkey Aug 15, 2016

Choose a reason for hiding this comment

shiftkey commented Aug 15, 2016

ryangribble commented Aug 16, 2016

ryangribble commented Aug 25, 2016 • edited Loading

ryangribble commented Aug 25, 2016 • edited Loading

shiftkey commented Aug 29, 2016

ryangribble commented Aug 10, 2016 •

edited

Loading

ryangribble commented Aug 25, 2016 •

edited

Loading

ryangribble commented Aug 25, 2016 •

edited

Loading