Fix Copilot review findings from PR #305 and #308#318
Open
duanemay wants to merge 2 commits into
Open
Conversation
Require at least one update flag so update-user no longer silently no-ops, align flag names (--givenName/--familyName/--email/--phone/ --delAttrs) with create-user, and assert PUT request body contents in tests instead of only status codes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace the untyped map[string]interface{} request body with a typed
struct, and align the command doc page with the standard command-doc
template (header + command-reference backlinks, Flags/Global Flags
tables, See Also section).
The suggestion to drop the manual X-Identity-Zone-Id header was not
applied: api.Curl() bypasses the go-uaa SDK's structured request path
where WithZoneID's header injection happens, so removing it breaks
zone-scoped requests (confirmed by the existing "supports zone
switching" test failing once the header was removed).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from prior PRs by tightening update-user behavior (requiring an actual update), aligning its flag names with create-user, strengthening request-body assertions in tests, and improving change-client-secret by using a typed request payload and bringing its documentation in line with the standard command-doc template.
Changes:
update-user: require at least one update flag; rename flags to--givenName/--familyName/--email/--phone/--delAttrsto matchcreate-user.update-usertests: assert PUT request JSON body contents for updated fields and deletion behavior.change-client-secret: replacemap[string]interface{}payload with a typed struct; update docs to standard template and add “See Also” links.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/commands/update-user.md | Updates docs to the new flag names and documents the “must specify at least one update flag” behavior. |
| docs/commands/change-client-secret.md | Converts doc page to the standard template (flags/global flags/see also) and improves navigation links. |
| cmd/update_user.go | Enforces “at least one update flag” validation and renames flags to camelCase to align with create-user. |
| cmd/update_user_test.go | Adds PUT-body capture + JSON assertions for updated user payload contents (names/emails/phones/deletions). |
| cmd/change_client_secret.go | Uses a typed request struct for the secret-change payload and clarifies why zone header is set manually for api.Curl. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+23
to
+28
| return func(w http.ResponseWriter, req *http.Request) { | ||
| body, err := io.ReadAll(req.Body) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| *dest = body | ||
| req.Body = io.NopCloser(bytes.NewReader(body)) | ||
| } |
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
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.
Summary
update-user(PR Addupdate-usercommand with documentation and tests #305): require at least one update flag instead of silently no-oping, align flag names (--givenName/--familyName/--email/--phone/--delAttrs) withcreate-user, and assert PUT request body contents in tests.change-client-secret(PR Addchange-client-secretcommand for authenticated client self-service secret updates #308): replace the untypedmap[string]interface{}request body with a typed struct, and align the command doc page with the standard command-doc template. (The suggestion to drop the manualX-Identity-Zone-Idheader was investigated and not applied —api.Curl()bypasses the go-uaa SDK path where that header is normally injected, so removing it breaks zone-scoped requests.)Test plan
go build ./...go vet ./...go test ./...(246 specs passing)🤖 Generated with Claude Code