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.

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 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, roleIDstring, opts*CreateOrUpdateOrgRoleOptions) (*CustomOrgRoles, *Response, error) {
func (s*OrganizationsService) UpdateCustomOrgRole(ctx context.Context, orgstring, roleIDint64, opts*CreateOrUpdateOrgRoleOptions) (*CustomOrgRoles, *Response, error) {

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

+// 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, roleIDstring) (*Response, error) {
func (s*OrganizationsService) DeleteCustomOrgRole(ctx context.Context, orgstring, roleIDint64) (*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

 //
 //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.

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

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 83cac3cCompare April 16, 2024 06:21

gmlewis requested changes Apr 16, 2024
View reviewed changes

Comment threadgithub/orgs_custom_roles_test.go

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 rolesApr 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:masterMay 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_roleintegrations/terraform-provider-github#2224
Closed
1 task

	func (sOrganizationsService) UpdateCustomOrgRole(ctx context.Context, org, roleIDstring, optsCreateOrUpdateOrgRoleOptions) (CustomOrgRoles, Response, error) {
	func (sOrganizationsService) UpdateCustomOrgRole(ctx context.Context, orgstring, roleIDint64, optsCreateOrUpdateOrgRoleOptions) (CustomOrgRoles, Response, error) {

	func (sOrganizationsService) DeleteCustomOrgRole(ctx context.Context, org, roleIDstring) (Response, error) {
	func (sOrganizationsService) DeleteCustomOrgRole(ctx context.Context, orgstring, roleIDint64) (Response, error) {

Uh oh!

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!

codecovBot 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

codecovBot commented Apr 16, 2024•
edited
Loading