feat!: Support querying organization custom roles by tomfeigin · Pull Request #3129 · google/go-github

tomfeigin · 2024-04-16T06:16:16Z

Added support for querying, creating, updating and deleting organization custom roles.

BREAKING CHANGE: CreateOrUpdateCustomRoleOptions has been renamed to CreateOrUpdateCustomRepoRoleOptions and roleID has been changed from type string to int64.

codecov · 2024-04-16T06:19:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.90%. Comparing base (2b8c7fa) to head (1c5ad01).
Report is 41 commits behind head on master.

❗ Current head 1c5ad01 differs from pull request most recent head 41c96d8. Consider uploading reports for the commit 41c96d8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3129      +/-   ##
==========================================
- Coverage   97.72%   92.90%   -4.82%     
==========================================
  Files         153      170      +17     
  Lines       13390    11463    -1927     
==========================================
- Hits        13085    10650    -2435     
- Misses        215      723     +508     
  Partials       90       90

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

Added support for querying, creating, updating and deleting organization custom roles.

gmlewis

Thank you, @tomfeigin !
Just one addition, please, then I think we will be ready for a second LGTM+Approval before merging.

github/orgs_custom_roles_test.go

gmlewis · 2024-04-16T12:08:23Z

(Also, there is no need to force push in this repo - we always squash&merge - see CONTRIBUTING.md for details.)

gmlewis · 2024-04-16T12:14:07Z

github/orgs_custom_roles.go

+// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#update-a-custom-organization-role
+//
+//meta:operation PATCH /orgs/{org}/organization-roles/{role_id}
+func (s *OrganizationsService) UpdateCustomOrgRole(ctx context.Context, org, roleID string, opts *CreateOrUpdateOrgRoleOptions) (*CustomOrgRoles, *Response, error) {


According to: https://docs.github.com/en/rest/orgs/organization-roles?apiVersion=2022-11-28#update-a-custom-organization-role
roleID should be an integer.

Suggested change

func (s *OrganizationsService) UpdateCustomOrgRole(ctx context.Context, org, roleID string, opts *CreateOrUpdateOrgRoleOptions) (*CustomOrgRoles, *Response, error) {

func (s *OrganizationsService) UpdateCustomOrgRole(ctx context.Context, org string, roleID int64, opts *CreateOrUpdateOrgRoleOptions) (*CustomOrgRoles, *Response, error) {

gmlewis · 2024-04-16T12:14:47Z

github/orgs_custom_roles.go

+// GitHub API docs: https://docs.github.com/rest/orgs/organization-roles#delete-a-custom-organization-role
+//
+//meta:operation DELETE /orgs/{org}/organization-roles/{role_id}
+func (s *OrganizationsService) DeleteCustomOrgRole(ctx context.Context, org, roleID string) (*Response, error) {


Same here: https://docs.github.com/rest/orgs/organization-roles#delete-a-custom-organization-role

Suggested change

func (s *OrganizationsService) DeleteCustomOrgRole(ctx context.Context, org, roleID string) (*Response, error) {

func (s *OrganizationsService) DeleteCustomOrgRole(ctx context.Context, org string, roleID int64) (*Response, error) {

I'll change it but it is aligned with DeleteCustomRepoRole

Ah, bummer. Looks like I didn't catch it before on the other one. 😞

Well, since this is already a breaking API change, do you want to go ahead and make it consistent and fix the older one(s) too?

I prefer to do that in another PR because here I am just introducing new stuff so it won't break existing usage

gmlewis

Thank you, @tomfeigin !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

tomfeigin · 2024-04-16T14:00:43Z

@gmlewis this specific PR is not a breaking a change as it only introduces new API wrappers (I am pretty sure 🙃 ) can you remove the label please?

gmlewis · 2024-04-16T14:29:28Z

github/orgs_custom_roles.go

 //
 //meta:operation POST /orgs/{org}/custom-repository-roles
-func (s *OrganizationsService) CreateCustomRepoRole(ctx context.Context, org string, opts *CreateOrUpdateCustomRoleOptions) (*CustomRepoRoles, *Response, error) {
+func (s *OrganizationsService) CreateCustomRepoRole(ctx context.Context, org string, opts *CreateOrUpdateCustomRepoRoleOptions) (*CustomRepoRoles, *Response, error) {


@tomfeigin - if I'm not mistaken, this line and line 203 below are both breaking API changes, as code written by a user of this repo that calls one or both of these endpoints will no longer compile correctly without changing their code. Agreed?

Oh you are right, should I keep the previous naming then?

Let's just have the breaking change, I'll fix the other APIs to use the int64 ID

Well, honestly I don't have a problem making breaking API changes, which I suppose might be obvious from the current version number of this repo. 😂

Also, I think it is always a good idea to make things clearer and easier-to-understand for users of this repo... so I think your name change here was perfectly appropriate and that's also why I recommended fixing the bad field type while we are making a breaking API change.

So I'll leave it up to you, but I think the breaking API change is appropriate and am willing to move forward with it.

gmlewis

Thank you, @tomfeigin !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

gsaraf

Looks good to me :)

gmlewis · 2024-05-15T13:00:14Z

Thank you, @gsaraf !
Merging.

Support querying organization custom roles

83cac3c

Added support for querying, creating, updating and deleting organization custom roles.

tomfeigin force-pushed the handle-organization-roles branch from 1e8c944 to 83cac3c Compare April 16, 2024 06:21

gmlewis requested changes Apr 16, 2024

View reviewed changes

github/orgs_custom_roles_test.go Show resolved Hide resolved

gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Apr 16, 2024

gmlewis changed the title ~~Support querying organization custom roles~~ feat!: Support querying organization custom roles Apr 16, 2024

gmlewis reviewed Apr 16, 2024

View reviewed changes

Pass role ID as integer + more tests

1c5ad01

tomfeigin requested a review from gmlewis April 16, 2024 12:32

gmlewis approved these changes Apr 16, 2024

View reviewed changes

gmlewis reviewed Apr 16, 2024

View reviewed changes

Change roleID argument type to int64

41c96d8

gmlewis approved these changes Apr 16, 2024

View reviewed changes

gsaraf approved these changes May 15, 2024

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label May 15, 2024

gmlewis merged commit 4b16015 into google:master May 15, 2024

gmlewis mentioned this pull request Jul 10, 2024

Bump version of go-github to v63.0.0 #3205

Merged

lewismiddleton mentioned this pull request Jul 15, 2024

[MAINT]: Deprecate github_organization_custom_role and replace with github_organization_custom_repository_role and github_organization_custom_organization_role integrations/terraform-provider-github#2224

Closed

1 task

	func (s OrganizationsService) UpdateCustomOrgRole(ctx context.Context, org, roleID string, opts CreateOrUpdateOrgRoleOptions) (CustomOrgRoles, Response, error) {
	func (s OrganizationsService) UpdateCustomOrgRole(ctx context.Context, org string, roleID int64, opts CreateOrUpdateOrgRoleOptions) (CustomOrgRoles, Response, error) {

	func (s OrganizationsService) DeleteCustomOrgRole(ctx context.Context, org, roleID string) (Response, error) {
	func (s OrganizationsService) DeleteCustomOrgRole(ctx context.Context, org string, roleID int64) (Response, error) {

Conversation

tomfeigin commented Apr 16, 2024 • edited by gmlewis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmlewis commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

tomfeigin commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

gsaraf left a comment

Choose a reason for hiding this comment

Uh oh!

gmlewis commented May 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomfeigin commented Apr 16, 2024 •

edited by gmlewis

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading