refactor!: Pass LDAP mapping request bodies by value via new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest - #4432
Open
JamBalaya56562 wants to merge 1 commit into
Conversation
…serLDAPMappingRequest` and `UpdateTeamLDAPMappingRequest` UpdateUserLDAPMapping and UpdateTeamLDAPMapping reused the UserLDAPMapping and TeamLDAPMapping response types as their request bodies, but ldap_dn is the only parameter either endpoint accepts, and it is required. The new request types model that schema exactly, with a non-pointer LDAPDN, and are passed by value. The response types stay unchanged, the teamID parameter is renamed for clarity, and both old entries are removed from the .golangci.yml allowlist. BREAKING CHANGE: AdminService.UpdateUserLDAPMapping and UpdateTeamLDAPMapping now take new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest (with non-pointer LDAPDN) by value instead of *UserLDAPMapping and *TeamLDAPMapping.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4432 +/- ##
=======================================
Coverage 97.54% 97.54%
=======================================
Files 194 194
Lines 19836 19836
=======================================
Hits 19350 19350
Misses 268 268
Partials 218 218 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continues the request-body-by-value work in #3644, this time for the two LDAP mapping endpoints on
AdminService.Both methods reused their response types as request bodies, but per the GHES docs each endpoint accepts exactly one body parameter,
ldap_dn, and it's required:UpdateUserLDAPMappingtook*UserLDAPMapping— 17 all-pointer fields, most of them server-generated (avatar_url,events_url, …).UpdateTeamLDAPMappingtook*TeamLDAPMapping— 10 all-pointer fields, same story.The existing tests show this: both build the request with only
LDAPDNset, while the response expectations populate more fields. So this adds dedicated request types, following the same approach asPullRequestSubmitReviewRequest(#4406) andUpdateConnectedExternalGroupRequest(#4425):LDAPDNis a non-pointerstringsince it's required, both bodies are passed by value, and both old entries are removed from thebody-allowed-pointer-typesallowlist. The two methods live in the same file and follow the identical pattern, so they're converted together.Notes:
UserLDAPMappingandTeamLDAPMappingare unchanged — they stay the response types, so their fields keep pointer semantics.team int64parameter is renamed toteamIDfor clarity (non-breaking).Verified with
go build ./...,go vet -tags integration ./test/integration/,gofmt, the full./github/test suite (both methods and the generatedGetLDAPDNaccessors at 100%), andcustom-gcl(noparamcheckfindings after removing the allowlist entries).Updates #3644
BREAKING CHANGE: AdminService.UpdateUserLDAPMapping and UpdateTeamLDAPMapping now take new UpdateUserLDAPMappingRequest and UpdateTeamLDAPMappingRequest (with non-pointer LDAPDN) by value instead of *UserLDAPMapping and *TeamLDAPMapping.
cc @jvm986 — flagging for #3644 coordination; this is in the
adminservice, so it shouldn't overlap with theIssueswork.