Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (2b8c7fa) to head (da56a8b).
Report is 95 commits behind head on master.

@@            Coverage Diff             @@
##           master    #3235      +/-   ##
==========================================
- Coverage   97.72%   92.94%   -4.78%     
==========================================
  Files         153      171      +18     
  Lines       13390    11633    -1757     
==========================================
- Hits        13085    10812    -2273     
- Misses        215      727     +512     
- Partials       90       94       +4     

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

See a discussion about the approach in the relevant issue #3226

I don't think I understand the need of having separate endpoint.

It is clear for me that method CreateCustomRepoRole will fail if you call it without any of properties of type CreateOrUpdateCustomRepoRoleOptions. Which is expected and makes sense after reading GitHub Rest API

However it will also fail when you call it with defined values for each of the properties where empty array is used as value for Permission. Which is my case exactly.

The aim of this PR is to fix the bug. While still using the same methods without breaking current API as you requested :)

Taking into account above written, what would be the benefits from having extra endpoint?

So isn't it true that users will want the omitempty when they wish to not modify existing permissions?
Or are you saying that there is never a usage for not sending the permissions field?
I'll go read the docs again as well.

I know we have other endpoints where we must differentiate between an omitted field and an empty field, and I thought this was one of those cases.

This endpoint says that permissions is required: https://docs.github.com/en/enterprise-cloud@latest/rest/orgs/custom-roles?apiVersion=2022-11-28#create-a-custom-repository-role

However, this endpoint makes NO MENTION of the permissions field: https://docs.github.com/en/rest/orgs/organization-roles?apiVersion=2022-11-28#create-a-custom-organization-role

What's up with that?

So isn't it true that users will want the omitempty when they wish to not modify existing permissions?

Yes it is true. In this case I would expect Permission property to be nil, so condition will return false and parameters would be send untouched.

The intention of the changes is to rewrite type for the case when Permissions property has an empty array as value

However, this endpoint makes NO MENTION of the permissions field: https://docs.github.com/en/rest/orgs/organization-roles?apiVersion=2022-11-28#create-a-custom-organization-role

This link lead me to: Get all organization roles

If you open this link https://docs.github.com/en/enterprise-cloud@latest/rest/orgs/organization-roles?apiVersion=2022-11-28#create-a-custom-organization-role

You find permission key as required

OK, so if I'm reading the docs correctly, then the permissions field is ALWAYS required, correct?

If so, I truly apologize for chasing you down an incorrect rabbit hole that I created.

So it looks like completely removing the omitempty is simply the right choice afterall, agreed?

NP. Glad we clarified the intention here :)

Unfortunatelly type CreateOrUpdateCustomRepoRoleOptions used for both Create (where all the properties are required) and Update where all of them are optional 😀

So we either rewrite API or use tweaks like I suggested in the PR

OK, now that we've tried all the possibilities, I'll let you decide which you think is most appropriate.
Please let me know when you want me to take another look.
If it requires breaking the API, then so be it. If, however, you can avoid it, that would be nice-to-have.

@andriyun - can you please explain to me how what you have written is different from simply removing the omitempty on lines 56 and 64?

I'm honestly having troubles understanding the difference.

You are right %)
I was wrong in my assumption before and solution here is way simpler

(nit: @andriyun - In the future, please don't use force-push in this repo, as the history of all the experiments we went through and discussed are now lost and we can't refer to them in case others want to discuss them. Also, we always squash-and-merge in this repo so there is never a concern about bloated history in the final commits.)

Ohhh.. ok, got you. I have previous changes saved in separate branches locally.

Do you want me to add them?
@gmlewis

No, that's OK... if someone in the future wants to dig up what we discussed, we can try and recreate then. 😂
Thank you for offering, though! 😄

Thank you, @tomfeigin !
Merging.