[BB-1588] - Add Wrap Errors With Grpc - JumpCloud Connector #23
Conversation
WalkthroughAdds a centralized Go Client that wraps two generated API clients (jcapi1, jcapi2) and an extension HTTP client; adds pagination and error-wrapping helpers; and updates import paths and test import/formatting across jcapi1/jcapi2 docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Client
end
rect rgba(200,255,220,0.5)
participant JCAPI1
participant JCAPI2
participant ExtensionHTTP
end
Client->>Client: NewClient(ctx, apiKey, orgId)
Client->>JCAPI1: init API client (shared HTTP client, headers, user-agent)
Client->>JCAPI2: init API client (shared HTTP client, headers, user-agent)
Client->>ExtensionHTTP: init base HTTP client
Client->>JCAPI1: ListApplications(ctx, opts) [inject x-api-key]
JCAPI1-->>Client: Applications list (+resp)
Client->>Client: getNextPageToken / wrapSDKError if needed
Client->>JCAPI2: ListDirectories(ctx, opts) [inject x-api-key]
JCAPI2-->>Client: Directories list (+resp)
Client->>ExtensionHTTP: UserListRequest.Execute(ctx) [GET /api/users, x-api-key, x-org-id]
ExtensionHTTP-->>Client: Users list (+resp)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/connector/ext_user_list.go (2)
67-75: Resource leak: response body may not be closed on network error.When
ulr.client.Do(req)fails,respis nil, so thedefer resp.Body.Close()on line 75 would panic if reached. However, since the error returns before the defer, this is safe. But ifclient.Doreturns both a non-nil response and an error (which is valid per Go'shttp.Client.Dodocumentation for redirect errors), the body won't be closed.Consider closing the body defensively:
resp, err := ulr.client.Do(req) if err != nil { + if resp != nil { + resp.Body.Close() + } return nil, nil, uhttp.WrapErrors( codes.Unavailable, "failed to execute extension user list request: network or connection error", err, ) } defer resp.Body.Close()
126-134: Same consideration for response body handling on error.Same pattern as above -
http.Client.Docan return both a response and an error in redirect scenarios.resp, err := ugr.client.Do(req) if err != nil { + if resp != nil { + resp.Body.Close() + } return nil, nil, uhttp.WrapErrors( codes.Unavailable, "failed to execute extension user get request: network or connection error", err, ) } defer resp.Body.Close()pkg/connector/errors.go (1)
45-143: Comprehensive error classification; consider minor future‑proofingThe classification logic for nil/non‑nil
respis thorough: context cancellations, timeouts, URL configuration, and generic network failures are all mapped to appropriate gRPC codes, and HTTP‑backed errors get clear, operation‑specific messages. Two small, non‑blocking nits you could consider later:
- If you ever want to classify non‑
url.Errornetwork issues, you might also check fornet.Errorso non‑URL transports get similar treatment.- In the
codes.Unavailablecase, theelsebranch is currently unreachable givenhttpStatusToGRPCCode, but it’s harmless and could be left as future‑proofing or removed for clarity.Functionally, this is in good shape.
pkg/connector/groups.go (2)
157-203: Idempotent Grant semantics and unexpected‑status handling look correctThe Grant logic now:
- Rejects non‑user principals with a clear error (line 167).
- Treats HTTP 409 as success for “already a member” (lines 177‑183), closing the body defensively.
- Wraps other errors via
wrapSDKErrorwith a clear operation string (line 185).- For non‑error responses, flags any status other than 204/409 as unexpected, maps it to a gRPC code via
httpStatusToGRPCCode, logs, and returns a wrapped error (lines 189‑203).This gives you idempotent behavior while still surfacing genuinely unexpected responses. One optional refinement: for the invalid‑principal‑type case you could also use
uhttp.WrapErrors(codes.InvalidArgument, ...)to keep everything gRPC‑typed, but the current behavior is functionally fine.
208-260: Symmetric Revoke handling with not‑found as successRevoke mirrors Grant well:
- Non‑user principals are rejected with a clear error (line 221).
- HTTP 404 on error is treated as “already not a member” success, with the body closed when present (lines 231‑237).
- Other errors are wrapped via
wrapSDKErrorwith logging (lines 238‑240).- Non‑error responses that are not 204/404 are wrapped via
uhttp.WrapErrorswith a mapped gRPC code and logged before returning (lines 243‑257).Semantics and resource handling look correct. As with Grant, you could optionally wrap the “only users can have group membership revoked” error in a gRPC code for consistency, but it’s not required.
pkg/connector/users.go (1)
227-249: Manager fetch errors are now gRPC‑typed and loggedOn manager lookup failures, you now:
- Log with context (manager ID) (lines 238‑242).
- Wrap the error via
wrapSDKErrorwith an operation string (line 243).Given that
userTraitintentionally ignores non‑nil errors fromfetchManagerto keep trait construction best‑effort, this wrapping is mostly useful for any future callers that decide to surface the error. Current behavior remains safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/connector/apps.go(10 hunks)pkg/connector/connector.go(3 hunks)pkg/connector/errors.go(1 hunks)pkg/connector/ext_user_list.go(5 hunks)pkg/connector/groups.go(10 hunks)pkg/connector/helper.go(2 hunks)pkg/connector/pagination.go(3 hunks)pkg/connector/users.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(90-107)
🔇 Additional comments (20)
pkg/connector/pagination.go (1)
4-4: LGTM! Error wrapping improvements are well-implemented.The added context to error messages using
fmt.Errorfwith%wverb correctly preserves the error chain while providing descriptive context about each failure point in pagination operations.Also applies to: 14-14, 21-21, 35-35
pkg/connector/helper.go (1)
45-57: LGTM! Error handling improvements are consistent.The empty email validation error provides clear context, and
wrapSDKErroris correctly used for the API call to include response details in the error.pkg/connector/apps.go (4)
35-77: LGTM! Comprehensive error wrapping in the List method.All error paths now include descriptive context. The use of
wrapSDKErrorfor API calls andfmt.Errorfwith%wfor internal errors provides a consistent and debuggable error chain.
79-94: LGTM! Error context in resource creation.Wrapping the trait construction error provides clear context about where failures occur during app resource creation.
142-195: LGTM! Admin grants error handling is well-structured.The
wrapExtensionErrorfor the extension API call andfmt.Errorffor the system user fetch provide appropriate context for debugging grant processing failures.
197-291: LGTM! Grants method error handling is thorough.All pagination and API error paths are wrapped with descriptive messages. The error at line 250 helpfully includes the current pagination state for debugging unexpected conditions.
pkg/connector/ext_user_list.go (2)
136-151: LGTM! Explicit NotFound handling before generic status check.The explicit handling of
http.StatusNotFoundwithcodes.NotFoundbefore the generic non-OK check provides clear semantics for 404 responses, which is important for downstream error handling.
77-84:httpStatusToGRPCCodefunction is properly defined and handles all relevant HTTP status codes.The function exists in
pkg/connector/errors.gowith comprehensive handling: explicit cases for 400, 401, 403, 404, 409, 429, 500, 502, 503, 504, and sensible fallbacks for unhandled codes (5xx→Unavailable, 4xx→InvalidArgument, others→Unknown).pkg/connector/connector.go (2)
90-107: LGTM! Connector initialization error handling is well-structured.The option application error wrapping and the gRPC-compatible error for missing client configuration provide clear error messages for initialization failures. Using
codes.InvalidArgumentis semantically correct for configuration issues.
148-168: LGTM! Validate method error handling is comprehensive.The dual error handling approach correctly covers both SDK errors (via
wrapSDKError) and unexpected HTTP status codes (viahttpStatusToGRPCCodeanduhttp.WrapErrors). The logging before returning errors aids debugging.pkg/connector/errors.go (2)
14-43: HTTP → gRPC status mapping looks solidThe mapping for common 4xx/5xx codes and the generic 4xx/5xx fallbacks is consistent with typical gRPC semantics and should give Baton good retryability signals. No issues from a correctness standpoint.
145-150: Good reuse for extension errorsReusing
wrapSDKErrorfor the extension client keeps HTTP error semantics consistent across SDK and extension calls. No concerns here.pkg/connector/groups.go (3)
13-59: Consistent HTTP error wrapping for group listingUsing
wrapSDKErrorforGroupsUserListerrors (line 43) and adding contextual messaging around skip‑token marshaling (line 58) makes failures both gRPC‑classifiable and easier to debug. This is a clean, consistent upgrade over bare error returns.
63-87: Improved context when building group traits/resourcesThe new wrapped errors in
groupResourceandgroupTrait(lines 67, 87) give precise context about where trait/resource construction failed, which will be very helpful when protobuf struct creation or annotations fail. No functional issues.
120-155: Grants pagination errors now classified and contextualWrapping the membership list error with
wrapSDKError(line 135) and adding context to skip‑token marshal failures (line 152) aligns this method with the rest of the connector’s error‑handling approach and should make pagination issues much easier to diagnose. Looks good.pkg/connector/users.go (5)
50-88: Admin vs system user grant handling and extension error wrappingThe Grants implementation now clearly distinguishes admin users from system users:
codes.NotFoundfrom the extension client is treated as “system user, no role grants” (lines 53‑56), which matches the comment and avoids noisy errors for non‑admin accounts.- Other errors are wrapped via
wrapExtensionError(line 57), preserving HTTP context and mapping to gRPC codes.This is a sensible and consistent approach with the new shared error helpers.
113-163: User listing paths now produce structured, contextual errorsBoth branches of List now:
- Wrap
SystemusersListfailures withwrapSDKErrorand an operation string (line 115).- Wrap admin user list failures from the extension with
wrapExtensionError(line 133).- Use a clearer error message for unknown pagination states (line 162).
Pagination behavior is unchanged, but failures should now be much easier to classify and debug.
168-203: Admin user resource construction now surfaces clearer failuresWrapping
sdkResources.NewUserResourceerrors with a descriptive message (line 199) makes it immediately obvious that the failure happened during admin user resource creation, without losing the original error. No issues here.
205-218: User resource creation now clearly attributes trait failuresIf
userTraitfails, the new wrapper (line 208) makes the call site explicit (“during user resource creation”), which is very helpful given the number of places traits can fail. Behavior on success is unchanged.
251-257: User trait profile construction now reports protobuf struct failures clearlyWrapping
structpb.NewStructerrors with a contextual message (line 256) is a nice improvement for diagnosing unexpected profile‑encoding issues. The rest of the trait building remains unchanged.
There was a problem hiding this comment.
we should use uhttp client here and remove all manual error handling
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
baf701a to
cbd6e3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/users.go (1)
45-86: Potential nil response body issue when adminUser is nil.When
adminUserisnil(404 case), the code returns early on line 55-56, butdefer resp.Body.Close()on line 57 is placed after the early return. Looking atext_user_list.golines 114-115, when returning nil for 404, the body is already closed. This means line 57's defer is unreachable whenadminUseris nil, which is correct. However, the current code structure is slightly confusing.Consider moving the defer before the nil check for clarity, or noting that the 404 path already closes the body:
🔎 Suggested clarification
adminUser, resp, err := o.ext.UserGet(userID).Execute(ctx) if err != nil { return nil, nil, wrapSDKError(err, resp, "failed to get user") } // adminUser is nil when user is not found (404) - this is a system user, not an admin user. + // Note: The 404 path in UserGetRequest.Execute already closes the response body. if adminUser == nil { return nil, nil, nil } defer resp.Body.Close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/errors.go
- pkg/connector/pagination.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/connector.go (1)
pkg/connector/ext.go (1)
ExtensionClient(5-9)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (20)
pkg/connector/helper.go (1)
45-57: LGTM - Improved error handling with contextual wrapping.The updated error message provides better context for debugging, and the use of
wrapSDKErroraligns with the PR's goal of standardizing error handling across the connector.pkg/connector/ext.go (1)
3-9: LGTM - Clean migration to uhttp.BaseHttpClient.The type change from
*http.Clientto*uhttp.BaseHttpClientaligns with the PR's goal of standardizing HTTP client usage across the connector.pkg/connector/apps.go (4)
44-76: LGTM - Comprehensive error wrapping in List method.The error handling improvements provide clear context at each failure point: app resource creation, API calls, and pagination. The consistent use of
wrapSDKErrorfor API errors andfmt.Errorfwith%wfor local errors follows a good pattern.
79-94: LGTM - Error context added to appResource.The wrapped error in trait construction provides clear context for debugging failures during app resource creation.
142-195: LGTM - Improved error handling in adminGrants.Good error wrapping for both API calls (
wrapSDKError) and local processing errors (fmt.Errorf). The context about "admin grants processing" helps identify the specific operation that failed.
216-291: LGTM - Thorough error handling in Grants method.All error paths now include descriptive context. The inclusion of
resourceIDandresourceTypeIDin line 250's error message is particularly helpful for debugging unexpected pagination states.pkg/connector/connector.go (3)
56-66: LGTM - Proper initialization of BaseHttpClient for ExtensionClient.The
baseHttpClientis correctly created using the existinghttpClientand error handling is properly implemented. This aligns theExtensionClientwith the uhttp-based client pattern used elsewhere.
96-112: LGTM - Improved connector initialization with gRPC error codes.The use of
uhttp.WrapErrorswithcodes.InvalidArgumentprovides proper gRPC-compatible error responses for configuration failures. The error message and underlying error provide good context for debugging.
154-176: LGTM - Consistent validation error handling with gRPC codes.Good pattern:
wrapSDKErrorhandles the API call error case, whilehttpStatusToGRPCCode+uhttp.WrapErrorshandles unexpected status codes. This ensures all validation failures return properly coded gRPC errors.pkg/connector/groups.go (5)
29-60: LGTM - Consistent error handling in List method.Good use of
wrapSDKErrorfor API calls and descriptivefmt.Errorfwrapping for pagination errors.
63-94: LGTM - Proper error context in resource and trait construction.Both
groupResourceandgroupTraitnow provide clear error context that helps identify where in the pipeline a failure occurred.
120-155: LGTM - Error handling improvements in Grants method.Consistent error wrapping for pagination token handling and API calls.
157-208: Good handling of 409 Conflict for idempotent grant operations.The 409 handling correctly returns
GrantAlreadyExistsannotation instead of an error, making the grant operation idempotent. The response body is properly closed before the early return.
210-264: Good handling of 404 NotFound for idempotent revoke operations.The 404 handling correctly returns
GrantAlreadyRevokedannotation, addressing the past review comment. The response body is properly closed before the early return.pkg/connector/ext_user_list.go (2)
32-75: LGTM - Clean migration to uhttp-based request flow.The refactored
Executemethod properly uses uhttp's request building and response handling patterns. TheWithJSONResponseoption simplifies the response parsing.
84-135: Consider fixing the response body read-after-close pattern.The
defer resp.Body.Close()on line 124 schedules the close, butio.ReadAll(resp.Body)on line 125 reads the body before the function returns, so this works correctly. However, the pattern is slightly unusual—typically the defer comes before any body reads.More importantly, the 404 handling (lines 112-116) correctly returns
nilfor the user without an error, which is the expected behavior for system users as documented. This addresses the past review comment about using the uhttp client.pkg/connector/users.go (4)
88-164: LGTM - Consistent error wrapping in List method.Good use of
wrapSDKErrorfor API calls and descriptive error messages for pagination state issues. The error message on line 160 includes the unexpected state value which aids debugging.
166-201: LGTM - Error context added to adminUserResource.The wrapped error on line 197 provides clear context when resource creation fails.
203-216: LGTM - Error wrapping in userResource.Clear error context for trait retrieval failures.
225-256: LGTM - Improved error handling in fetchManager and userTrait.Both methods now wrap errors with descriptive context. The
wrapSDKErrorinfetchManagerincludes the response for proper HTTP-to-gRPC code mapping.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
cbd6e3b to
1da923a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/connector/users.go:
- Around line 48-57: The code can return early when adminUser == nil without
closing resp.Body; update the handling after calling
o.ext.UserGet(userID).Execute(ctx) so the response body is always closed: either
move the defer resp.Body.Close() to immediately after confirming resp != nil and
before any early returns, or explicitly call resp.Body.Close() (guarded by resp
!= nil) right before the early return when adminUser == nil; reference the
adminUser, resp, Execute and UserGet symbols to locate the change.
🧹 Nitpick comments (2)
pkg/connector/groups.go (1)
191-205: Consider consolidating error creation and return.The error is created, logged, and then wrapped again with
fmt.Errorf. Sinceuhttp.WrapErrorsalready produces a descriptive error, the additional wrapping may be redundant.🔎 Suggested simplification
if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusConflict { err := uhttp.WrapErrors( httpStatusToGRPCCode(resp.StatusCode), fmt.Sprintf("failed to add user to group: unexpected status code %d", resp.StatusCode), nil, ) l.Error( "jumpcloud-connector: failed to add user to group", zap.Error(err), zap.String("group", entitlement.Resource.Id.Resource), zap.String("user", principal.Id.Resource), zap.Int("status", resp.StatusCode), ) - return nil, fmt.Errorf("failed to add user to group after successful API call: %w", err) + return nil, err }pkg/connector/ext_user_list.go (1)
122-135: Consider usinguhttp.WithJSONResponsefor consistency with the List path.The Get path manually reads the body and unmarshals JSON, while the List path uses
uhttp.WithJSONResponse. For consistency and to leverage uhttp's built-in error handling, consider aligning the patterns.However, the current approach is intentional to handle the 404 case specially before JSON parsing, so this is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/apps.go
- pkg/connector/errors.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
pkg/connector/connector.go (1)
pkg/connector/ext.go (1)
ExtensionClient(5-9)
🔇 Additional comments (14)
pkg/connector/helper.go (1)
45-57: LGTM! Error handling improvements look good.The enhanced error message for empty email validation and the use of
wrapSDKErrorfor API call failures align well with the PR's objective of consistent gRPC error wrapping across the connector.pkg/connector/pagination.go (1)
10-38: LGTM! Contextual error wrapping improves debuggability.The wrapped errors with descriptive messages (unmarshal, parse, marshal operations) will help trace pagination issues during debugging.
pkg/connector/ext.go (1)
3-9: LGTM! Migration touhttp.BaseHttpClientenables standardized HTTP handling.The type change from
*http.Clientto*uhttp.BaseHttpClientproperly aligns with the SDK's HTTP abstraction and enables consistent error wrapping in downstream request handlers.pkg/connector/connector.go (3)
56-66: LGTM! Proper initialization ofBaseHttpClientfor the extension client.The creation of
baseHttpClientviauhttp.NewBaseHttpClientWithContextwith proper error handling enables the uhttp-based flow for extension API calls.
104-110: LGTM! Appropriate use of gRPC error codes for configuration validation.Using
codes.InvalidArgumentwithuhttp.WrapErrorscorrectly signals that the connector was improperly configured, which is a client-side error.
158-174: LGTM! Validation error handling is well-structured.The combination of
wrapSDKErrorfor API failures andhttpStatusToGRPCCodeconversion for unexpected status codes provides consistent error classification during credential validation.pkg/connector/groups.go (3)
41-59: LGTM! Consistent error wrapping for group listing operations.The use of
wrapSDKErrorfor API failures and contextualfmt.Errorffor pagination errors provides good traceability.
175-188: LGTM! Proper idempotent handling for existing group membership.Returning
GrantAlreadyExistsannotation on HTTP 409 correctly signals idempotent success. The explicitresp.Body.Close()in the early return path prevents resource leaks.
231-244: LGTM! Proper idempotent handling for already-revoked membership.Returning
GrantAlreadyRevokedannotation on HTTP 404 correctly signals idempotent success, addressing the previous review feedback.pkg/connector/users.go (3)
109-114: LGTM! Proper use ofwrapSDKErrorfor SDK API failures.The error wrapping for system users list correctly uses the SDK error wrapper.
128-132: Verify: Should this usewrapSDKErrorinstead offmt.Errorf?The admin users list call goes through the extension client which returns uhttp-wrapped errors. However, for consistency with other SDK-like calls in this file, consider whether
wrapSDKErrorwould be more appropriate here if the extension client returns an HTTP response.Based on past review comments, the extension client calls don't need SDK error wrapping since they use uhttp internally. The current
fmt.Errorfapproach is correct.
225-242: LGTM! Manager fetch error handling is well-structured.The combination of logging with context and
wrapSDKErrorprovides good observability for manager lookup failures.pkg/connector/ext_user_list.go (2)
32-75: LGTM! Clean migration to uhttp-based request/response flow.The request construction using
ulr.client.NewRequestwith request options anduhttp.WithJSONResponsefor automatic JSON binding is idiomatic for the SDK's HTTP abstraction.
105-120: Good documentation, but verify the caller handles the closed response body.The detailed comment explains why
HttpClient.Dois used directly to avoid 404 error logging. However, thedefer resp.Body.Close()on line 115 will execute before returning on line 119, meaning the caller receives a response with an already-closed body.This is fine if the caller only checks
adminUser == niland doesn't read fromresp.Body, which is the current behavior inusers.go. Just ensure this contract is maintained.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
1da923a to
8baf212
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @pkg/connector/connector.go:
- Around line 105-109: The call to uhttp.WrapErrors in connector initialization
is creating a redundant fmt.Errorf instead of passing no underlying error;
update the uhttp.WrapErrors invocation in connector.go (the return that
references uhttp.WrapErrors and the message "connector initialization failed:
API client not configured") to pass nil as the third argument rather than
fmt.Errorf(...) so the wrapper reflects that there is no underlying error to
wrap.
🧹 Nitpick comments (4)
pkg/connector/ext_user_list.go (2)
56-61: Consider initializing the slice with capacity or using a single declaration.Minor style improvement: you can declare and append in one line for cleaner code.
🔎 Proposed refactor
- var reqOpts []uhttp.RequestOption - reqOpts = append(reqOpts, uhttp.WithAcceptJSONHeader()) - reqOpts = append(reqOpts, uhttp.WithHeader("x-api-key", ulr.apiKey)) - if ulr.orgId != "" { - reqOpts = append(reqOpts, uhttp.WithHeader("x-org-id", ulr.orgId)) + reqOpts := []uhttp.RequestOption{ + uhttp.WithAcceptJSONHeader(), + uhttp.WithHeader("x-api-key", ulr.apiKey), + } + if ulr.orgId != "" { + reqOpts = append(reqOpts, uhttp.WithHeader("x-org-id", ulr.orgId)) }
87-91: EscapeuserIDin URL path to prevent path traversal.The
userIDis directly concatenated into the URL path. Although JumpCloud IDs are typically safe, usingurl.PathEscapeguards against edge cases and malicious input.🔎 Proposed fix
u := &url.URL{ Scheme: "https", Host: "console.jumpcloud.com", - Path: "/api/users/" + ugr.userID, + Path: "/api/users/" + url.PathEscape(ugr.userID), }pkg/connector/users.go (1)
128-132: Inconsistent error wrapping withlist-userscase.Line 113 uses
wrapSDKError(err, resp, ...)for the SDK call, but line 131 usesfmt.Errorf. Sinceo.ext.UserList().Execute(ctx)also returns a response, consider usingwrapSDKErrorfor consistency with the rest of the PR's error handling pattern.🔎 Proposed fix
case "list-admin-users": adminUsers, resp, err := o.ext.UserList().Skip(skip).Execute(ctx) if err != nil { - return nil, nil, fmt.Errorf("failed to list admin users: %w", err) + return nil, nil, wrapSDKError(err, resp, "failed to list admin users") }Note: If
wrapSDKErroris specifically for SDK client calls and not the extension client, this may be intentional. Please verify.pkg/connector/groups.go (1)
247-261: Avoid double-wrapping the error.The error is created with
uhttp.WrapErrorsand then wrapped again withfmt.Errorf. This creates nested wrapping that may obscure the original error. Either return theuhttp.WrapErrorserror directly or only usefmt.Errorf.🔎 Proposed fix
if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { - err := uhttp.WrapErrors( + l.Error( + "jumpcloud-connector: failed to remove user from group", + zap.String("group", entitlement.Resource.Id.Resource), + zap.String("user", principal.Id.Resource), + zap.Int("status", resp.StatusCode), + ) + return nil, uhttp.WrapErrors( httpStatusToGRPCCode(resp.StatusCode), fmt.Sprintf("failed to remove user from group: unexpected status code %d", resp.StatusCode), nil, ) - l.Error( - "jumpcloud-connector: failed to remove user from group", - zap.Error(err), - zap.String("group", entitlement.Resource.Id.Resource), - zap.String("user", principal.Id.Resource), - zap.Int("status", resp.StatusCode), - ) - return nil, fmt.Errorf("failed to remove user from group: %w", err) }The same pattern applies to the
Grantfunction at lines 192-204.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/errors.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/connector.go (1)
pkg/connector/ext.go (1)
ExtensionClient(5-9)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (26)
pkg/connector/helper.go (2)
47-47: LGTM!The error message now provides better context by specifying which parameter is empty and in what operation it's being used.
56-56: LGTM!The use of
wrapSDKErrorprovides consistent error handling for SDK operations and adds helpful context about the failed operation.pkg/connector/ext.go (1)
3-9: LGTM!The migration from
*http.Clientto*uhttp.BaseHttpClientenables standardized error handling and observability across HTTP operations. The field is unexported, so the change is contained within the package.pkg/connector/pagination.go (3)
14-14: LGTM!The error wrapping provides clear context about the pagination operation that failed while preserving the original error.
21-21: LGTM!Clear error message that explains the parsing failure during pagination token processing.
35-35: LGTM!Appropriate error context for the token marshaling operation.
pkg/connector/apps.go (12)
46-46: LGTM!The error wrapping provides helpful context about which resource failed to create.
60-60: LGTM!Appropriate use of
wrapSDKErrorfor SDK API calls.
67-67: LGTM!Clear error context about the construction phase that failed.
82-82: LGTM!Appropriate error wrapping for the trait construction operation.
152-152: LGTM!Clear error message for the admin user list operation.
166-166: LGTM!The error wrapping provides application-level context. Note that
fetchUserByEmailalready wraps SDK errors internally withwrapSDKError, so this adds an additional layer of context specific to the admin grants processing flow, which is appropriate.
219-219: LGTM!Appropriate context for pagination bag unmarshaling failure.
232-232: LGTM!Clear error message for pagination token parsing failure.
250-250: LGTM!Good error message that includes the unexpected state values for debugging.
255-255: LGTM!Appropriate use of
wrapSDKErrorfor SDK association list calls.
266-266: LGTM!Clear context for pagination advancement failure.
287-287: LGTM!Appropriate error wrapping for page token marshaling.
pkg/connector/connector.go (4)
56-63: LGTM!The initialization properly creates a
uhttp.BaseHttpClientto align with the updatedExtensionClienttype that now expects*uhttp.BaseHttpClientinstead of*http.Client. Error handling for the client creation is appropriate.
100-100: LGTM!The updated error message provides better context about when the failure occurred.
161-161: LGTM!Using
wrapSDKErrorprovides consistent error handling for SDK operations and adds helpful context about the validation failure.
166-173: LGTM!The introduction of HTTP status to gRPC code mapping provides better error classification for validation failures. The error wrapping includes the status code for debugging.
pkg/connector/ext_user_list.go (1)
105-118: LGTM on the 404 handling approach.The bypass of
uhttp.Do()for 404 responses is well-documented and justified. The trade-off between avoiding error log noise and manual JSON handling is reasonable for this use case.pkg/connector/users.go (1)
234-243: LGTM on fetchManager error handling.The error is logged with context and wrapped using
wrapSDKErrorconsistently with other SDK calls in the codebase.pkg/connector/groups.go (2)
29-44: LGTM on List error handling.The
wrapSDKErrorusage and skip token marshaling error wrapping provide good context for debugging.
232-244: LGTM on 404 handling for idempotent revoke.The handling of 404 as
GrantAlreadyRevokedis correct and follows best practices for idempotent revocation. Body is properly closed before returning.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
8baf212 to
e14f30c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @pkg/connector/apps.go:
- Around line 164-167: The error wrapping uses the wrong HTTP response (resp
from the earlier UserList call) when handling errors from fetchUserByEmail;
update the wrapSDKError call in the admin grants processing path to pass nil
instead of resp (i.e., wrapSDKError(err, nil, "...")) so the wrapped error
doesn't carry the unrelated UserList response — locate the call site around
fetchUserByEmail and change the second argument to nil (alternatively, return
the actual SystemusersList response from fetchUserByEmail if you prefer richer
context).
In @pkg/connector/ext_user_list.go:
- Around line 84-91: The Execute method on UserGetRequest builds the URL by
concatenating ugr.userID into the path, which allows path traversal or malformed
paths; before constructing the URL in UserGetRequest.Execute, validate and
safely encode the userID (e.g., call url.PathEscape on ugr.userID or otherwise
validate against disallowed characters) and use the escaped value when building
Path so the request cannot be manipulated via `../` or encoded separators.
- Around line 105-123: The call to the non-existent function
uhttp.WrapErrorsWithRateLimitInfo in ext_user_list.go will not compile; replace
that call with the existing uhttp.WrapErrors API and pass a descriptive message
plus the underlying error info (e.g., use
uhttp.WrapErrors(httpStatusToGRPCCode(resp.StatusCode), "unexpected response
from user endpoint", resp) or similar), updating the call site near the resp
StatusCode != http.StatusOK branch (referencing ugr.client.HttpClient.Do, resp,
httpStatusToGRPCCode, and uhttp.WrapErrors).
🧹 Nitpick comments (1)
pkg/connector/groups.go (1)
247-261: Consider removing redundanthttp.StatusNotFoundcheck for consistency.Similar to the
Grantmethod, if the SDK returns an error for status >= 300, the 404 case should be handled in theerr != nilblock above (lines 232-244), making theStatusNotFoundcheck here unreachable.Suggested fix
- if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { + if resp.StatusCode != http.StatusNoContent {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/connector/pagination.go
- pkg/connector/errors.go
- pkg/connector/helper.go
- pkg/connector/users.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (10)
pkg/connector/connector.go (2)
56-66: LGTM! Proper migration touhttp.BaseHttpClient.The
ExtensionClientnow correctly usesuhttp.BaseHttpClientcreated viauhttp.NewBaseHttpClientWithContext, with proper error handling for the client creation.
154-174: LGTM! Consistent error handling in Validate.The validation logic properly uses
wrapSDKErrorfor SDK errors anduhttp.WrapErrorswithhttpStatusToGRPCCodefor unexpected status codes. The logging context is clear with the "jumpcloud-connector" prefix.pkg/connector/ext.go (1)
3-9: LGTM! Clean migration touhttp.BaseHttpClient.The
ExtensionClientstruct now correctly uses*uhttp.BaseHttpClientinstead of*http.Client, aligning with the centralized HTTP client pattern introduced in this PR.pkg/connector/apps.go (2)
46-46: LGTM! Consistent error wrapping in List and resource construction.Error messages are descriptive and provide appropriate context for debugging failures during app resource creation and listing.
Also applies to: 60-60, 67-67, 82-82
219-219: LGTM! Comprehensive error wrapping in Grants pagination.All error paths in the
Grantsmethod now include contextual messages that identify the specific operation that failed (unmarshal, parsing, pagination state, association listing, advancement, marshaling).Also applies to: 232-232, 250-250, 255-255, 266-266, 287-287
pkg/connector/groups.go (3)
43-43: LGTM! Consistent error wrapping throughout List, Grants, and resource construction.All error paths now include contextual messages that clearly identify the failing operation.
Also applies to: 58-58, 66-66, 86-86, 129-129, 134-134, 152-152
176-188: LGTM! Proper 409 Conflict handling withGrantAlreadyExistsannotation.The 409 handling correctly closes the response body, returns the
GrantAlreadyExistsannotation, and logs the error with appropriate context before wrapping withwrapSDKError.
232-244: LGTM! Proper 404 Not Found handling withGrantAlreadyRevokedannotation.The 404 handling correctly mirrors the 409 pattern in
Grant, properly closing the response body and returning theGrantAlreadyRevokedannotation.pkg/connector/ext_user_list.go (2)
56-74: LGTM! Clean migration touhttprequest flow.The
UserListRequest.Executenow properly usesuhttp.BaseHttpClient.NewRequestandDowithWithJSONResponse, following the SDK patterns. Request options for headers are correctly applied.
124-134: LGTM! Manual JSON unmarshaling is acceptable given the 404 trade-off.The manual
io.ReadAllandjson.Unmarshalflow is a reasonable trade-off to maintain the 404 non-error behavior while still properly parsing successful responses.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
e14f30c to
d4941a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/groups.go (1)
247-261: Consider removing the redundanthttp.StatusNotFoundcheck.Similar to the Grant method, the condition at line 247 checks for both
http.StatusNoContentandhttp.StatusNotFound, but a 404 response should already be handled in theerr != nilblock (lines 232-241). If the SDK returns an error for 404 status codes, this check is redundant.🔎 Suggested fix
- if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { + if resp.StatusCode != http.StatusNoContent {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/pagination.go
- pkg/connector/errors.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/connector.go (1)
pkg/connector/ext.go (1)
ExtensionClient(5-9)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (16)
pkg/connector/helper.go (1)
45-57: LGTM! Error handling improvements are well-implemented.The updated error message at line 47 provides better context, and the use of
wrapSDKErrorat line 56 correctly wraps the SDK error with response context, aligning with the PR's centralized error handling strategy.pkg/connector/ext.go (1)
3-9: LGTM! Client type migration to uhttp.BaseHttpClient is correct.The change from
*http.Clientto*uhttp.BaseHttpClientaligns with the broader migration to a unified uhttp-based HTTP flow across the connector package.pkg/connector/apps.go (4)
164-167: Fix applied correctly - usingnilfor response context.The
fetchUserByEmailerror is now correctly wrapped withnilas the second argument since the response fromfetchUserByEmailis not available at this call site. This addresses the previously identified issue.
46-47: LGTM! Descriptive error context added.The error wrapping provides clear context about where the failure occurred during JumpCloud Administration app resource creation.
58-61: LGTM! SDK error properly wrapped.The
wrapSDKErrorfunction correctly wraps the SDK error with response context for the applications listing operation.
249-251: Consider adding resource context to the unexpected pagination state error.The error message includes resourceID and resourceTypeID which is helpful for debugging. This is a good practice for troubleshooting pagination issues.
pkg/connector/connector.go (2)
56-66: LGTM! BaseHttpClient initialization is correct.The
uhttp.NewBaseHttpClientWithContextis properly used to create the base HTTP client, and error handling is appropriate. TheExtensionClientis correctly initialized with the new client type.
159-173: LGTM! Validate error handling with gRPC codes is well-implemented.The error handling correctly uses
wrapSDKErrorfor SDK errors andhttpStatusToGRPCCodewithuhttp.WrapErrorsfor unexpected status codes. The logging provides useful context for debugging.pkg/connector/users.go (3)
111-114: LGTM! SDK error properly wrapped for user listing.The
wrapSDKErrorfunction correctly wraps the SDK error with response context.
128-132: Consider usingwrapSDKErrorfor consistency with other SDK-adjacent errors.While this isn't a direct SDK call, the error originates from an extension client that wraps HTTP calls. For consistency with the PR's error handling strategy, consider whether
wrapSDKErrormight be more appropriate here if the response contains useful context.However, since
respis returned and might contain rate limit or status information, this could be intentional. The current approach is acceptable.
234-242: LGTM! Manager fetch error handling is appropriate.The error is logged with context and wrapped using
wrapSDKErrorfor consistent error propagation.pkg/connector/ext_user_list.go (2)
87-91: Path traversal vulnerability addressed.The use of
url.PathEscape(ugr.userID)correctly mitigates the path traversal risk by safely encoding the userID before concatenating it into the URL path.
32-74: LGTM! UserListRequest.Execute correctly uses uhttp flow.The implementation properly uses
uhttp.NewRequestwith appropriate request options anduhttp.WithJSONResponsefor response handling. The error propagation includes the response object for context.pkg/connector/groups.go (3)
41-44: LGTM! SDK error properly wrapped for group listing.The
wrapSDKErrorfunction correctly wraps the SDK error with response context.
176-188: LGTM! 409 Conflict handling with GrantAlreadyExists annotation.The handling correctly checks for 409 status within the error path, closes the response body, and returns the
GrantAlreadyExistsannotation. This aligns with the expected behavior when a user is already a member of the group.
232-244: LGTM! 404 handling with GrantAlreadyRevoked annotation.The handling correctly checks for 404 status within the error path, closes the response body, and returns the
GrantAlreadyRevokedannotation. This aligns with the expected behavior when a user is already not a member of the group.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
d4941a4 to
d2aaa89
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/groups.go (1)
247-261: Remove redundanthttp.StatusNotFoundcheck.Similar to the Grant method, the 404 case is already handled in the
err != nilblock (lines 234-240). The SDK returns an error for status >= 300, so this condition is unreachable whenerr == nil.♻️ Suggested simplification
- if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusNotFound { + if resp.StatusCode != http.StatusNoContent {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/connector/errors.go
- pkg/connector/apps.go
- pkg/connector/helper.go
- pkg/connector/users.go
- pkg/connector/pagination.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/connector.go (1)
pkg/connector/ext.go (1)
ExtensionClient(5-9)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (9)
pkg/connector/ext.go (1)
3-9: LGTM!The migration from
*http.Clientto*uhttp.BaseHttpClientaligns with the PR's goal of centralizing HTTP handling through the SDK's uhttp package. The struct field change is consistent with usage inext_user_list.go.pkg/connector/connector.go (2)
56-66: LGTM!Good addition of error handling for the
uhttp.NewBaseHttpClientWithContextcall. ThebaseHttpClientis properly propagated toExtensionClient, aligning with the type changes inext.go.
158-174: LGTM!The error handling in
Validateis well-structured:
- Uses
wrapSDKErrorfor SDK-level errors with response context- Uses
httpStatusToGRPCCode+uhttp.WrapErrorsfor unexpected status codes- Consistent logging with the wrapped error
pkg/connector/ext_user_list.go (2)
54-72: LGTM!Clean migration to the uhttp request pattern. The use of
uhttp.RequestOptionfor headers anduhttp.WithJSONResponsefor automatic JSON unmarshalling simplifies the code while maintaining proper error propagation with the response object.
82-109: LGTM!Good implementation addressing previous security concerns:
url.PathEscape(ugr.userID)prevents path traversal attacks- Consistent request/response pattern with
UserListRequest- Comments updated to reflect the use of
uhttp.Do()withWithJSONResponsepkg/connector/groups.go (4)
41-44: LGTM!Good use of
wrapSDKErrorto provide context for SDK errors while preserving the response information for status code mapping.
176-188: LGTM!Good handling of the 409 Conflict case with the
GrantAlreadyExistsannotation. Properly closes the response body and provides clear logging.
232-244: LGTM!Good handling of the 404 Not Found case with the
GrantAlreadyRevokedannotation. Consistent pattern with the Grant method's conflict handling.
56-59: LGTM!Descriptive error messages with proper context (e.g., "during group list pagination") make debugging easier.
mateoHernandez123
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
d2aaa89 to
ebe6d72
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/groups.go (2)
174-212: Potential resource leak: response body not closed on non-409 error path.When
err != niland the status code is not 409 (Conflict), the function returns at Line 191 without closing the response body. Thedefer resp.Body.Close()at Lines 193-195 is only registered after the error handling block, so it won't execute on the error return path.🐛 Proposed fix
if err != nil { // If the user is already a member of the group, we get a 409 and we want to return success // with the GrantAlreadyExists annotation. if resp != nil && resp.StatusCode == http.StatusConflict { if resp.Body != nil { resp.Body.Close() } var annos annotations.Annotations annos.Update(&v2.GrantAlreadyExists{}) return annos, nil } + if resp != nil && resp.Body != nil { + resp.Body.Close() + } l.Error("jumpcloud-connector: failed to add user to group", zap.Error(err), zap.String("group", entitlement.Resource.Id.Resource), zap.String("user", principal.Id.Resource)) return nil, wrapSDKError(err, resp, "failed to add user to group") }Alternatively, restructure to defer close at the start:
+ if resp != nil { + defer resp.Body.Close() + } if err != nil { // If the user is already a member of the group, we get a 409 and we want to return success // with the GrantAlreadyExists annotation. if resp != nil && resp.StatusCode == http.StatusConflict { - if resp.Body != nil { - resp.Body.Close() - } var annos annotations.Annotations annos.Update(&v2.GrantAlreadyExists{}) return annos, nil } l.Error("jumpcloud-connector: failed to add user to group", zap.Error(err), zap.String("group", entitlement.Resource.Id.Resource), zap.String("user", principal.Id.Resource)) return nil, wrapSDKError(err, resp, "failed to add user to group") } - if resp != nil { - defer resp.Body.Close() - }
232-254: Same resource leak pattern in Revoke.Similar to
Grant, whenerr != niland the status code is not 404, the response body is not closed before returning at Line 249. The defer at Lines 251-253 is registered after the error handling block.🐛 Proposed fix
if err != nil { // If the user is already not a member of the group, we get a 404 and we want to return success // with the GrantAlreadyRevoked annotation. if resp != nil && resp.StatusCode == http.StatusNotFound { if resp.Body != nil { resp.Body.Close() } var annos annotations.Annotations annos.Update(&v2.GrantAlreadyRevoked{}) return annos, nil } + if resp != nil && resp.Body != nil { + resp.Body.Close() + } l.Error("jumpcloud-connector: failed to remove user from group", zap.Error(err), zap.String("group", entitlement.Resource.Id.Resource), zap.String("user", principal.Id.Resource)) return nil, wrapSDKError(err, resp, "failed to remove user from group") }
🧹 Nitpick comments (2)
pkg/connector/users.go (2)
48-55: Fragile error type detection using string matching.The check
strings.Contains(err.Error(), codes.NotFound.String())is brittle - it relies on error message formatting that could change. Consider using proper gRPC status extraction if the SDK returns gRPC errors, or create a helper function that encapsulates this check consistently.♻️ Suggested improvement
If the SDK returns gRPC status errors, use proper status extraction:
import "google.golang.org/grpc/status" // Check if error is NotFound if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound { // handle not found }Alternatively, if this pattern is used elsewhere, centralize it in a helper function for consistency.
129-135: Inconsistent error wrapping pattern.Line 134 uses
fmt.Errorfwhile Line 114 useswrapSDKErrorfor a similar operation. For consistency with the rest of the PR, consider usingwrapSDKError(err, resp, "failed to list admin users")here as well, which would provide HTTP status to gRPC code mapping.♻️ Proposed fix
if err != nil { - return nil, nil, fmt.Errorf("failed to list admin users: %w", err) + return nil, nil, wrapSDKError(err, resp, "failed to list admin users") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/connector/apps.gopkg/connector/connector.gopkg/connector/errors.gopkg/connector/ext.gopkg/connector/ext_user_list.gopkg/connector/groups.gopkg/connector/helper.gopkg/connector/pagination.gopkg/connector/users.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/helper.go
- pkg/connector/errors.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/groups.go (1)
pkg/connector/connector.go (1)
New(96-113)
🔇 Additional comments (18)
pkg/connector/ext.go (1)
3-9: LGTM!The migration from
*http.Clientto*uhttp.BaseHttpClientaligns with the broader uhttp adoption across the connector. The type change enables consistent HTTP client usage and better error handling integration.pkg/connector/pagination.go (2)
10-26: LGTM!The contextual error wrapping in
unmarshalSkipTokenimproves debuggability by clearly identifying where pagination failures occur. The error messages are descriptive and properly chain the underlying errors.
28-38: LGTM!Good addition of contextual error wrapping in
marshalSkipToken. Note that Line 30 returnsnextToken(b, "")directly without wrapping - this is intentional as the error will bubble up with its own context.pkg/connector/connector.go (3)
56-66: LGTM!Good addition of
baseHttpClientinitialization with proper error propagation. TheExtensionClientnow correctly uses the uhttp-based client for consistent HTTP handling.
104-110: LGTM!Good use of
uhttp.WrapErrorswithcodes.InvalidArgumentfor configuration validation. This provides proper gRPC error semantics for initialization failures.
158-176: LGTM!The response body handling is correct - checking for nil before deferring close. The error handling properly uses
wrapSDKErrorfor API errors anduhttp.WrapErrorswith HTTP-to-gRPC code mapping for unexpected status codes.pkg/connector/apps.go (4)
58-64: LGTM!Correct response body handling pattern - nil check before defer ensures no panic on nil response. Error wrapping with
wrapSDKErrorprovides consistent gRPC error semantics.
144-158: LGTM!The
adminGrantsfunction correctly handles response body cleanup and provides descriptive error context. The flow properly manages pagination state.
220-255: LGTM!Good improvements to the
Grantsfunction:
- Contextual error wrapping for pagination unmarshal and skip parsing
- Descriptive error message for unexpected pagination state includes resource identifiers for debugging
- Proper response body handling
257-294: LGTM!Response body handling and error wrapping are consistent. The page advancement and token marshaling errors are properly wrapped with context.
pkg/connector/users.go (3)
109-115: LGTM!Proper response body handling with nil check before defer, and consistent use of
wrapSDKErrorfor the list users error.
236-247: LGTM!The
fetchManagerfunction properly handles response body cleanup and useswrapSDKErrorfor consistent error wrapping with the manager ID context already in the log message.
253-259: LGTM!Good contextual error wrapping for the user profile struct construction failure in
userTrait.pkg/connector/ext_user_list.go (2)
30-73: LGTM!The
UserListRequest.Executemigration to uhttp is well-implemented:
- Proper URL construction with query parameters
- Headers set via request options (x-api-key, x-org-id, Accept)
- Uses
WithJSONResponsefor automatic unmarshaling- Returns response on error path, allowing callers to inspect status and close body
82-110: LGTM!The
UserGetRequest.Executeimplementation correctly:
- Uses
url.PathEscapeto safely encode the userID in the path- Sets appropriate headers via request options
- Returns response on error for caller handling
pkg/connector/groups.go (3)
41-47: LGTM!Proper response body handling with nil check before defer, and consistent use of
wrapSDKErrorfor the list groups error.
65-69: LGTM!Good contextual error wrapping for group trait construction failures.
129-140: LGTM!The
Grantsfunction properly handles response body cleanup and useswrapSDKErrorfor consistent error wrapping.
luisina-santos
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
ebe6d72 to
1e8ac1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @pkg/client/client.go:
- Around line 205-220: The AddGroupMember and RemoveGroupMember functions defer
resp.Body.Close() but then return resp to the caller, which closes the body
prematurely; remove the deferred resp.Body.Close() calls from both
AddGroupMember and RemoveGroupMember so the caller is responsible for closing
the response body, and ensure that on error paths you explicitly close resp.Body
(if resp != nil) before calling wrapSDKError to avoid leaks.
- Around line 127-141: ListSystemUsers (and the other list methods ListGroups,
ListGroupMembers, ListApplications, ListApplicationAssociations) currently defer
resp.Body.Close() but also return resp to callers, which prevents callers from
reading the response body. Fix by either removing the defer resp.Body.Close()
and leaving responsibility to the caller to close the body when you return resp,
or alternatively read and buffer the body (or convert to a suitable return
value) inside the method and close resp.Body before returning the buffered data
while not returning resp; apply the same change consistently in the methods
referenced so you don't close the body before returning the response object.
- Around line 109-125: In GetUserByID, defer resp.Body.Close() closes the HTTP
response before the caller can use it; remove the defer so the returned
*http.Response (resp) is left open for the caller to read and close, or
alternatively stop returning resp and fully consume/close resp.Body inside
GetUserByID; choose the former: delete the defer resp.Body.Close() line in
GetUserByID so callers are responsible for closing resp.Body.
🧹 Nitpick comments (7)
pkg/client/extension/extension.go (3)
30-40: Stale comment references PUT method but code performs GET.The curl example in the comment describes a PUT request, but the actual implementation performs a GET request. This is misleading and should be corrected or removed.
🧹 Suggested cleanup
func (ulr *UserListRequest) Execute(ctx context.Context) ([]jcapi1.Userreturn, *http.Response, error) { - // curl --request PUT \ - // --url https://console.jumpcloud.com/api/users/{id} \ - // --header 'content-type: application/json' \ - // --header 'x-api-key: REPLACE_KEY_VALUE' \ - // --header 'x-org-id: ' \ - // --data '{"email":"user@example.com","enableMultiFactor":true,"firstname":"string","growthData":{},"lastWhatsNewChecked":"2019-08-24","lastname":"string","roleName":"string"}' - // // JumpCloud doesn't export the List endpoint in their // OpenAPI spec, but they do export the PUT... - // .... so.. here we go! + // so we implement the List endpoint manually here.
47-52: Hardcoded base URL reduces testability and flexibility.The URL
console.jumpcloud.comis hardcoded, which makes it difficult to test against mock servers or different environments. Consider making the base URL configurable via the request struct or a package-level variable.
20-23:TotalCountis parsed but never used.The
listUserResponsestruct capturesTotalCountfrom the API response, but it's not returned or used anywhere. This could be valuable for pagination logic (e.g., determining if more pages exist).Also applies to: 66-72
pkg/client/client.go (3)
23-24: Extract hardcoded version string to a constant.The user-agent string
"baton-jumpcloud/0.1.0"is duplicated in multiple places. Consider extracting it to a package-level constant for easier maintenance.♻️ Suggested refactor
+const userAgent = "baton-jumpcloud/0.1.0" + func NewClient(ctx context.Context, apiKey string, orgId string) (*Client, error) { - httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent("baton-jumpcloud/0.1.0")) + httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent(userAgent)) if err != nil { return nil, err } cc1 := jcapi1.NewConfiguration() cc1.HTTPClient = httpClient - cc1.UserAgent = "baton-jumpcloud/0.1.0" + cc1.UserAgent = userAgent cc2 := jcapi2.NewConfiguration() cc2.HTTPClient = httpClient - cc2.UserAgent = "baton-jumpcloud/0.1.0" + cc2.UserAgent = userAgentAlso applies to: 29-31, 33-35
84-107: Inconsistent error handling compared to other methods.
ListDirectoriesdoesn't usewrapSDKErrorand handles errors differently from other methods. Iferrisnilbut the status code indicates failure, the wrapped error has no underlying cause (nilpassed toWrapErrors). Also,wrapSDKErroralready handles non-2xx responses when an error is returned by the SDK.Consider aligning with other methods for consistency:
♻️ Suggested refactor
func (jc *Client) ListDirectories(ctx context.Context, opts *Options) ([]jcapi2.Directory, error) { ctx, client := jc.client2(ctx) limit := opts.getLimit() page := opts.getPage() directories, resp, err := client.DirectoriesApi.DirectoriesList(ctx).Limit(limit).Skip(page).Execute() if err != nil { - return nil, err + return nil, wrapSDKError(err, resp, "failed to list directories") } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - code := httpStatusToGRPCCode(resp.StatusCode) - err := uhttp.WrapErrors( - code, - fmt.Sprintf("list directories: unexpected status code %d", resp.StatusCode), - nil, - ) - return nil, err - } - return directories, nil }
143-161: Redundant pagination assignment.
Pageis set topagein the struct initializer (line 149) and then overwritten bySkip(page)(line 152). Remove the redundant assignment.♻️ Suggested fix
func (jc *Client) ListAdminUsers(ctx context.Context, opts *Options) ([]jcapi1.Userreturn, *http.Response, string, error) { page := opts.getPage() userListRequest := &extension.UserListRequest{ Client: jc.extensionClient, ApiKey: jc.apiKey, OrgId: jc.orgId, - Page: page, } users, resp, err := userListRequest.Skip(page).Execute(ctx)pkg/client/helpers.go (1)
83-90:urlErr.Temporary()is deprecated since Go 1.18 and should be removed.The
Temporary()method onurl.Erroris deprecated and unreliable. The fallback at lines 103-107 already handles this case by returningcodes.Unavailable. Note: theurlErr.Timeout()check at line 75 has the same deprecation issue. Consider removing both deprecated method calls or, if specific error messages are important, useerrors.Is()with concrete error types instead.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/baton-jumpcloud/main.gois excluded by none and included by none
📒 Files selected for processing (296)
pkg/client/client.gopkg/client/extension/extension.gopkg/client/helpers.gopkg/client/jcapi1/.gitignorepkg/client/jcapi1/.openapi-generator-ignorepkg/client/jcapi1/.openapi-generator/FILESpkg/client/jcapi1/.openapi-generator/VERSIONpkg/client/jcapi1/.travis.ymlpkg/client/jcapi1/README.mdpkg/client/jcapi1/api/openapi.yamlpkg/client/jcapi1/api_application_templates.gopkg/client/jcapi1/api_applications.gopkg/client/jcapi1/api_command_results.gopkg/client/jcapi1/api_command_triggers.gopkg/client/jcapi1/api_commands.gopkg/client/jcapi1/api_managed_service_provider.gopkg/client/jcapi1/api_organizations.gopkg/client/jcapi1/api_radius_servers.gopkg/client/jcapi1/api_search.gopkg/client/jcapi1/api_systems.gopkg/client/jcapi1/api_systemusers.gopkg/client/jcapi1/api_users.gopkg/client/jcapi1/client.gopkg/client/jcapi1/configuration.gopkg/client/jcapi1/docs/Application.mdpkg/client/jcapi1/docs/ApplicationConfig.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrl.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrlTooltip.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrlTooltipVariables.mdpkg/client/jcapi1/docs/ApplicationConfigConstantAttributes.mdpkg/client/jcapi1/docs/ApplicationConfigConstantAttributesValueInner.mdpkg/client/jcapi1/docs/ApplicationConfigDatabaseAttributes.mdpkg/client/jcapi1/docs/ApplicationLogo.mdpkg/client/jcapi1/docs/ApplicationTemplatesApi.mdpkg/client/jcapi1/docs/ApplicationsApi.mdpkg/client/jcapi1/docs/Applicationslist.mdpkg/client/jcapi1/docs/Applicationtemplate.mdpkg/client/jcapi1/docs/ApplicationtemplateJit.mdpkg/client/jcapi1/docs/ApplicationtemplateLogo.mdpkg/client/jcapi1/docs/ApplicationtemplateOidc.mdpkg/client/jcapi1/docs/ApplicationtemplateProvision.mdpkg/client/jcapi1/docs/Applicationtemplateslist.mdpkg/client/jcapi1/docs/Command.mdpkg/client/jcapi1/docs/CommandResultsApi.mdpkg/client/jcapi1/docs/CommandTriggersApi.mdpkg/client/jcapi1/docs/Commandfilereturn.mdpkg/client/jcapi1/docs/CommandfilereturnResultsInner.mdpkg/client/jcapi1/docs/Commandresult.mdpkg/client/jcapi1/docs/CommandresultResponse.mdpkg/client/jcapi1/docs/CommandresultResponseData.mdpkg/client/jcapi1/docs/Commandresultslist.mdpkg/client/jcapi1/docs/CommandresultslistResultsInner.mdpkg/client/jcapi1/docs/CommandsApi.mdpkg/client/jcapi1/docs/Commandslist.mdpkg/client/jcapi1/docs/CommandslistResultsInner.mdpkg/client/jcapi1/docs/Error.mdpkg/client/jcapi1/docs/ErrorDetails.mdpkg/client/jcapi1/docs/ErrorDetailsAllOf.mdpkg/client/jcapi1/docs/Fde.mdpkg/client/jcapi1/docs/ManagedServiceProviderApi.mdpkg/client/jcapi1/docs/Mfa.mdpkg/client/jcapi1/docs/MfaEnrollment.mdpkg/client/jcapi1/docs/MfaEnrollmentStatus.mdpkg/client/jcapi1/docs/Organization.mdpkg/client/jcapi1/docs/OrganizationPutRequest.mdpkg/client/jcapi1/docs/Organizationentitlement.mdpkg/client/jcapi1/docs/OrganizationentitlementEntitlementProductsInner.mdpkg/client/jcapi1/docs/OrganizationsApi.mdpkg/client/jcapi1/docs/Organizationsettings.mdpkg/client/jcapi1/docs/OrganizationsettingsFeatures.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsights.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsightsPremium.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesSystemInsights.mdpkg/client/jcapi1/docs/OrganizationsettingsNewSystemUserStateDefaults.mdpkg/client/jcapi1/docs/OrganizationsettingsPasswordPolicy.mdpkg/client/jcapi1/docs/OrganizationsettingsUserPortal.mdpkg/client/jcapi1/docs/OrganizationsettingsWindowsMDM.mdpkg/client/jcapi1/docs/Organizationsettingsput.mdpkg/client/jcapi1/docs/OrganizationsettingsputNewSystemUserStateDefaults.mdpkg/client/jcapi1/docs/OrganizationsettingsputPasswordPolicy.mdpkg/client/jcapi1/docs/Organizationslist.mdpkg/client/jcapi1/docs/OrganizationslistResultsInner.mdpkg/client/jcapi1/docs/RadiusServersApi.mdpkg/client/jcapi1/docs/RadiusServersPutRequest.mdpkg/client/jcapi1/docs/Radiusserver.mdpkg/client/jcapi1/docs/Radiusserverpost.mdpkg/client/jcapi1/docs/Radiusserverput.mdpkg/client/jcapi1/docs/Radiusserverslist.mdpkg/client/jcapi1/docs/Search.mdpkg/client/jcapi1/docs/SearchApi.mdpkg/client/jcapi1/docs/Sshkeylist.mdpkg/client/jcapi1/docs/Sshkeypost.mdpkg/client/jcapi1/docs/Sso.mdpkg/client/jcapi1/docs/System.mdpkg/client/jcapi1/docs/SystemBuiltInCommandsInner.mdpkg/client/jcapi1/docs/SystemDomainInfo.mdpkg/client/jcapi1/docs/SystemMdm.mdpkg/client/jcapi1/docs/SystemMdmInternal.mdpkg/client/jcapi1/docs/SystemNetworkInterfacesInner.mdpkg/client/jcapi1/docs/SystemOsVersionDetail.mdpkg/client/jcapi1/docs/SystemProvisionMetadata.mdpkg/client/jcapi1/docs/SystemProvisionMetadataProvisioner.mdpkg/client/jcapi1/docs/SystemServiceAccountState.mdpkg/client/jcapi1/docs/SystemSshdParamsInner.mdpkg/client/jcapi1/docs/SystemSystemInsights.mdpkg/client/jcapi1/docs/SystemUserMetricsInner.mdpkg/client/jcapi1/docs/Systemput.mdpkg/client/jcapi1/docs/SystemputAgentBoundMessagesInner.mdpkg/client/jcapi1/docs/SystemsApi.mdpkg/client/jcapi1/docs/Systemslist.mdpkg/client/jcapi1/docs/Systemuserput.mdpkg/client/jcapi1/docs/SystemuserputAddressesInner.mdpkg/client/jcapi1/docs/SystemuserputAttributesInner.mdpkg/client/jcapi1/docs/SystemuserputPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserputRelationshipsInner.mdpkg/client/jcapi1/docs/Systemuserputpost.mdpkg/client/jcapi1/docs/SystemuserputpostAddressesInner.mdpkg/client/jcapi1/docs/SystemuserputpostPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserputpostRecoveryEmail.mdpkg/client/jcapi1/docs/Systemuserreturn.mdpkg/client/jcapi1/docs/SystemuserreturnAddressesInner.mdpkg/client/jcapi1/docs/SystemuserreturnPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserreturnRecoveryEmail.mdpkg/client/jcapi1/docs/SystemusersApi.mdpkg/client/jcapi1/docs/SystemusersResetmfaRequest.mdpkg/client/jcapi1/docs/SystemusersStateActivateRequest.mdpkg/client/jcapi1/docs/Systemuserslist.mdpkg/client/jcapi1/docs/Triggerreturn.mdpkg/client/jcapi1/docs/TrustedappConfigGet.mdpkg/client/jcapi1/docs/TrustedappConfigGetTrustedAppsInner.mdpkg/client/jcapi1/docs/TrustedappConfigPut.mdpkg/client/jcapi1/docs/Userput.mdpkg/client/jcapi1/docs/Userreturn.mdpkg/client/jcapi1/docs/UserreturnGrowthData.mdpkg/client/jcapi1/docs/UsersApi.mdpkg/client/jcapi1/git_push.shpkg/client/jcapi1/model_application.gopkg/client/jcapi1/model_application_config.gopkg/client/jcapi1/model_application_config_acs_url.gopkg/client/jcapi1/model_application_config_acs_url_tooltip.gopkg/client/jcapi1/model_application_config_acs_url_tooltip_variables.gopkg/client/jcapi1/model_application_config_constant_attributes.gopkg/client/jcapi1/model_application_config_constant_attributes_value_inner.gopkg/client/jcapi1/model_application_config_database_attributes.gopkg/client/jcapi1/model_application_logo.gopkg/client/jcapi1/model_applicationslist.gopkg/client/jcapi1/model_applicationtemplate.gopkg/client/jcapi1/model_applicationtemplate_jit.gopkg/client/jcapi1/model_applicationtemplate_logo.gopkg/client/jcapi1/model_applicationtemplate_oidc.gopkg/client/jcapi1/model_applicationtemplate_provision.gopkg/client/jcapi1/model_applicationtemplateslist.gopkg/client/jcapi1/model_command.gopkg/client/jcapi1/model_commandfilereturn.gopkg/client/jcapi1/model_commandfilereturn_results_inner.gopkg/client/jcapi1/model_commandresult.gopkg/client/jcapi1/model_commandresult_response.gopkg/client/jcapi1/model_commandresult_response_data.gopkg/client/jcapi1/model_commandresultslist.gopkg/client/jcapi1/model_commandresultslist_results_inner.gopkg/client/jcapi1/model_commandslist.gopkg/client/jcapi1/model_commandslist_results_inner.gopkg/client/jcapi1/model_error.gopkg/client/jcapi1/model_error_details.gopkg/client/jcapi1/model_error_details_all_of.gopkg/client/jcapi1/model_fde.gopkg/client/jcapi1/model_mfa.gopkg/client/jcapi1/model_mfa_enrollment.gopkg/client/jcapi1/model_mfa_enrollment_status.gopkg/client/jcapi1/model_organization.gopkg/client/jcapi1/model_organization_put_request.gopkg/client/jcapi1/model_organizationentitlement.gopkg/client/jcapi1/model_organizationentitlement_entitlement_products_inner.gopkg/client/jcapi1/model_organizationsettings.gopkg/client/jcapi1/model_organizationsettings_features.gopkg/client/jcapi1/model_organizationsettings_features_directory_insights.gopkg/client/jcapi1/model_organizationsettings_features_directory_insights_premium.gopkg/client/jcapi1/model_organizationsettings_features_system_insights.gopkg/client/jcapi1/model_organizationsettings_new_system_user_state_defaults.gopkg/client/jcapi1/model_organizationsettings_password_policy.gopkg/client/jcapi1/model_organizationsettings_user_portal.gopkg/client/jcapi1/model_organizationsettings_windows_mdm.gopkg/client/jcapi1/model_organizationsettingsput.gopkg/client/jcapi1/model_organizationsettingsput_new_system_user_state_defaults.gopkg/client/jcapi1/model_organizationsettingsput_password_policy.gopkg/client/jcapi1/model_organizationslist.gopkg/client/jcapi1/model_organizationslist_results_inner.gopkg/client/jcapi1/model_radius_servers_put_request.gopkg/client/jcapi1/model_radiusserver.gopkg/client/jcapi1/model_radiusserverpost.gopkg/client/jcapi1/model_radiusserverput.gopkg/client/jcapi1/model_radiusserverslist.gopkg/client/jcapi1/model_search.gopkg/client/jcapi1/model_sshkeylist.gopkg/client/jcapi1/model_sshkeypost.gopkg/client/jcapi1/model_sso.gopkg/client/jcapi1/model_system.gopkg/client/jcapi1/model_system_built_in_commands_inner.gopkg/client/jcapi1/model_system_domain_info.gopkg/client/jcapi1/model_system_mdm.gopkg/client/jcapi1/model_system_mdm_internal.gopkg/client/jcapi1/model_system_network_interfaces_inner.gopkg/client/jcapi1/model_system_os_version_detail.gopkg/client/jcapi1/model_system_provision_metadata.gopkg/client/jcapi1/model_system_provision_metadata_provisioner.gopkg/client/jcapi1/model_system_service_account_state.gopkg/client/jcapi1/model_system_sshd_params_inner.gopkg/client/jcapi1/model_system_system_insights.gopkg/client/jcapi1/model_system_user_metrics_inner.gopkg/client/jcapi1/model_systemput.gopkg/client/jcapi1/model_systemput_agent_bound_messages_inner.gopkg/client/jcapi1/model_systemslist.gopkg/client/jcapi1/model_systemuserput.gopkg/client/jcapi1/model_systemuserput_addresses_inner.gopkg/client/jcapi1/model_systemuserput_attributes_inner.gopkg/client/jcapi1/model_systemuserput_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserput_relationships_inner.gopkg/client/jcapi1/model_systemuserputpost.gopkg/client/jcapi1/model_systemuserputpost_addresses_inner.gopkg/client/jcapi1/model_systemuserputpost_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserputpost_recovery_email.gopkg/client/jcapi1/model_systemuserreturn.gopkg/client/jcapi1/model_systemuserreturn_addresses_inner.gopkg/client/jcapi1/model_systemuserreturn_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserreturn_recovery_email.gopkg/client/jcapi1/model_systemusers_resetmfa_request.gopkg/client/jcapi1/model_systemusers_state_activate_request.gopkg/client/jcapi1/model_systemuserslist.gopkg/client/jcapi1/model_triggerreturn.gopkg/client/jcapi1/model_trustedapp_config_get.gopkg/client/jcapi1/model_trustedapp_config_get_trusted_apps_inner.gopkg/client/jcapi1/model_trustedapp_config_put.gopkg/client/jcapi1/model_userput.gopkg/client/jcapi1/model_userreturn.gopkg/client/jcapi1/model_userreturn_growth_data.gopkg/client/jcapi1/response.gopkg/client/jcapi1/test/api_application_templates_test.gopkg/client/jcapi1/test/api_applications_test.gopkg/client/jcapi1/test/api_command_results_test.gopkg/client/jcapi1/test/api_command_triggers_test.gopkg/client/jcapi1/test/api_commands_test.gopkg/client/jcapi1/test/api_managed_service_provider_test.gopkg/client/jcapi1/test/api_organizations_test.gopkg/client/jcapi1/test/api_radius_servers_test.gopkg/client/jcapi1/test/api_search_test.gopkg/client/jcapi1/test/api_systems_test.gopkg/client/jcapi1/test/api_systemusers_test.gopkg/client/jcapi1/test/api_users_test.gopkg/client/jcapi1/utils.gopkg/client/jcapi2/.gitignorepkg/client/jcapi2/.openapi-generator-ignorepkg/client/jcapi2/.openapi-generator/FILESpkg/client/jcapi2/.openapi-generator/VERSIONpkg/client/jcapi2/.travis.ymlpkg/client/jcapi2/README.mdpkg/client/jcapi2/api/openapi.yamlpkg/client/jcapi2/api_active_directory.gopkg/client/jcapi2/api_administrators.gopkg/client/jcapi2/api_apple_mdm.gopkg/client/jcapi2/api_applications.gopkg/client/jcapi2/api_authentication_policies.gopkg/client/jcapi2/api_bulk_job_requests.gopkg/client/jcapi2/api_cloud_insights.gopkg/client/jcapi2/api_command_results.gopkg/client/jcapi2/api_commands.gopkg/client/jcapi2/api_custom_emails.gopkg/client/jcapi2/api_directories.gopkg/client/jcapi2/api_duo.gopkg/client/jcapi2/api_fde.gopkg/client/jcapi2/api_feature_trials.gopkg/client/jcapi2/api_g_suite.gopkg/client/jcapi2/api_g_suite_import.gopkg/client/jcapi2/api_google_emm.gopkg/client/jcapi2/api_graph.gopkg/client/jcapi2/api_groups.gopkg/client/jcapi2/api_image.gopkg/client/jcapi2/api_ip_lists.gopkg/client/jcapi2/api_ldap_servers.gopkg/client/jcapi2/api_logos.gopkg/client/jcapi2/api_managed_service_provider.gopkg/client/jcapi2/api_office365.gopkg/client/jcapi2/api_office365_import.gopkg/client/jcapi2/api_organizations.gopkg/client/jcapi2/api_policies.gopkg/client/jcapi2/api_policy_group_associations.gopkg/client/jcapi2/api_policy_group_members_membership.gopkg/client/jcapi2/api_policy_group_templates.gopkg/client/jcapi2/api_policy_groups.gopkg/client/jcapi2/api_policytemplates.gopkg/client/jcapi2/api_providers.gopkg/client/jcapi2/api_radius_servers.gopkg/client/jcapi2/api_samba_domains.gopkg/client/jcapi2/api_scim_import.gopkg/client/jcapi2/api_software_apps.gopkg/client/jcapi2/api_subscriptions.gopkg/client/jcapi2/api_system_group_associations.go
🔇 Additional comments (2)
pkg/client/helpers.go (2)
17-44: LGTM! Comprehensive HTTP-to-gRPC code mapping.The mapping covers common HTTP status codes appropriately, with sensible fallbacks for unhandled 4xx and 5xx ranges.
146-152: Verify pagination token calculation logic.The formula
nextSkip = resultCount + pageseems to calculate the next offset for skip-based pagination. However, ifpagealready represents the current skip offset (not page number), this logic is correct. But ifpageis a page index starting from 0, the formula might need adjustment to(page + 1) * limitor similar.Please verify this matches the expected pagination contract with the JumpCloud API.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/client/jcapi1/test/api_radius_servers_test.go (1)
12-18: Update the Makefile generator sed replacement to account for the new directory structure.The generator config is out of sync with the actual file locations. The Makefile's sed replacement produces
conductorone/baton-jumpcloud/pkg/jcapi1, but the current imports require the full module pathgithub.com/conductorone/baton-jumpcloud/pkg/connector/client/jcapi1. When these files are regenerated via the Makefile, the import paths will be incorrect and break the code. Update the sed command in bothbuild-jcapi1andbuild-jcapi2targets to account for thegithub.com/prefix and/connector/client/subdirectory.
🤖 Fix all issues with AI agents
In @pkg/connector/client/client.go:
- Around line 84-107: The ListDirectories method currently manually inspects
resp.StatusCode and returns a custom error, making it inconsistent with other
methods; change it to use the shared wrapSDKError helper like other list
methods: after calling
client.DirectoriesApi.DirectoriesList(ctx).Limit(limit).Skip(page).Execute(),
defer closing resp only if resp != nil, then call wrapSDKError(resp, err, "list
directories") to translate/return any non-2xx or underlying errors and return
directories on success; remove the manual resp.StatusCode check and the
uhttp.WrapErrors usage so error handling matches ListSystemUsers/ListGroups.
🧹 Nitpick comments (9)
pkg/connector/client/jcapi1/test/api_organizations_test.go (1)
20-65: Consider removing auto-generated test files with all tests skipped.This auto-generated test file has all tests skipped and provides no test coverage. Consider either:
- Implementing these tests with actual test logic if Organizations API testing is needed
- Removing the file to reduce maintenance burden of unused generated code
pkg/connector/client/helpers.go (1)
83-90:Temporary()is deprecated.The
net.Error.Temporary()method is deprecated as of Go 1.18 and always returnsfalsein many implementations. This check may not work as intended.♻️ Suggested fix: Remove the unreliable check
- // Temporary network errors (DNS, connection issues) - if urlErr.Temporary() { - return uhttp.WrapErrors( - codes.Unavailable, - fmt.Sprintf("%s: temporary network error", operation), - urlErr, - ) - }The default case at lines 102-107 already handles network errors with
codes.Unavailable, so removing this branch won't change behavior.pkg/connector/client/extension/extension.go (3)
25-28: Naming mismatch: method namedSkipsetsPagefield.The method is called
Skipbut it sets thePagefield. Consider renaming the struct field toSkipfor consistency, or renaming the method toSetPage.
30-40: Stale comment references PUT but code uses GET.The curl example in the comment shows a PUT request, but the actual implementation performs a GET request. Consider updating or removing the misleading comment.
47-52: Hardcoded JumpCloud URL.The base URL
console.jumpcloud.comis hardcoded. Consider making this configurable for testing purposes or different environments (e.g., staging).Also applies to: 85-89
pkg/connector/apps.go (1)
291-306: Typo in parameter name:resoureTypeId.♻️ Suggested fix
-func appGrant(resource *v2.Resource, resoureTypeId string, member *jcapi2.GraphConnection) *v2.Grant { +func appGrant(resource *v2.Resource, resourceTypeId string, member *jcapi2.GraphConnection) *v2.Grant { groupID := resource.Id.GetResource() - ur := &v2.Resource{Id: &v2.ResourceId{ResourceType: resoureTypeId, Resource: member.To.Id}} + ur := &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeId, Resource: member.To.Id}}pkg/connector/client/client.go (3)
51-57: Unreachable nil check - dead code.
jcapi1.NewAPIClientandjcapi2.NewAPIClientreturn initialized structs and never returnnil. This check will never trigger.♻️ Suggested fix: Remove the dead code
baseHttpClient, err := uhttp.NewBaseHttpClientWithContext(ctx, httpClient) if err != nil { return nil, err } - if client1 == nil || client2 == nil { - return nil, uhttp.WrapErrors( - codes.InvalidArgument, - "client initialization failed: API client not configured", - fmt.Errorf("API clients not properly initialized during connector setup"), - ) - } - return &Client{
109-124: Missing error wrapping inGetUserByID.Unlike other methods, this doesn't wrap errors with
wrapSDKError. Consider wrapping for consistency.♻️ Suggested fix
func (jc *Client) GetUserByID(ctx context.Context, userID string) (*jcapi1.Userreturn, error) { userGetRequest := &extension.UserGetRequest{ Client: jc.extensionClient, ApiKey: jc.apiKey, OrgId: jc.orgId, UserID: userID, } user, resp, err := userGetRequest.Execute(ctx) if err != nil { - return nil, err + return nil, wrapSDKError(err, resp, "failed to get user by ID") } defer resp.Body.Close() return user, nil }
142-160: Redundant pagination:Pageis set twice.
Pageis set topageat line 148, thenSkip(page)is called at line 151 which setsPageagain. This is redundant.♻️ Suggested fix: Remove the redundant assignment
func (jc *Client) ListAdminUsers(ctx context.Context, opts *Options) ([]jcapi1.Userreturn, string, error) { page := opts.getPage() userListRequest := &extension.UserListRequest{ Client: jc.extensionClient, ApiKey: jc.apiKey, OrgId: jc.orgId, - Page: page, } users, resp, err := userListRequest.Skip(page).Execute(ctx)Or alternatively, set
Pagedirectly and remove theSkip(page)call.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (298)
pkg/connector/apps.gopkg/connector/client/client.gopkg/connector/client/extension/extension.gopkg/connector/client/helpers.gopkg/connector/client/jcapi1/.gitignorepkg/connector/client/jcapi1/.openapi-generator-ignorepkg/connector/client/jcapi1/.openapi-generator/FILESpkg/connector/client/jcapi1/.openapi-generator/VERSIONpkg/connector/client/jcapi1/.travis.ymlpkg/connector/client/jcapi1/README.mdpkg/connector/client/jcapi1/api/openapi.yamlpkg/connector/client/jcapi1/api_application_templates.gopkg/connector/client/jcapi1/api_applications.gopkg/connector/client/jcapi1/api_command_results.gopkg/connector/client/jcapi1/api_command_triggers.gopkg/connector/client/jcapi1/api_commands.gopkg/connector/client/jcapi1/api_managed_service_provider.gopkg/connector/client/jcapi1/api_organizations.gopkg/connector/client/jcapi1/api_radius_servers.gopkg/connector/client/jcapi1/api_search.gopkg/connector/client/jcapi1/api_systems.gopkg/connector/client/jcapi1/api_systemusers.gopkg/connector/client/jcapi1/api_users.gopkg/connector/client/jcapi1/client.gopkg/connector/client/jcapi1/configuration.gopkg/connector/client/jcapi1/docs/Application.mdpkg/connector/client/jcapi1/docs/ApplicationConfig.mdpkg/connector/client/jcapi1/docs/ApplicationConfigAcsUrl.mdpkg/connector/client/jcapi1/docs/ApplicationConfigAcsUrlTooltip.mdpkg/connector/client/jcapi1/docs/ApplicationConfigAcsUrlTooltipVariables.mdpkg/connector/client/jcapi1/docs/ApplicationConfigConstantAttributes.mdpkg/connector/client/jcapi1/docs/ApplicationConfigConstantAttributesValueInner.mdpkg/connector/client/jcapi1/docs/ApplicationConfigDatabaseAttributes.mdpkg/connector/client/jcapi1/docs/ApplicationLogo.mdpkg/connector/client/jcapi1/docs/ApplicationTemplatesApi.mdpkg/connector/client/jcapi1/docs/ApplicationsApi.mdpkg/connector/client/jcapi1/docs/Applicationslist.mdpkg/connector/client/jcapi1/docs/Applicationtemplate.mdpkg/connector/client/jcapi1/docs/ApplicationtemplateJit.mdpkg/connector/client/jcapi1/docs/ApplicationtemplateLogo.mdpkg/connector/client/jcapi1/docs/ApplicationtemplateOidc.mdpkg/connector/client/jcapi1/docs/ApplicationtemplateProvision.mdpkg/connector/client/jcapi1/docs/Applicationtemplateslist.mdpkg/connector/client/jcapi1/docs/Command.mdpkg/connector/client/jcapi1/docs/CommandResultsApi.mdpkg/connector/client/jcapi1/docs/CommandTriggersApi.mdpkg/connector/client/jcapi1/docs/Commandfilereturn.mdpkg/connector/client/jcapi1/docs/CommandfilereturnResultsInner.mdpkg/connector/client/jcapi1/docs/Commandresult.mdpkg/connector/client/jcapi1/docs/CommandresultResponse.mdpkg/connector/client/jcapi1/docs/CommandresultResponseData.mdpkg/connector/client/jcapi1/docs/Commandresultslist.mdpkg/connector/client/jcapi1/docs/CommandresultslistResultsInner.mdpkg/connector/client/jcapi1/docs/CommandsApi.mdpkg/connector/client/jcapi1/docs/Commandslist.mdpkg/connector/client/jcapi1/docs/CommandslistResultsInner.mdpkg/connector/client/jcapi1/docs/Error.mdpkg/connector/client/jcapi1/docs/ErrorDetails.mdpkg/connector/client/jcapi1/docs/ErrorDetailsAllOf.mdpkg/connector/client/jcapi1/docs/Fde.mdpkg/connector/client/jcapi1/docs/ManagedServiceProviderApi.mdpkg/connector/client/jcapi1/docs/Mfa.mdpkg/connector/client/jcapi1/docs/MfaEnrollment.mdpkg/connector/client/jcapi1/docs/MfaEnrollmentStatus.mdpkg/connector/client/jcapi1/docs/Organization.mdpkg/connector/client/jcapi1/docs/OrganizationPutRequest.mdpkg/connector/client/jcapi1/docs/Organizationentitlement.mdpkg/connector/client/jcapi1/docs/OrganizationentitlementEntitlementProductsInner.mdpkg/connector/client/jcapi1/docs/OrganizationsApi.mdpkg/connector/client/jcapi1/docs/Organizationsettings.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsFeatures.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsights.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsightsPremium.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsFeaturesSystemInsights.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsNewSystemUserStateDefaults.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsPasswordPolicy.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsUserPortal.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsWindowsMDM.mdpkg/connector/client/jcapi1/docs/Organizationsettingsput.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsputNewSystemUserStateDefaults.mdpkg/connector/client/jcapi1/docs/OrganizationsettingsputPasswordPolicy.mdpkg/connector/client/jcapi1/docs/Organizationslist.mdpkg/connector/client/jcapi1/docs/OrganizationslistResultsInner.mdpkg/connector/client/jcapi1/docs/RadiusServersApi.mdpkg/connector/client/jcapi1/docs/RadiusServersPutRequest.mdpkg/connector/client/jcapi1/docs/Radiusserver.mdpkg/connector/client/jcapi1/docs/Radiusserverpost.mdpkg/connector/client/jcapi1/docs/Radiusserverput.mdpkg/connector/client/jcapi1/docs/Radiusserverslist.mdpkg/connector/client/jcapi1/docs/Search.mdpkg/connector/client/jcapi1/docs/SearchApi.mdpkg/connector/client/jcapi1/docs/Sshkeylist.mdpkg/connector/client/jcapi1/docs/Sshkeypost.mdpkg/connector/client/jcapi1/docs/Sso.mdpkg/connector/client/jcapi1/docs/System.mdpkg/connector/client/jcapi1/docs/SystemBuiltInCommandsInner.mdpkg/connector/client/jcapi1/docs/SystemDomainInfo.mdpkg/connector/client/jcapi1/docs/SystemMdm.mdpkg/connector/client/jcapi1/docs/SystemMdmInternal.mdpkg/connector/client/jcapi1/docs/SystemNetworkInterfacesInner.mdpkg/connector/client/jcapi1/docs/SystemOsVersionDetail.mdpkg/connector/client/jcapi1/docs/SystemProvisionMetadata.mdpkg/connector/client/jcapi1/docs/SystemProvisionMetadataProvisioner.mdpkg/connector/client/jcapi1/docs/SystemServiceAccountState.mdpkg/connector/client/jcapi1/docs/SystemSshdParamsInner.mdpkg/connector/client/jcapi1/docs/SystemSystemInsights.mdpkg/connector/client/jcapi1/docs/SystemUserMetricsInner.mdpkg/connector/client/jcapi1/docs/Systemput.mdpkg/connector/client/jcapi1/docs/SystemputAgentBoundMessagesInner.mdpkg/connector/client/jcapi1/docs/SystemsApi.mdpkg/connector/client/jcapi1/docs/Systemslist.mdpkg/connector/client/jcapi1/docs/Systemuserput.mdpkg/connector/client/jcapi1/docs/SystemuserputAddressesInner.mdpkg/connector/client/jcapi1/docs/SystemuserputAttributesInner.mdpkg/connector/client/jcapi1/docs/SystemuserputPhoneNumbersInner.mdpkg/connector/client/jcapi1/docs/SystemuserputRelationshipsInner.mdpkg/connector/client/jcapi1/docs/Systemuserputpost.mdpkg/connector/client/jcapi1/docs/SystemuserputpostAddressesInner.mdpkg/connector/client/jcapi1/docs/SystemuserputpostPhoneNumbersInner.mdpkg/connector/client/jcapi1/docs/SystemuserputpostRecoveryEmail.mdpkg/connector/client/jcapi1/docs/Systemuserreturn.mdpkg/connector/client/jcapi1/docs/SystemuserreturnAddressesInner.mdpkg/connector/client/jcapi1/docs/SystemuserreturnPhoneNumbersInner.mdpkg/connector/client/jcapi1/docs/SystemuserreturnRecoveryEmail.mdpkg/connector/client/jcapi1/docs/SystemusersApi.mdpkg/connector/client/jcapi1/docs/SystemusersResetmfaRequest.mdpkg/connector/client/jcapi1/docs/SystemusersStateActivateRequest.mdpkg/connector/client/jcapi1/docs/Systemuserslist.mdpkg/connector/client/jcapi1/docs/Triggerreturn.mdpkg/connector/client/jcapi1/docs/TrustedappConfigGet.mdpkg/connector/client/jcapi1/docs/TrustedappConfigGetTrustedAppsInner.mdpkg/connector/client/jcapi1/docs/TrustedappConfigPut.mdpkg/connector/client/jcapi1/docs/Userput.mdpkg/connector/client/jcapi1/docs/Userreturn.mdpkg/connector/client/jcapi1/docs/UserreturnGrowthData.mdpkg/connector/client/jcapi1/docs/UsersApi.mdpkg/connector/client/jcapi1/git_push.shpkg/connector/client/jcapi1/model_application.gopkg/connector/client/jcapi1/model_application_config.gopkg/connector/client/jcapi1/model_application_config_acs_url.gopkg/connector/client/jcapi1/model_application_config_acs_url_tooltip.gopkg/connector/client/jcapi1/model_application_config_acs_url_tooltip_variables.gopkg/connector/client/jcapi1/model_application_config_constant_attributes.gopkg/connector/client/jcapi1/model_application_config_constant_attributes_value_inner.gopkg/connector/client/jcapi1/model_application_config_database_attributes.gopkg/connector/client/jcapi1/model_application_logo.gopkg/connector/client/jcapi1/model_applicationslist.gopkg/connector/client/jcapi1/model_applicationtemplate.gopkg/connector/client/jcapi1/model_applicationtemplate_jit.gopkg/connector/client/jcapi1/model_applicationtemplate_logo.gopkg/connector/client/jcapi1/model_applicationtemplate_oidc.gopkg/connector/client/jcapi1/model_applicationtemplate_provision.gopkg/connector/client/jcapi1/model_applicationtemplateslist.gopkg/connector/client/jcapi1/model_command.gopkg/connector/client/jcapi1/model_commandfilereturn.gopkg/connector/client/jcapi1/model_commandfilereturn_results_inner.gopkg/connector/client/jcapi1/model_commandresult.gopkg/connector/client/jcapi1/model_commandresult_response.gopkg/connector/client/jcapi1/model_commandresult_response_data.gopkg/connector/client/jcapi1/model_commandresultslist.gopkg/connector/client/jcapi1/model_commandresultslist_results_inner.gopkg/connector/client/jcapi1/model_commandslist.gopkg/connector/client/jcapi1/model_commandslist_results_inner.gopkg/connector/client/jcapi1/model_error.gopkg/connector/client/jcapi1/model_error_details.gopkg/connector/client/jcapi1/model_error_details_all_of.gopkg/connector/client/jcapi1/model_fde.gopkg/connector/client/jcapi1/model_mfa.gopkg/connector/client/jcapi1/model_mfa_enrollment.gopkg/connector/client/jcapi1/model_mfa_enrollment_status.gopkg/connector/client/jcapi1/model_organization.gopkg/connector/client/jcapi1/model_organization_put_request.gopkg/connector/client/jcapi1/model_organizationentitlement.gopkg/connector/client/jcapi1/model_organizationentitlement_entitlement_products_inner.gopkg/connector/client/jcapi1/model_organizationsettings.gopkg/connector/client/jcapi1/model_organizationsettings_features.gopkg/connector/client/jcapi1/model_organizationsettings_features_directory_insights.gopkg/connector/client/jcapi1/model_organizationsettings_features_directory_insights_premium.gopkg/connector/client/jcapi1/model_organizationsettings_features_system_insights.gopkg/connector/client/jcapi1/model_organizationsettings_new_system_user_state_defaults.gopkg/connector/client/jcapi1/model_organizationsettings_password_policy.gopkg/connector/client/jcapi1/model_organizationsettings_user_portal.gopkg/connector/client/jcapi1/model_organizationsettings_windows_mdm.gopkg/connector/client/jcapi1/model_organizationsettingsput.gopkg/connector/client/jcapi1/model_organizationsettingsput_new_system_user_state_defaults.gopkg/connector/client/jcapi1/model_organizationsettingsput_password_policy.gopkg/connector/client/jcapi1/model_organizationslist.gopkg/connector/client/jcapi1/model_organizationslist_results_inner.gopkg/connector/client/jcapi1/model_radius_servers_put_request.gopkg/connector/client/jcapi1/model_radiusserver.gopkg/connector/client/jcapi1/model_radiusserverpost.gopkg/connector/client/jcapi1/model_radiusserverput.gopkg/connector/client/jcapi1/model_radiusserverslist.gopkg/connector/client/jcapi1/model_search.gopkg/connector/client/jcapi1/model_sshkeylist.gopkg/connector/client/jcapi1/model_sshkeypost.gopkg/connector/client/jcapi1/model_sso.gopkg/connector/client/jcapi1/model_system.gopkg/connector/client/jcapi1/model_system_built_in_commands_inner.gopkg/connector/client/jcapi1/model_system_domain_info.gopkg/connector/client/jcapi1/model_system_mdm.gopkg/connector/client/jcapi1/model_system_mdm_internal.gopkg/connector/client/jcapi1/model_system_network_interfaces_inner.gopkg/connector/client/jcapi1/model_system_os_version_detail.gopkg/connector/client/jcapi1/model_system_provision_metadata.gopkg/connector/client/jcapi1/model_system_provision_metadata_provisioner.gopkg/connector/client/jcapi1/model_system_service_account_state.gopkg/connector/client/jcapi1/model_system_sshd_params_inner.gopkg/connector/client/jcapi1/model_system_system_insights.gopkg/connector/client/jcapi1/model_system_user_metrics_inner.gopkg/connector/client/jcapi1/model_systemput.gopkg/connector/client/jcapi1/model_systemput_agent_bound_messages_inner.gopkg/connector/client/jcapi1/model_systemslist.gopkg/connector/client/jcapi1/model_systemuserput.gopkg/connector/client/jcapi1/model_systemuserput_addresses_inner.gopkg/connector/client/jcapi1/model_systemuserput_attributes_inner.gopkg/connector/client/jcapi1/model_systemuserput_phone_numbers_inner.gopkg/connector/client/jcapi1/model_systemuserput_relationships_inner.gopkg/connector/client/jcapi1/model_systemuserputpost.gopkg/connector/client/jcapi1/model_systemuserputpost_addresses_inner.gopkg/connector/client/jcapi1/model_systemuserputpost_phone_numbers_inner.gopkg/connector/client/jcapi1/model_systemuserputpost_recovery_email.gopkg/connector/client/jcapi1/model_systemuserreturn.gopkg/connector/client/jcapi1/model_systemuserreturn_addresses_inner.gopkg/connector/client/jcapi1/model_systemuserreturn_phone_numbers_inner.gopkg/connector/client/jcapi1/model_systemuserreturn_recovery_email.gopkg/connector/client/jcapi1/model_systemusers_resetmfa_request.gopkg/connector/client/jcapi1/model_systemusers_state_activate_request.gopkg/connector/client/jcapi1/model_systemuserslist.gopkg/connector/client/jcapi1/model_triggerreturn.gopkg/connector/client/jcapi1/model_trustedapp_config_get.gopkg/connector/client/jcapi1/model_trustedapp_config_get_trusted_apps_inner.gopkg/connector/client/jcapi1/model_trustedapp_config_put.gopkg/connector/client/jcapi1/model_userput.gopkg/connector/client/jcapi1/model_userreturn.gopkg/connector/client/jcapi1/model_userreturn_growth_data.gopkg/connector/client/jcapi1/response.gopkg/connector/client/jcapi1/test/api_application_templates_test.gopkg/connector/client/jcapi1/test/api_applications_test.gopkg/connector/client/jcapi1/test/api_command_results_test.gopkg/connector/client/jcapi1/test/api_command_triggers_test.gopkg/connector/client/jcapi1/test/api_commands_test.gopkg/connector/client/jcapi1/test/api_managed_service_provider_test.gopkg/connector/client/jcapi1/test/api_organizations_test.gopkg/connector/client/jcapi1/test/api_radius_servers_test.gopkg/connector/client/jcapi1/test/api_search_test.gopkg/connector/client/jcapi1/test/api_systems_test.gopkg/connector/client/jcapi1/test/api_systemusers_test.gopkg/connector/client/jcapi1/test/api_users_test.gopkg/connector/client/jcapi1/utils.gopkg/connector/client/jcapi2/.gitignorepkg/connector/client/jcapi2/.openapi-generator-ignorepkg/connector/client/jcapi2/.openapi-generator/FILESpkg/connector/client/jcapi2/.openapi-generator/VERSIONpkg/connector/client/jcapi2/.travis.ymlpkg/connector/client/jcapi2/README.mdpkg/connector/client/jcapi2/api/openapi.yamlpkg/connector/client/jcapi2/api_active_directory.gopkg/connector/client/jcapi2/api_administrators.gopkg/connector/client/jcapi2/api_apple_mdm.gopkg/connector/client/jcapi2/api_applications.gopkg/connector/client/jcapi2/api_authentication_policies.gopkg/connector/client/jcapi2/api_bulk_job_requests.gopkg/connector/client/jcapi2/api_cloud_insights.gopkg/connector/client/jcapi2/api_command_results.gopkg/connector/client/jcapi2/api_commands.gopkg/connector/client/jcapi2/api_custom_emails.gopkg/connector/client/jcapi2/api_directories.gopkg/connector/client/jcapi2/api_duo.gopkg/connector/client/jcapi2/api_fde.gopkg/connector/client/jcapi2/api_feature_trials.gopkg/connector/client/jcapi2/api_g_suite.gopkg/connector/client/jcapi2/api_g_suite_import.gopkg/connector/client/jcapi2/api_google_emm.gopkg/connector/client/jcapi2/api_graph.gopkg/connector/client/jcapi2/api_groups.gopkg/connector/client/jcapi2/api_image.gopkg/connector/client/jcapi2/api_ip_lists.gopkg/connector/client/jcapi2/api_ldap_servers.gopkg/connector/client/jcapi2/api_logos.gopkg/connector/client/jcapi2/api_managed_service_provider.gopkg/connector/client/jcapi2/api_office365.gopkg/connector/client/jcapi2/api_office365_import.gopkg/connector/client/jcapi2/api_organizations.gopkg/connector/client/jcapi2/api_policies.gopkg/connector/client/jcapi2/api_policy_group_associations.gopkg/connector/client/jcapi2/api_policy_group_members_membership.gopkg/connector/client/jcapi2/api_policy_group_templates.gopkg/connector/client/jcapi2/api_policy_groups.gopkg/connector/client/jcapi2/api_policytemplates.gopkg/connector/client/jcapi2/api_providers.gopkg/connector/client/jcapi2/api_radius_servers.gopkg/connector/client/jcapi2/api_samba_domains.gopkg/connector/client/jcapi2/api_scim_import.gopkg/connector/client/jcapi2/api_software_apps.gopkg/connector/client/jcapi2/api_subscriptions.gopkg/connector/client/jcapi2/api_system_group_associations.gopkg/connector/client/jcapi2/api_system_group_members_membership.go
✅ Files skipped from review due to trivial changes (4)
- pkg/connector/client/jcapi1/test/api_search_test.go
- pkg/connector/client/jcapi1/docs/CommandTriggersApi.md
- pkg/connector/client/jcapi1/README.md
- pkg/connector/client/jcapi1/docs/RadiusServersApi.md
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/connector/client/client.go (1)
pkg/connector/client/options.go (1)
Options(3-7)
pkg/connector/apps.go (2)
pkg/connector/client/client.go (1)
Client(15-21)pkg/connector/client/options.go (1)
Options(3-7)
🔇 Additional comments (32)
pkg/connector/client/jcapi2/README.md (1)
321-321: Import path correctly updated to reflect new package structure.The example import statement now points to the correct module path under
pkg/connector/client/jcapi2, consistent with the PR's reorganization of the connector client package.pkg/connector/client/jcapi1/test/api_organizations_test.go (1)
17-17: LGTM! Import path updated correctly.The import path change aligns with the PR's refactoring to reorganize modules under
pkg/connector/client/.pkg/connector/client/jcapi1/test/api_managed_service_provider_test.go (1)
12-18: LGTM: updated openapi client import path.pkg/connector/client/jcapi1/test/api_command_results_test.go (1)
12-18: LGTM: updated openapi client import path.pkg/connector/client/jcapi1/test/api_systemusers_test.go (1)
12-18: LGTM: updated openapi client import path.pkg/connector/client/jcapi1/docs/SearchApi.md (5)
28-33: Example import path updated consistently with new jcapi1 location.
104-109: Example import path updated consistently with new jcapi1 location.
180-185: Example import path updated consistently with new jcapi1 location.
254-259: Example import path updated consistently with new jcapi1 location.
330-335: Example import path updated consistently with new jcapi1 location.pkg/connector/client/jcapi1/test/api_systems_test.go (1)
12-18: Import path migration is complete; old path fully eliminated.The old
pkg/jcapi1path has no remaining references in the codebase. All 17 usages—including test files like api_systems_test.go—consistently import from the newpkg/connector/client/jcapi1location. The test file at line 17 is correct.Note: Test files are part of the OpenAPI-generated artifacts (per the
.openapi-generator/FILESmanifest), but the actual generation invocation is not visible in the repository. The.openapi-generator-ignorefile is currently empty, so consider either adding test files to it or verifying the OpenAPI generation configuration specifies the new module path.pkg/connector/client/jcapi1/docs/CommandsApi.md (1)
30-35: Doc examples updated to newjcapi1import path (no behavior change).
Consistent with the repo refactor; LGTM.Also applies to: 108-113, 180-185, 254-259, 328-333, 404-409, 472-477
pkg/connector/client/jcapi1/test/api_commands_test.go (1)
12-18: Test import updated to relocatedjcapi1package.
LGTM; no functional change beyond the path update.pkg/connector/client/jcapi1/test/api_users_test.go (1)
12-18: Test import updated to relocatedjcapi1package.
LGTM; aligns with the new directory structure.pkg/connector/client/jcapi1/test/api_application_templates_test.go (1)
12-18: Test import updated to relocatedjcapi1package.
LGTM; consistent with other updated jcapi1 tests/docs.pkg/connector/client/jcapi1/docs/ManagedServiceProviderApi.md (1)
27-32: Doc examples updated to newjcapi1import path.
The import path correctly referencesgithub.com/conductorone/baton-jumpcloud/pkg/connector/client/jcapi1. Verified that no stale references to the old path remain in the codebase, and the OpenAPI generator is correctly configured.pkg/connector/client/jcapi1/docs/OrganizationsApi.md (1)
30-30: LGTM! Import paths updated correctly.The import path updates in the documentation code examples are consistent and correctly reflect the new module structure under
pkg/connector/client/jcapi1.Also applies to: 108-108, 180-180
pkg/connector/client/jcapi1/test/api_command_triggers_test.go (1)
17-17: LGTM! Import path correctly updated.The import statement has been updated to reflect the new module structure. Test logic remains unchanged.
pkg/connector/client/jcapi1/test/api_applications_test.go (1)
17-17: LGTM! Import path correctly updated.The import statement has been updated to reflect the new module structure. All test logic remains unchanged.
pkg/connector/client/jcapi1/docs/SystemsApi.md (1)
35-35: LGTM! Import paths updated correctly.All code examples in the documentation have been updated with the correct import path reflecting the new module structure under
pkg/connector/client/jcapi1.Also applies to: 105-105, 175-175, 245-245, 315-315, 391-391, 471-471, 549-549
pkg/connector/client/jcapi1/docs/UsersApi.md (1)
30-30: LGTM! Import paths updated correctly.The import path updates in the documentation code examples are consistent and correctly reflect the new module structure under
pkg/connector/client/jcapi1.Also applies to: 98-98, 172-172
pkg/connector/client/jcapi1/docs/ApplicationTemplatesApi.md (1)
29-29: Import path update in examples looks correct.
This aligns the docs with the newpkg/connector/client/jcapi1location.Also applies to: 111-111
pkg/connector/client/jcapi1/docs/SystemusersApi.md (1)
40-40: Import path updates in examples look consistent throughout.
All snippets now referencegithub.com/conductorone/baton-jumpcloud/pkg/connector/client/jcapi1.Also applies to: 115-115, 187-187, 261-261, 335-335, 407-407, 483-483, 561-561, 629-629, 699-699, 775-775, 849-849, 921-921
pkg/connector/client/jcapi1/docs/CommandResultsApi.md (1)
30-30: Docs example imports updated correctly.Also applies to: 102-102, 178-178
pkg/connector/client/jcapi1/docs/ApplicationsApi.md (1)
32-32: Docs example imports updated correctly and consistently.Also applies to: 104-104, 176-176, 252-252, 320-320
pkg/connector/client/helpers.go (2)
17-44: LGTM! Comprehensive HTTP to gRPC code mapping.The mapping covers standard HTTP error codes with sensible fallbacks for 4xx/5xx ranges.
146-152: LGTM! Pagination token generation is correct.The logic correctly computes the next skip offset by adding the current result count to the current page offset.
pkg/connector/client/extension/extension.go (1)
82-110: LGTM! UserGetRequest implementation is clean.Uses proper path escaping for the user ID and follows the same pattern as UserListRequest.
pkg/connector/apps.go (3)
54-58: LGTM! Clean integration with the new client.The options pattern and token-based pagination are properly integrated.
149-157: LGTM! Good use of batch lookup via cache.Collecting emails and performing a batch lookup via
GetSystemUsersByEmailListis more efficient than individual lookups.
257-259: Good defensive check for unexpected pagination state.This guard prevents silent failures when the pagination state is invalid.
pkg/connector/client/client.go (1)
204-234: LGTM! Add/Remove group member methods are well-structured.Proper error handling with
wrapSDKErrorand resource cleanup withdefer resp.Body.Close().
luisina-santos
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
44543df to
add4f1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @pkg/connector/client/client.go:
- Line 6: The build fails due to an unused import "net/http" in the client
package; remove "net/http" from the import block in
pkg/connector/client/client.go (the import list that includes "net/http") so the
package compiles, or if the intent was to use it, replace the unused import with
actual usage in the Client code (e.g., in functions like NewClient or any
HTTP-related methods) — otherwise simply delete the "net/http" entry from the
imports.
- Around line 152-162: The error message in GetSystemUserByID incorrectly says
"failed to fetch manager details"; update the wrapSDKError call in
Client.GetSystemUserByID to use an accurate message like "failed to get system
user" (or similar) so it reflects the operation performed; locate the
wrapSDKError invocation in the GetSystemUserByID method and replace the message
string accordingly while leaving error and resp handling unchanged.
- Around line 51-57: Remove the unreachable nil-check block that tests client1
and client2 (created via jcapi1.NewAPIClient and jcapi2.NewAPIClient); delete
the if client1 == nil || client2 == nil { ... } return uhttp.WrapErrors(...)
branch so the function no longer returns that error, relying on the constructors
which always return non-nil *APIClient values; ensure no other code expects that
error path.
- Around line 99-114: GetUserByID currently returns the raw error from
userGetRequest.Execute(ctx) instead of wrapping it like other methods; update
the error return to use wrapSDKError(err, resp, "failed to get user by ID") so
that GetUserByID (in pkg/connector/client/client.go) calls wrapSDKError when the
Execute call fails, preserving resp for the wrapper and returning nil plus the
wrapped error.
🧹 Nitpick comments (2)
pkg/connector/client/client.go (2)
15-21: Consider consistent field naming.The private API clients use underscore prefixes (
_client1,_client2) while other private fields (extensionClient,apiKey,orgId) do not. For consistency, either remove the underscores from_client1and_client2or apply a consistent convention across all unexported fields.
24-24: Extract the user agent string to a constant.The user agent string
"baton-jumpcloud/0.1.0"is duplicated on lines 24, 31, and 35. Consider extracting it to a package-level constant to ensure consistency and ease future updates.♻️ Proposed refactor
Add a constant at the package level:
const userAgent = "baton-jumpcloud/0.1.0"Then replace the three occurrences:
- httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent("baton-jumpcloud/0.1.0")) + httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent(userAgent)) if err != nil { return nil, err } cc1 := jcapi1.NewConfiguration() cc1.HTTPClient = httpClient - cc1.UserAgent = "baton-jumpcloud/0.1.0" + cc1.UserAgent = userAgent cc2 := jcapi2.NewConfiguration() cc2.HTTPClient = httpClient - cc2.UserAgent = "baton-jumpcloud/0.1.0" + cc2.UserAgent = userAgent
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/client/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/client/client.go (1)
pkg/connector/client/options.go (1)
Options(3-7)
🪛 GitHub Actions: ci
pkg/connector/client/client.go
[error] 6-6: build error: "net/http" imported and not used. Remove the unused import to fix the compilation. Command failed: go test -v -covermode=count -json ./... | tee test.json
🪛 GitHub Check: go-lint
pkg/connector/client/client.go
[failure] 6-6:
"net/http" imported and not used (typecheck)
🪛 GitHub Check: test-groups
pkg/connector/client/client.go
[failure] 6-6:
"net/http" imported and not used
🔇 Additional comments (3)
pkg/connector/client/client.go (3)
68-82: LGTM!The helper methods correctly bind API keys to the context for authentication with each API client.
164-224: LGTM!The group-related methods follow consistent patterns for listing, adding, and removing members, with proper error handling and resource cleanup.
226-255: LGTM!The application-related methods correctly implement listing and association retrieval with proper pagination, error handling, and resource cleanup.
All helper functions (
wrapSDKError,getNextPageToken,getLimit,getPage,getTargets) are properly defined in their respective modules (helpers.goandoptions.go), with appropriate null-safety and sensible defaults.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @pkg/connector/client/client.go:
- Around line 151-161: The error message in GetSystemUserByID is incorrect;
update the wrapSDKError call inside the error branch of Client.GetSystemUserByID
to use an accurate message like "failed to fetch system user details" (or
similar) instead of "failed to fetch manager details" so the returned error from
wrapSDKError correctly reflects the SystemusersApi.SystemusersGet operation.
- Around line 98-113: GetUserByID returns raw errors instead of using the
client's standardized wrapper; update the error return in GetUserByID (in
pkg/connector/client/client.go) to call wrapSDKError(err, resp) instead of
returning err directly so its error handling matches other methods (keep the
rest of the function the same and ensure resp is passed into wrapSDKError as
done elsewhere).
- Around line 131-149: In ListAdminUsers remove the redundant Page field
assignment from the userListRequest struct literal: the Page is already set by
the subsequent userListRequest.Skip(page).Execute call, so delete the "Page:
page," entry when constructing the extension.UserListRequest (variable
userListRequest) to avoid duplicating the same value.
- Around line 83-96: The List* methods (ListDirectories, ListSystemUsers,
ListAdminUsers, ListGroups, ListGroupMembers, ListApplications,
ListApplicationAssociations) can panic if opts is nil because they call
opts.getLimit()/getPage()/getTargets(); add a defensive nil-check at the top of
each method such as "if opts == nil { opts = &Options{} }" so the subsequent
calls are safe, then proceed as before (reuse existing symbols: jc.client2,
wrapSDKError, and the API call chains).
🧹 Nitpick comments (1)
pkg/connector/client/client.go (1)
23-23: Consider extracting the user agent string to a constant.The user agent
"baton-jumpcloud/0.1.0"is duplicated three times. Extracting it to a package-level constant would improve maintainability and ensure consistency.♻️ Proposed refactor
Add a constant at the top of the file:
+const userAgent = "baton-jumpcloud/0.1.0" + type Client struct {Then use it throughout:
- httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent("baton-jumpcloud/0.1.0")) + httpClient, err := uhttp.NewClient(ctx, uhttp.WithLogger(true, nil), uhttp.WithUserAgent(userAgent))- cc1.UserAgent = "baton-jumpcloud/0.1.0" + cc1.UserAgent = userAgent- cc2.UserAgent = "baton-jumpcloud/0.1.0" + cc2.UserAgent = userAgentAlso applies to: 30-30, 34-34
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/client/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/client/client.go (1)
pkg/connector/client/options.go (1)
Options(3-7)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/client/jcapi1/test/api_application_templates_test.go`:
- Around line 16-18: The import block has a corrupted/malformed line causing
compile errors: replace the broken line containing
`github.com/stretchr/testify/assert"batbaton-jumpcloud/pkg/client/jcapi` with a
correct import entry `github.com/stretchr/testify/assert` and remove the stray
`batbaton-jumpcloud/pkg/client/jcapi` fragment; ensure the import group includes
`openapiclient "github.com/conductorone/baton-jumpcloud/pkg/client/jcapi1"`,
`github.com/stretchr/testify/assert`, and `github.com/stretchr/testify/require`
so the file compiles.
In `@pkg/client/jcapi1/test/api_applications_test.go`:
- Around line 16-18: The import block in api_applications_test.go has a syntax
error: the assert import line contains stray text
"baton-jumpcloud/pkg/client/jcapi" appended to it; fix it by removing the
extraneous text so the import reads just "github.com/stretchr/testify/assert"
(or, if you intended to import the jcapi package, add it as a separate quoted
import "github.com/conductorone/baton-jumpcloud/pkg/client/jcapi"). Ensure the
import block uses one valid quoted import per line and run `go vet`/`go build`
to verify; check symbols in this file such as openapiclient and any references
to jcapi to decide whether to add the jcapi import separately.
In `@pkg/client/jcapi1/test/api_command_results_test.go`:
- Around line 16-18: The import block contains a corrupted combined string
("github.com/stretchr/testify/assert"batbaton-jumpcloud/pkg/client/jcapi) that
breaks compilation; split this into two proper import entries — one for
"github.com/stretchr/testify/assert" and one for the jcapi client package (the
correct module path used elsewhere:
github.com/conductorone/baton-jumpcloud/pkg/client/jcapi) so that the existing
symbols (openapiclient, assert, require, and jcapi) resolve correctly.
🧹 Nitpick comments (3)
pkg/client/helpers.go (1)
17-41: Consider mapping HTTP 408 to DeadlineExceeded.
Request timeouts are usually retryable; mapping 408 toDeadlineExceededkeeps retry logic consistent.♻️ Suggested tweak
- case http.StatusGatewayTimeout: + case http.StatusRequestTimeout, http.StatusGatewayTimeout: return codes.DeadlineExceededpkg/client/extension/extension.go (1)
47-52: Make the API host configurable.
Hardcoding the API host makes staging/testing and future endpoint changes harder. Consider threading a base URL from the client/config instead of inlining it here.Also applies to: 85-89
pkg/client/client.go (1)
127-146: ListAdminUsers ignoresopts.getLimit().All other list methods in the client properly plumb the limit through to their API calls (e.g.,
ListDirectories,ListSystemUsers,ListGroups). TheUserListRequestshould be updated to support and use the limit parameter to maintain consistent pagination behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (298)
pkg/client/client.gopkg/client/extension/extension.gopkg/client/helpers.gopkg/client/jcapi1/.gitignorepkg/client/jcapi1/.openapi-generator-ignorepkg/client/jcapi1/.openapi-generator/FILESpkg/client/jcapi1/.openapi-generator/VERSIONpkg/client/jcapi1/.travis.ymlpkg/client/jcapi1/README.mdpkg/client/jcapi1/api/openapi.yamlpkg/client/jcapi1/api_application_templates.gopkg/client/jcapi1/api_applications.gopkg/client/jcapi1/api_command_results.gopkg/client/jcapi1/api_command_triggers.gopkg/client/jcapi1/api_commands.gopkg/client/jcapi1/api_managed_service_provider.gopkg/client/jcapi1/api_organizations.gopkg/client/jcapi1/api_radius_servers.gopkg/client/jcapi1/api_search.gopkg/client/jcapi1/api_systems.gopkg/client/jcapi1/api_systemusers.gopkg/client/jcapi1/api_users.gopkg/client/jcapi1/client.gopkg/client/jcapi1/configuration.gopkg/client/jcapi1/docs/Application.mdpkg/client/jcapi1/docs/ApplicationConfig.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrl.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrlTooltip.mdpkg/client/jcapi1/docs/ApplicationConfigAcsUrlTooltipVariables.mdpkg/client/jcapi1/docs/ApplicationConfigConstantAttributes.mdpkg/client/jcapi1/docs/ApplicationConfigConstantAttributesValueInner.mdpkg/client/jcapi1/docs/ApplicationConfigDatabaseAttributes.mdpkg/client/jcapi1/docs/ApplicationLogo.mdpkg/client/jcapi1/docs/ApplicationTemplatesApi.mdpkg/client/jcapi1/docs/ApplicationsApi.mdpkg/client/jcapi1/docs/Applicationslist.mdpkg/client/jcapi1/docs/Applicationtemplate.mdpkg/client/jcapi1/docs/ApplicationtemplateJit.mdpkg/client/jcapi1/docs/ApplicationtemplateLogo.mdpkg/client/jcapi1/docs/ApplicationtemplateOidc.mdpkg/client/jcapi1/docs/ApplicationtemplateProvision.mdpkg/client/jcapi1/docs/Applicationtemplateslist.mdpkg/client/jcapi1/docs/Command.mdpkg/client/jcapi1/docs/CommandResultsApi.mdpkg/client/jcapi1/docs/CommandTriggersApi.mdpkg/client/jcapi1/docs/Commandfilereturn.mdpkg/client/jcapi1/docs/CommandfilereturnResultsInner.mdpkg/client/jcapi1/docs/Commandresult.mdpkg/client/jcapi1/docs/CommandresultResponse.mdpkg/client/jcapi1/docs/CommandresultResponseData.mdpkg/client/jcapi1/docs/Commandresultslist.mdpkg/client/jcapi1/docs/CommandresultslistResultsInner.mdpkg/client/jcapi1/docs/CommandsApi.mdpkg/client/jcapi1/docs/Commandslist.mdpkg/client/jcapi1/docs/CommandslistResultsInner.mdpkg/client/jcapi1/docs/Error.mdpkg/client/jcapi1/docs/ErrorDetails.mdpkg/client/jcapi1/docs/ErrorDetailsAllOf.mdpkg/client/jcapi1/docs/Fde.mdpkg/client/jcapi1/docs/ManagedServiceProviderApi.mdpkg/client/jcapi1/docs/Mfa.mdpkg/client/jcapi1/docs/MfaEnrollment.mdpkg/client/jcapi1/docs/MfaEnrollmentStatus.mdpkg/client/jcapi1/docs/Organization.mdpkg/client/jcapi1/docs/OrganizationPutRequest.mdpkg/client/jcapi1/docs/Organizationentitlement.mdpkg/client/jcapi1/docs/OrganizationentitlementEntitlementProductsInner.mdpkg/client/jcapi1/docs/OrganizationsApi.mdpkg/client/jcapi1/docs/Organizationsettings.mdpkg/client/jcapi1/docs/OrganizationsettingsFeatures.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsights.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesDirectoryInsightsPremium.mdpkg/client/jcapi1/docs/OrganizationsettingsFeaturesSystemInsights.mdpkg/client/jcapi1/docs/OrganizationsettingsNewSystemUserStateDefaults.mdpkg/client/jcapi1/docs/OrganizationsettingsPasswordPolicy.mdpkg/client/jcapi1/docs/OrganizationsettingsUserPortal.mdpkg/client/jcapi1/docs/OrganizationsettingsWindowsMDM.mdpkg/client/jcapi1/docs/Organizationsettingsput.mdpkg/client/jcapi1/docs/OrganizationsettingsputNewSystemUserStateDefaults.mdpkg/client/jcapi1/docs/OrganizationsettingsputPasswordPolicy.mdpkg/client/jcapi1/docs/Organizationslist.mdpkg/client/jcapi1/docs/OrganizationslistResultsInner.mdpkg/client/jcapi1/docs/RadiusServersApi.mdpkg/client/jcapi1/docs/RadiusServersPutRequest.mdpkg/client/jcapi1/docs/Radiusserver.mdpkg/client/jcapi1/docs/Radiusserverpost.mdpkg/client/jcapi1/docs/Radiusserverput.mdpkg/client/jcapi1/docs/Radiusserverslist.mdpkg/client/jcapi1/docs/Search.mdpkg/client/jcapi1/docs/SearchApi.mdpkg/client/jcapi1/docs/Sshkeylist.mdpkg/client/jcapi1/docs/Sshkeypost.mdpkg/client/jcapi1/docs/Sso.mdpkg/client/jcapi1/docs/System.mdpkg/client/jcapi1/docs/SystemBuiltInCommandsInner.mdpkg/client/jcapi1/docs/SystemDomainInfo.mdpkg/client/jcapi1/docs/SystemMdm.mdpkg/client/jcapi1/docs/SystemMdmInternal.mdpkg/client/jcapi1/docs/SystemNetworkInterfacesInner.mdpkg/client/jcapi1/docs/SystemOsVersionDetail.mdpkg/client/jcapi1/docs/SystemProvisionMetadata.mdpkg/client/jcapi1/docs/SystemProvisionMetadataProvisioner.mdpkg/client/jcapi1/docs/SystemServiceAccountState.mdpkg/client/jcapi1/docs/SystemSshdParamsInner.mdpkg/client/jcapi1/docs/SystemSystemInsights.mdpkg/client/jcapi1/docs/SystemUserMetricsInner.mdpkg/client/jcapi1/docs/Systemput.mdpkg/client/jcapi1/docs/SystemputAgentBoundMessagesInner.mdpkg/client/jcapi1/docs/SystemsApi.mdpkg/client/jcapi1/docs/Systemslist.mdpkg/client/jcapi1/docs/Systemuserput.mdpkg/client/jcapi1/docs/SystemuserputAddressesInner.mdpkg/client/jcapi1/docs/SystemuserputAttributesInner.mdpkg/client/jcapi1/docs/SystemuserputPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserputRelationshipsInner.mdpkg/client/jcapi1/docs/Systemuserputpost.mdpkg/client/jcapi1/docs/SystemuserputpostAddressesInner.mdpkg/client/jcapi1/docs/SystemuserputpostPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserputpostRecoveryEmail.mdpkg/client/jcapi1/docs/Systemuserreturn.mdpkg/client/jcapi1/docs/SystemuserreturnAddressesInner.mdpkg/client/jcapi1/docs/SystemuserreturnPhoneNumbersInner.mdpkg/client/jcapi1/docs/SystemuserreturnRecoveryEmail.mdpkg/client/jcapi1/docs/SystemusersApi.mdpkg/client/jcapi1/docs/SystemusersResetmfaRequest.mdpkg/client/jcapi1/docs/SystemusersStateActivateRequest.mdpkg/client/jcapi1/docs/Systemuserslist.mdpkg/client/jcapi1/docs/Triggerreturn.mdpkg/client/jcapi1/docs/TrustedappConfigGet.mdpkg/client/jcapi1/docs/TrustedappConfigGetTrustedAppsInner.mdpkg/client/jcapi1/docs/TrustedappConfigPut.mdpkg/client/jcapi1/docs/Userput.mdpkg/client/jcapi1/docs/Userreturn.mdpkg/client/jcapi1/docs/UserreturnGrowthData.mdpkg/client/jcapi1/docs/UsersApi.mdpkg/client/jcapi1/git_push.shpkg/client/jcapi1/model_application.gopkg/client/jcapi1/model_application_config.gopkg/client/jcapi1/model_application_config_acs_url.gopkg/client/jcapi1/model_application_config_acs_url_tooltip.gopkg/client/jcapi1/model_application_config_acs_url_tooltip_variables.gopkg/client/jcapi1/model_application_config_constant_attributes.gopkg/client/jcapi1/model_application_config_constant_attributes_value_inner.gopkg/client/jcapi1/model_application_config_database_attributes.gopkg/client/jcapi1/model_application_logo.gopkg/client/jcapi1/model_applicationslist.gopkg/client/jcapi1/model_applicationtemplate.gopkg/client/jcapi1/model_applicationtemplate_jit.gopkg/client/jcapi1/model_applicationtemplate_logo.gopkg/client/jcapi1/model_applicationtemplate_oidc.gopkg/client/jcapi1/model_applicationtemplate_provision.gopkg/client/jcapi1/model_applicationtemplateslist.gopkg/client/jcapi1/model_command.gopkg/client/jcapi1/model_commandfilereturn.gopkg/client/jcapi1/model_commandfilereturn_results_inner.gopkg/client/jcapi1/model_commandresult.gopkg/client/jcapi1/model_commandresult_response.gopkg/client/jcapi1/model_commandresult_response_data.gopkg/client/jcapi1/model_commandresultslist.gopkg/client/jcapi1/model_commandresultslist_results_inner.gopkg/client/jcapi1/model_commandslist.gopkg/client/jcapi1/model_commandslist_results_inner.gopkg/client/jcapi1/model_error.gopkg/client/jcapi1/model_error_details.gopkg/client/jcapi1/model_error_details_all_of.gopkg/client/jcapi1/model_fde.gopkg/client/jcapi1/model_mfa.gopkg/client/jcapi1/model_mfa_enrollment.gopkg/client/jcapi1/model_mfa_enrollment_status.gopkg/client/jcapi1/model_organization.gopkg/client/jcapi1/model_organization_put_request.gopkg/client/jcapi1/model_organizationentitlement.gopkg/client/jcapi1/model_organizationentitlement_entitlement_products_inner.gopkg/client/jcapi1/model_organizationsettings.gopkg/client/jcapi1/model_organizationsettings_features.gopkg/client/jcapi1/model_organizationsettings_features_directory_insights.gopkg/client/jcapi1/model_organizationsettings_features_directory_insights_premium.gopkg/client/jcapi1/model_organizationsettings_features_system_insights.gopkg/client/jcapi1/model_organizationsettings_new_system_user_state_defaults.gopkg/client/jcapi1/model_organizationsettings_password_policy.gopkg/client/jcapi1/model_organizationsettings_user_portal.gopkg/client/jcapi1/model_organizationsettings_windows_mdm.gopkg/client/jcapi1/model_organizationsettingsput.gopkg/client/jcapi1/model_organizationsettingsput_new_system_user_state_defaults.gopkg/client/jcapi1/model_organizationsettingsput_password_policy.gopkg/client/jcapi1/model_organizationslist.gopkg/client/jcapi1/model_organizationslist_results_inner.gopkg/client/jcapi1/model_radius_servers_put_request.gopkg/client/jcapi1/model_radiusserver.gopkg/client/jcapi1/model_radiusserverpost.gopkg/client/jcapi1/model_radiusserverput.gopkg/client/jcapi1/model_radiusserverslist.gopkg/client/jcapi1/model_search.gopkg/client/jcapi1/model_sshkeylist.gopkg/client/jcapi1/model_sshkeypost.gopkg/client/jcapi1/model_sso.gopkg/client/jcapi1/model_system.gopkg/client/jcapi1/model_system_built_in_commands_inner.gopkg/client/jcapi1/model_system_domain_info.gopkg/client/jcapi1/model_system_mdm.gopkg/client/jcapi1/model_system_mdm_internal.gopkg/client/jcapi1/model_system_network_interfaces_inner.gopkg/client/jcapi1/model_system_os_version_detail.gopkg/client/jcapi1/model_system_provision_metadata.gopkg/client/jcapi1/model_system_provision_metadata_provisioner.gopkg/client/jcapi1/model_system_service_account_state.gopkg/client/jcapi1/model_system_sshd_params_inner.gopkg/client/jcapi1/model_system_system_insights.gopkg/client/jcapi1/model_system_user_metrics_inner.gopkg/client/jcapi1/model_systemput.gopkg/client/jcapi1/model_systemput_agent_bound_messages_inner.gopkg/client/jcapi1/model_systemslist.gopkg/client/jcapi1/model_systemuserput.gopkg/client/jcapi1/model_systemuserput_addresses_inner.gopkg/client/jcapi1/model_systemuserput_attributes_inner.gopkg/client/jcapi1/model_systemuserput_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserput_relationships_inner.gopkg/client/jcapi1/model_systemuserputpost.gopkg/client/jcapi1/model_systemuserputpost_addresses_inner.gopkg/client/jcapi1/model_systemuserputpost_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserputpost_recovery_email.gopkg/client/jcapi1/model_systemuserreturn.gopkg/client/jcapi1/model_systemuserreturn_addresses_inner.gopkg/client/jcapi1/model_systemuserreturn_phone_numbers_inner.gopkg/client/jcapi1/model_systemuserreturn_recovery_email.gopkg/client/jcapi1/model_systemusers_resetmfa_request.gopkg/client/jcapi1/model_systemusers_state_activate_request.gopkg/client/jcapi1/model_systemuserslist.gopkg/client/jcapi1/model_triggerreturn.gopkg/client/jcapi1/model_trustedapp_config_get.gopkg/client/jcapi1/model_trustedapp_config_get_trusted_apps_inner.gopkg/client/jcapi1/model_trustedapp_config_put.gopkg/client/jcapi1/model_userput.gopkg/client/jcapi1/model_userreturn.gopkg/client/jcapi1/model_userreturn_growth_data.gopkg/client/jcapi1/response.gopkg/client/jcapi1/test/api_application_templates_test.gopkg/client/jcapi1/test/api_applications_test.gopkg/client/jcapi1/test/api_command_results_test.gopkg/client/jcapi1/test/api_command_triggers_test.gopkg/client/jcapi1/test/api_commands_test.gopkg/client/jcapi1/test/api_managed_service_provider_test.gopkg/client/jcapi1/test/api_organizations_test.gopkg/client/jcapi1/test/api_radius_servers_test.gopkg/client/jcapi1/test/api_search_test.gopkg/client/jcapi1/test/api_systems_test.gopkg/client/jcapi1/test/api_systemusers_test.gopkg/client/jcapi1/test/api_users_test.gopkg/client/jcapi1/utils.gopkg/client/jcapi2/.gitignorepkg/client/jcapi2/.openapi-generator-ignorepkg/client/jcapi2/.openapi-generator/FILESpkg/client/jcapi2/.openapi-generator/VERSIONpkg/client/jcapi2/.travis.ymlpkg/client/jcapi2/README.mdpkg/client/jcapi2/api/openapi.yamlpkg/client/jcapi2/api_active_directory.gopkg/client/jcapi2/api_administrators.gopkg/client/jcapi2/api_apple_mdm.gopkg/client/jcapi2/api_applications.gopkg/client/jcapi2/api_authentication_policies.gopkg/client/jcapi2/api_bulk_job_requests.gopkg/client/jcapi2/api_cloud_insights.gopkg/client/jcapi2/api_command_results.gopkg/client/jcapi2/api_commands.gopkg/client/jcapi2/api_custom_emails.gopkg/client/jcapi2/api_directories.gopkg/client/jcapi2/api_duo.gopkg/client/jcapi2/api_fde.gopkg/client/jcapi2/api_feature_trials.gopkg/client/jcapi2/api_g_suite.gopkg/client/jcapi2/api_g_suite_import.gopkg/client/jcapi2/api_google_emm.gopkg/client/jcapi2/api_graph.gopkg/client/jcapi2/api_groups.gopkg/client/jcapi2/api_image.gopkg/client/jcapi2/api_ip_lists.gopkg/client/jcapi2/api_ldap_servers.gopkg/client/jcapi2/api_logos.gopkg/client/jcapi2/api_managed_service_provider.gopkg/client/jcapi2/api_office365.gopkg/client/jcapi2/api_office365_import.gopkg/client/jcapi2/api_organizations.gopkg/client/jcapi2/api_policies.gopkg/client/jcapi2/api_policy_group_associations.gopkg/client/jcapi2/api_policy_group_members_membership.gopkg/client/jcapi2/api_policy_group_templates.gopkg/client/jcapi2/api_policy_groups.gopkg/client/jcapi2/api_policytemplates.gopkg/client/jcapi2/api_providers.gopkg/client/jcapi2/api_radius_servers.gopkg/client/jcapi2/api_samba_domains.gopkg/client/jcapi2/api_scim_import.gopkg/client/jcapi2/api_software_apps.gopkg/client/jcapi2/api_subscriptions.gopkg/client/jcapi2/api_system_group_associations.gopkg/client/jcapi2/api_system_group_members_membership.gopkg/client/jcapi2/api_system_groups.go
✅ Files skipped from review due to trivial changes (6)
- pkg/client/jcapi1/docs/OrganizationsApi.md
- pkg/client/jcapi1/test/api_commands_test.go
- pkg/client/jcapi1/docs/ManagedServiceProviderApi.md
- pkg/client/jcapi1/docs/CommandResultsApi.md
- pkg/client/jcapi1/test/api_systems_test.go
- pkg/client/jcapi1/test/api_systemusers_test.go
🧰 Additional context used
🪛 GitHub Actions: ci
pkg/client/jcapi1/test/api_application_templates_test.go
[error] 17-17: go test failed: 17:38: expected ';', found batbaton
🪛 GitHub Check: go-lint
pkg/client/jcapi1/test/api_applications_test.go
[failure] 17-17:
expected ';', found baton (typecheck)
pkg/client/jcapi1/test/api_application_templates_test.go
[failure] 17-17:
expected ';', found batbaton (typecheck)
pkg/client/jcapi1/test/api_command_results_test.go
[failure] 17-17:
expected ';', found batbaton (typecheck)
🔇 Additional comments (27)
pkg/client/jcapi1/README.md (1)
292-294: LGTM on the updated import path.
Keeps the example aligned with the new module location.pkg/client/helpers.go (2)
46-144: Error wrapping flow looks solid.
Good coverage of context, URL, and HTTP status cases.
146-152: Pagination token helper is clear.
Straightforward and consistent with skip-based pagination.pkg/client/extension/extension.go (2)
30-73: Request construction and JSON decoding look good.
Clean request options and response unmarshaling.
82-110: User get request flow looks good.
Consistent headers and response handling.pkg/client/client.go (5)
73-125: Consistent nil-guarding and error wrapping.
These list/get flows are clean and consistent.
150-196: System user and group listing flows look good.
Consistent error wrapping and pagination patterns.
198-228: Group member add/remove operations look good.
Clean request composition and error handling.
230-265: Application listing and associations flow looks good.
Pagination and error handling are consistent.
20-22: Verify request logging won't expose API keys.
Logging is enabled on the shared client and requests includex-api-key; please confirm headers are redacted or disable/sanitize logging in prod.pkg/client/jcapi2/README.md (1)
318-322: LGTM on the updated import path.
Keeps the example aligned with the new module location.pkg/client/jcapi1/test/api_organizations_test.go (1)
12-19: LGTM!The import path update to
pkg/client/jcapi1correctly reflects the module relocation, and the import block formatting is consistent. This is auto-generated code from OpenAPI Generator, and the changes align with the broader refactoring effort.pkg/client/jcapi1/test/api_users_test.go (1)
12-19: LGTM!Import path update and formatting are consistent with the other test files in this PR.
pkg/client/jcapi1/docs/ApplicationTemplatesApi.md (1)
29-29: LGTM!Documentation import path correctly updated to reflect the new module location at
pkg/client/jcapi1.pkg/client/jcapi1/docs/SearchApi.md (1)
32-32: LGTM!All five code examples in this documentation file have been consistently updated to reference the new module path
pkg/client/jcapi1.pkg/client/jcapi1/docs/CommandTriggersApi.md (1)
28-28: LGTM!Documentation import path update is consistent with the broader module relocation effort.
pkg/client/jcapi1/test/api_search_test.go (1)
12-19: LGTM!Import path correctly updated to the new
pkg/client/jcapi1location. The formatting adjustments are consistent with the module reorganization.pkg/client/jcapi1/docs/SystemusersApi.md (1)
36-41: LGTM!Import paths consistently updated across all code examples in the documentation to reflect the new
pkg/client/jcapi1module location.pkg/client/jcapi1/docs/UsersApi.md (1)
26-31: LGTM!Import paths correctly updated to
pkg/client/jcapi1across all code examples.pkg/client/jcapi1/test/api_managed_service_provider_test.go (1)
12-19: LGTM!Import path correctly migrated to
pkg/client/jcapi1. Formatting adjustments are consistent with the other test files in this PR.pkg/client/jcapi1/docs/ApplicationsApi.md (1)
28-33: LGTM!Import paths consistently updated to
pkg/client/jcapi1across all code examples in this API documentation.pkg/client/jcapi1/docs/SystemsApi.md (1)
35-35: Import path update looks consistent.All examples now point to the relocated client package, which matches the repo-wide migration.
Also applies to: 105-105, 175-175, 245-245, 315-315, 391-391, 471-471, 549-549
pkg/client/jcapi1/test/api_command_triggers_test.go (1)
12-18: Imports and skip formatting look good.The test now references the updated client path, and skip-comment spacing is consistent.
Also applies to: 28-28
pkg/client/jcapi1/test/api_radius_servers_test.go (1)
12-18: Import path migration is consistent.Skip-comment formatting adjustments are clean and keep behavior unchanged.
Also applies to: 28-28, 42-42, 56-56, 68-68, 80-80
pkg/client/jcapi1/test/api_applications_test.go (1)
28-28: Skip-comment spacing change is fine.No behavior change; formatting is consistent with other tests.
Also applies to: 42-42, 56-56, 68-68, 80-80
pkg/client/jcapi1/docs/RadiusServersApi.md (1)
32-32: Import path update looks consistent.Doc examples now align with the relocated client package.
Also applies to: 104-104, 176-176, 252-252, 320-320
pkg/client/jcapi1/docs/CommandsApi.md (1)
34-34: Import path updates look consistent.Doc snippets now point to the relocated client path; no issues spotted.
Also applies to: 112-112, 184-184, 258-258, 332-332, 408-408, 476-476
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
luisina-santos
force-pushed
the
mateoHernandez123/add-wrap-errors-with-grpc
branch
from
b5a0088 to
459de5b
Compare
luisina-santos
deleted the
mateoHernandez123/add-wrap-errors-with-grpc
branch
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.