Add Copilot endpoints #2973

o-sama · 2023-10-24T10:04:44Z

Closes #2938.

codecov · 2023-10-24T10:08:16Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a354a6c) 97.71% compared to head (36bc1f4) 97.71%.

Files	Patch %	Lines
github/copilot.go	98.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2973    +/-   ##
========================================
  Coverage   97.71%   97.71%            
========================================
  Files         151      152     +1     
  Lines       13072    13241   +169     
========================================
+ Hits        12773    12939   +166     
- Misses        211      213     +2     
- Partials       88       89     +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

o-sama · 2023-10-24T11:52:13Z

CodeCov hasn't updated as of last commit, but I think it should increase the coverage to close to 100% for the new file.

gmlewis

Thank you, @o-sama - this is looking great!
Just a few tweaks, please, before we ask for a second review.

gmlewis · 2023-10-24T13:11:09Z

github/copilot.go

+	InactiveThisCycle   int64 `json:"inactive_this_cycle"`
+}
+
+type CopilotSeats struct {


Missing godoc-style comment.

github/copilot.go

github/copilot_test.go

gmlewis · 2023-10-24T13:30:02Z

github/copilot_test.go

+	"github.com/google/go-cmp/cmp"
+)
+
+// Test invalid JSON responses, vlaid responses are covered in the other tests


Suggested change

// Test invalid JSON responses, vlaid responses are covered in the other tests

// Test invalid JSON responses, valid responses are covered in the other tests

o-sama · 2023-10-24T13:38:12Z

Thanks @gmlewis, I'll add in the changes! My bad on the http method being PUT, I was doing some other work with S3 and I think I carried that context over.

gauraw10 · 2023-10-24T13:39:11Z

github/copilot.go

+	}
+
+	var copilotSeats *CopilotSeats
+	resp, err := s.client.Do(ctx, req, &copilotSeats)


How are we handling pagination here for more than 100 users?

Will be adding support for this, thanks for the callout 🙏

pagination works in current version

WillAbides

This looks good over all. I realize I left quite a few comments, but they are mostly nit-picks about naming and comments.

github/copilot.go

WillAbides · 2023-10-24T12:23:05Z

github/copilot.go

+		return nil, nil, err
+	}
+
+	var SeatCancellations *SeatCancellations


The var name should be downcased "seatCancellations"

Will do, my bad there

WillAbides · 2023-10-24T12:23:32Z

github/copilot.go

+		return nil, nil, err
+	}
+
+	var SeatCancellations *SeatCancellations


Downcase seatCancellations

github/copilot.go

WillAbides · 2023-10-24T13:04:07Z

github/copilot.go

+// CopilotSeatDetails represents the details of a Copilot for Business seat.
+// Assignee can either be a User, Team, or Organization.
+type CopilotSeatDetails struct {
+	Assignee                interface{} `json:"assignee"`


It may be ok to use *User here. There are a few other places where we use *User and let users distinguish using the Type field. @gmlewis will know what the preferred method is.

I feel like this might be a better option since we're explicitly setting the correct type for the field, but both approaches leave type-checking to the end user.

There are minor differences as far as I'm aware between the types, e.g. Team has no Login field, but has a Name field, which both User and Organization have. Login is used for the username in the case of a user, whereas Name is used for team name.

I'll leave you guys with these thoughts and will be happy to make changes based on what you think.

That's a good point about Team being different. I think this might be the best way to handle it after all.

WillAbides · 2023-10-24T13:38:18Z

github/copilot.go

+
+// AddCopilotTeams Adds teams to the Copilot for Business subscription for an organization
+//
+// https://docs.github.com/en/rest/copilot/copilot-for-business#add-teams-to-the-copilot-for-business-subscription-for-an-organization


Suggested change

// https://docs.github.com/en/rest/copilot/copilot-for-business#add-teams-to-the-copilot-for-business-subscription-for-an-organization

// GitHub API docs: https://docs.github.com/en/rest/copilot/copilot-for-business#add-teams-to-the-copilot-for-business-subscription-for-an-organization

github/copilot.go

WillAbides · 2023-10-24T13:38:59Z

github/copilot.go

+
+// RemoveCopilotUsers Removes users from the Copilot for Business subscription for an organization
+//
+// https://docs.github.com/en/rest/copilot/copilot-for-business#remove-users-from-the-copilot-for-business-subscription-for-an-organization


Suggested change

// https://docs.github.com/en/rest/copilot/copilot-for-business#remove-users-from-the-copilot-for-business-subscription-for-an-organization

// GitHub API docs: https://docs.github.com/en/rest/copilot/copilot-for-business#remove-users-from-the-copilot-for-business-subscription-for-an-organization

WillAbides · 2023-10-24T13:39:14Z

github/copilot.go

+
+// GetSeatDetails Gets Copilot for Business seat assignment details for a user
+//
+// https://docs.github.com/en/rest/copilot/copilot-for-business#get-copilot-for-business-seat-assignment-details-for-a-user


Suggested change

// https://docs.github.com/en/rest/copilot/copilot-for-business#get-copilot-for-business-seat-assignment-details-for-a-user

// GitHub API docs: https://docs.github.com/en/rest/copilot/copilot-for-business#get-copilot-for-business-seat-assignment-details-for-a-user

gmlewis · 2023-10-24T13:54:53Z

Thank you, @WillAbides for your review feedback and @gauraw10 for your excellent pagination question which I seem to frequently forget about until it becomes an issue later, unfortunately. It is definitely better to add pagination support up front rather than later. 😁

I've got some work to do but see that there are some questions for me, so I will try to circle back later this afternoon and address them.

WillAbides · 2023-10-24T14:06:56Z

github/copilot_test.go

+	tests := map[string]struct {
+		data    string
+		want    *CopilotSeatDetails
+		wantErr bool
+	}{


This could be:

tests := map[string]struct { name string data string } { { name: "invalid JSON", data: `{`, }, ... }

Two things changed here:

Make it a slice instead of a map for defined order when testing.

Because this only tests invalid, the "want" and "wantErr" fields are always the same.

I can make that change but want your thoughts on keeping the want and wantErr fields. Reason being is as of right now I'm only testing invalid cases and the unmarshaling isn't too complex, but in the future we might be adding valid cases here.

I'll defer to your judgement on want and wantErr. We may or may not need them in the future, and it doesn't hurt anything to keep them around.

WillAbides · 2023-10-24T14:15:16Z

github/copilot_test.go

+		seatDetails := &CopilotSeatDetails{}
+
+		t.Run(name, func(t *testing.T) {
+			err := seatDetails.UnmarshalJSON([]byte(tc.data))


It's better to test with json.Unmarshal instead of directly calling UnmarshalJSON because that is how users will call it.

Good call 🙂

o-sama · 2023-10-24T14:34:41Z

Apologies guys, clearly this isn't my best work. I really appreciate the review comments. I'll apply most suggestions and reply to the few that I think we should discuss more.

@gauraw10 Similarly to @gmlewis, I tend to forget about pagination sometimes. I'll add support for that.

Co-authored-by: WillAbides <233500+WillAbides@users.noreply.github.com> Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

gmlewis · 2023-10-24T15:23:15Z

Watching the conversations go by, I'm wondering if it might be nice to have 3 helper methods on the struct like GetUser() (*User, bool), etc.

Thoughts?

o-sama · 2023-10-24T15:26:37Z

@gmlewis Would that be while keeping Assignee as interface{}? I'm in favour of that since it allows easy checking/fetching with if, ok.

gmlewis · 2023-10-24T15:42:36Z

@gmlewis Would that be while keeping Assignee as interface{}? I'm in favour of that since it allows easy checking/fetching with if, ok.

Yes, that's what I was thinking might be nice, but I'm open to ideas.

WillAbides · 2023-10-25T14:05:59Z

I think that UnmarshalJSON will panic if "type" is not set. Would you add a test case where "type" isn't set and fix the issue if it panics?

o-sama · 2023-10-25T14:09:34Z

Good call, I wanted to expand coverage for that 🙂

What do you think about not returning errors for unmarshaling the Assignee field once we've done the type assertions? I'm not sure we would get errors there since we know the map matches the type of the struct we're unmarshaling into, and I can't think of a way to test for that.

WillAbides · 2023-10-25T14:18:25Z

In general, I would much rather have untested error branches than trust our assumption that an error can't happen and end up with a panic. Even though codecov puts a big, ugly ❌ on your PR, it isn't a hard rule that test coverage can't decrease.

That said, I think it will be possible to cover those cases with just the right json data. Let me have a look and see if I can come up with something.

WillAbides · 2023-10-25T14:25:32Z

@o-sama I see you have already cases that should cover those lines but don't. I think the problem is the comma at the end of "id": "bad",. That's causing the first json.Unmarshal to return an error so it doesn't make it to the part you are trying to test.

o-sama · 2023-10-25T14:34:09Z

Ah good catch @WillAbides, it's always the commas 🙃

gmlewis · 2023-12-19T13:15:01Z

@gmlewis I know that second lgtm is needed. However, is there somekind of way to find that another person who could give that LGTM? This has been waiting for sometime and at least we would like to use this feature.

OK, first let me resolve the merge conflicts, @zetaab - then maybe @valbeat has time to take a look at this one.

Fixes: google#2994.

Fixes: google#2996.

Fixes: google#2851.

Fixes: google#2998.

Fixes: google#3021.

Fixes: google#3006.

Fixes: google#3031.

Fixes: google#3018.

Fixes: google#3007.

)

Fixes: google#3014.

Fixes: google#3016.

gmlewis · 2023-12-19T13:34:39Z

@zetaab - you yourself could move this PR along by performing a review and giving it an LGTM+Approval.

zetaab

I could not find anything else than that one type change. I am currently coding against this PR and it was little bit weird that the type is not similar than other timestamps.

zetaab · 2023-12-19T14:11:13Z

github/copilot.go

+	// Assignee can either be a User, Team, or Organization.
+	Assignee                interface{} `json:"assignee"`
+	AssigningTeam           *Team       `json:"assigning_team,omitempty"`
+	PendingCancellationDate *string     `json:"pending_cancellation_date,omitempty"`


type should be *Timestamp instead that it works in similar way than other time fields.

zetaab · 2023-12-19T14:13:07Z

github/copilot.go

+	}
+
+	var copilotSeats *CopilotSeats
+	resp, err := s.client.Do(ctx, req, &copilotSeats)


pagination works in current version

zetaab

actually that PendingCancellationDate field is date so perhaps leave it like this.

LGTM

gmlewis · 2023-12-19T14:31:25Z

Thank you, @zetaab !
Merging.

Add Copilot Endpoints

aaa5b5b

o-sama added 5 commits October 24, 2023 06:08

clean up unmarshal func

67e0a68

Add coverage and adddress linting issues

3e32bab

undo wrong file committed

8dddf5f

Add json unmarshal tests for seat details

0864167

update seat details unmarshal tests

9d5be67

gmlewis requested changes Oct 24, 2023

View reviewed changes

gauraw10 reviewed Oct 24, 2023

View reviewed changes

WillAbides suggested changes Oct 24, 2023

View reviewed changes

WillAbides reviewed Oct 24, 2023

View reviewed changes

Apply suggestions from code review

3e27021

Co-authored-by: WillAbides <233500+WillAbides@users.noreply.github.com> Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

o-sama added 3 commits October 24, 2023 19:33

code review comments

568617d

Add coverage for Assignee helper methods

19339c2

Add coverage for Assignee helper methods error cases

4f78d68

Add requested test case

2c90d0c

Fix unmarshal test cases

e01ef53

caseyduquettesc and others added 20 commits December 19, 2023 08:16

Add default branch to repository edit event (google#2995)

766902b

Fixes: google#2994.

Add Draft to Issue type (google#2997)

76779c0

Fixes: google#2996.

Fix secondary rate limits URL (google#3001)

baf2515

Bump golang.org/x/net from 0.18.0 to 0.19.0 in /scrape (google#3003)

2adca97

Implement global security advisories API (google#2993)

b0181bb

Fixes: google#2851.

Change PushEvent.Pusher type to CommitAuthor (google#2999)

c7a9ad7

Fixes: google#2998.

Bump version of go-github to v57.0.0 (google#3009)

56a8c95

Bump go-github from v56 to v57 in /scrape (google#3010)

5390049

Update metadata (google#3012)

9f70f1f

Fix broken CreateOrUpdateRepoCustomPropertyValues (google#3023)

5e63691

Fixes: google#3021.

Bump actions/setup-go from 4 to 5 (google#3027)

cec367c

Add scanning validity checks (google#3026)

1080dff

Fixes: google#3006.

Add Referrer field to AuditEntry (google#3032)

27decbb

Fixes: google#3031.

Add code_search and dependency_snapshots for RateLimits (google#3019)

f5b837b

Fixes: google#3018.

Support temporary private fork creation via API (google#3025)

f53e74d

Fixes: google#3007.

Escape package names to support names which include a slash (google#3002

0053173

)

Don't update httpClient passed to NewClient (google#3011)

25e042b

Add GetAllCustomPropertyValues for repositories (google#3020)

6e03d4e

Fixes: google#3014.

Remove ambiguous fields from AuditEntry (google#3017)

428a0da

Fixes: google#3016.

Merge branch 'master' into add-copilot-endpoints

36bc1f4

gmlewis changed the title ~~Add Copilot Endpoints~~ Add Copilot endpoints Dec 19, 2023

zetaab suggested changes Dec 19, 2023

View reviewed changes

zetaab approved these changes Dec 19, 2023

View reviewed changes

gmlewis merged commit 9d6658a into google:master Dec 19, 2023
7 checks passed

zetaab mentioned this pull request Dec 19, 2023

Fix copilot API payload #3034

Merged

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Dec 19, 2023

gmlewis mentioned this pull request Jan 10, 2024

Bump version of go-github to v58.0.0 #3045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Copilot endpoints #2973

Add Copilot endpoints #2973

	// Test invalid JSON responses, vlaid responses are covered in the other tests
	// Test invalid JSON responses, valid responses are covered in the other tests

	// https://docs.github.com/en/rest/copilot/copilot-for-business#add-teams-to-the-copilot-for-business-subscription-for-an-organization
	// GitHub API docs: https://docs.github.com/en/rest/copilot/copilot-for-business#add-teams-to-the-copilot-for-business-subscription-for-an-organization

Add Copilot endpoints #2973

Add Copilot endpoints #2973

Conversation

Codecov Report

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

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

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