Update security manager operations to use organization roles REST APIs #3421
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3421 +/- ##
==========================================
- Coverage 97.72% 92.19% -5.53%
==========================================
Files 153 173 +20
Lines 13390 14949 +1559
==========================================
+ Hits 13085 13782 +697
- Misses 215 1072 +857
- Partials 90 95 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you, @just-joshing.
I've raised a pagination concern that hopefully you can address.
| } | ||
| } | ||
|
|
||
| return nil, resp, errors.New("security manager role not found") |
There was a problem hiding this comment.
Please add a unit test that covers this error.
| return teams, resp, nil | ||
| options := &ListOptions{PerPage: 100} | ||
| securityManagerTeams := make([]*Team, 0) | ||
| for { |
There was a problem hiding this comment.
With this change, this will be the first endpoint that handles pagination within the call itself, whereas all other endpoints in this repo expect the caller to handle pagination (which also typically involves checking for rate limit errors).
Honestly, I'm not seeing the need here to introduce pagination in this endpoint.
We have a section in the README.md discussing this issue.
Do you have a strong argument for including pagination here?
If we start doing this, then others will start wanting to add it to their favorite endpoint too, and I'm concerned that this will snowball and one implementation won't solve everyone's needs for pagination.
There was a problem hiding this comment.
Hi @gmlewis! The reason I did that is because the previous API did not support pagination, so I didn't want to change the contract here. My goal was for these operations to continue to work the same way they did previously so consumers of the library would not face any interruptions from these when updating to newer versions of the library.
But based on other feedback here as well, it sounds like it might be more desirable to keep the library closer to a 1:1 with the GitHub APIs, so I will mark them as deprecated instead.
|
@just-joshing why not mark these methods as deprecated (as has been done for other deprecated endpoint methods)? The org role methods are already available in this package and I don't see a benefit to adding this extra complexity? |
Hi @stevehipwell! The reason I took this route was because I believed it would be beneficial to consumers of the library since they could update to newer versions without needing to update any code using the security manager teams operations. These operations can continue to be supported by the library. It's just the internals that change. But if the library is supposed to be closer to a 1:1 representation of the GitHub APIs, then I can understand wanting to deprecate instead. I'll close this PR. |
|
@stevehipwell, @gmlewis, here's the replacement PR that adds deprecation messages instead. Thank you for reviewing this PR. |
The organization security manager role REST APIs are being sunset (https://gh.io/security-managers-rest-api-sunset) in favor of the organization roles REST APIs.
Instead of introducing a breaking change by removing the operations from go-github, this PR updates the operations to use the organization roles REST APIs so consumers (such as Terraform) can continue to manage organization security manager teams without needing to change code.