Skip to content

refactor!: Pass UpdateConnectedExternalGroup request body by value via new UpdateConnectedExternalGroupRequest - #4425

Open
JamBalaya56562 wants to merge 1 commit into
google:masterfrom
JamBalaya56562:refactor/3644-external-group-value-params
Open

refactor!: Pass UpdateConnectedExternalGroup request body by value via new UpdateConnectedExternalGroupRequest#4425
JamBalaya56562 wants to merge 1 commit into
google:masterfrom
JamBalaya56562:refactor/3644-external-group-value-params

Conversation

@JamBalaya56562

Copy link
Copy Markdown
Contributor

Continues the request-body-by-value work in #3644, this time for TeamsService.UpdateConnectedExternalGroup.

The method reused the ExternalGroup response type as its request body, but per the docs the endpoint accepts exactly one body parameter, group_id, and it's required. The other four fields (group_name, updated_at, teams, members) are response-only and are ignored by the server, so the signature didn't tell callers what to fill in. The existing test shows this: it builds the request with only GroupID set, while the response expectation populates all five fields.

So this adds a dedicated request type, following the same approach as PullRequestSubmitReviewRequest in #4406:

type UpdateConnectedExternalGroupRequest struct {
	GroupID int64 `json:"group_id"`
}

GroupID is a non-pointer int64 since it's required, the body is passed by value, and the type is removed from the body-allowed-pointer-types allowlist.

Notes:

  • ExternalGroup is unchanged — it stays the response type for GetExternalGroup, ListExternalGroups, ExternalGroupList and the iterator, so its fields keep their pointer semantics.
  • The method verb Update already matches the docs operation name, so no rename.

Verified with go build ./..., go vet -tags integration ./test/integration/, gofmt, the full ./github/ test suite (UpdateConnectedExternalGroup and the generated GetGroupID at 100%), and custom-gcl (no paramcheck findings after removing the allowlist entry).

Updates #3644

BREAKING CHANGE: TeamsService.UpdateConnectedExternalGroup now takes a new UpdateConnectedExternalGroupRequest (with non-pointer GroupID) by value instead of *ExternalGroup.

cc @jvm986 — flagging for #3644 coordination; this is in the teams service, so it shouldn't overlap with the Issues work.

…via new `UpdateConnectedExternalGroupRequest`

UpdateConnectedExternalGroup reused the ExternalGroup response type as its
request body, but group_id is the only parameter the endpoint accepts, and it
is required. The new UpdateConnectedExternalGroupRequest models that schema
exactly, with a non-pointer GroupID, and is passed by value.

ExternalGroup stays unchanged as the response type, and the old entry is
removed from the .golangci.yml allowlist.

BREAKING CHANGE: TeamsService.UpdateConnectedExternalGroup now takes a new UpdateConnectedExternalGroupRequest (with non-pointer GroupID) by value instead of *ExternalGroup.
@gmlewis 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 Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.54%. Comparing base (4d45b7c) to head (e00ee2b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4425   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files         194      194           
  Lines       19780    19780           
=======================================
  Hits        19294    19294           
  Misses        268      268           
  Partials      218      218           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gmlewis gmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @JamBalaya56562!
LGTM
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants