[BB-1542] Add license entitlement provisioning #16
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
agustin-conductor
left a comment
agustin-conductor
left a comment
There was a problem hiding this comment.
please address luisina's comment https://github.com/ConductorOne/baton-tableau/pull/16/files#r2372060918 using slugs can lead to missing grants if the slug is edited in C1
… with entitlement.Id parsing
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds programmatic grant/revoke for Tableau site roles and licenses by implementing Grant/Revoke on license and site connector types, a helper to parse entitlement IDs, and a client method (plus URL helper) to update a user’s siteRole via the Tableau API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Connector as Connector (license/site)
participant Client as Tableau Client
participant API as Tableau REST API
Caller->>Connector: Grant(resource, entitlement)
Note over Connector: Parse entitlement → resolve role/license
Connector->>Client: UpdateUserSiteRole(userId, siteRole)
Client->>API: PUT /sites/{siteId}/users/{userId} { "user": { "siteRole": siteRole } }
API-->>Client: 200 OK / error
Client-->>Connector: nil / error
Connector-->>Caller: annotations / error
sequenceDiagram
autonumber
actor Caller
participant Connector as Connector (license/site)
participant Client as Tableau Client
participant API as Tableau REST API
Caller->>Connector: Revoke(grant)
Note over Connector: Set siteRole to "Unlicensed"
Connector->>Client: UpdateUserSiteRole(userId, "Unlicensed")
Client->>API: PUT /sites/{siteId}/users/{userId} { "user": { "siteRole": "Unlicensed" } }
API-->>Client: 200 OK / error
Client-->>Connector: nil / error
Connector-->>Caller: annotations / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/tableau/client.go (1)
388-391: Critical: fix JSON Decode target — pass res (not &res)json.Decoder must write into the concrete pointer held in res; decoding into &res decodes into a local interface and discards the result, breaking all non-DELETE responses.
Locations:
- pkg/tableau/client.go:102
- pkg/tableau/client.go:389
- if err := json.NewDecoder(resp.Body).Decode(&res); err != nil { + if err := json.NewDecoder(resp.Body).Decode(res); err != nil { return err }
🧹 Nitpick comments (1)
pkg/connector/site.go (1)
172-178: Make entitlement parsing more robust (use last segment).IDs with additional colons would fail len(parts) == 3. Use the last segment.
Apply this diff:
func parseRoleFromEntitlementID(entitlementID string) (string, error) { - parts := strings.Split(entitlementID, ":") - if len(parts) != 3 { + parts := strings.Split(entitlementID, ":") + if len(parts) < 3 { return "", fmt.Errorf("invalid entitlement ID: %s", entitlementID) } - return parts[2], nil + return parts[len(parts)-1], nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/connector/license.go(2 hunks)pkg/connector/site.go(2 hunks)pkg/tableau/client.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/connector/site.go (1)
pkg/tableau/client.go (1)
Client(22-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
pkg/connector/license.go (4)
20-25: Fix for license-case mapping is good.Mapping lowercased resource IDs to Tableau’s cased role constants addresses the earlier case-sensitivity bug.
27-27: Roles-per-license set looks reasonable.Creator includes SiteAdministratorCreator; Explorer includes SiteAdministratorExplorer and ReadOnly. If this reflects Tableau semantics, LGTM.
Please confirm Explorer legitimately includes SiteAdministrator (without suffix) per Tableau docs.
114-128: Return rate limit annotations (carry forward prior feedback).You’re creating outputAnnotations but not populating rate limit info. Please add outputAnnotations.WithRateLimiting(...) once the client exposes rate-limit data from doRequest.
Would you like a small patch to plumb rate-limit headers from client.doRequest and attach them here?
130-140: Same: add rate limit annotations on revoke.Mirror Grant: attach rate-limit details to outputAnnotations.
pkg/connector/site.go (3)
6-6: strings import is appropriate for entitlement parsing.
127-152: Attach annotations (incl. rate limit) and return them.Currently returns nil annotations. Prefer returning an annotations object and, when available, include rate-limit data.
Apply this diff:
func (o *siteResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { + outputAnnotations := annotations.New() roleName, err := parseRoleFromEntitlementID(entitlement.Id) if err != nil { - return nil, fmt.Errorf("failed to parse role from entitlement ID: %w", err) + return outputAnnotations, fmt.Errorf("failed to parse role from entitlement ID: %w", err) } principalID := principal.Id.Resource var apiRoleName string for key, value := range roles { if value == roleName { apiRoleName = key break } } if apiRoleName == "" { - return nil, fmt.Errorf("unknown role: %s", roleName) + return outputAnnotations, fmt.Errorf("unknown role: %s", roleName) } err = o.client.UpdateUserSiteRole(ctx, principalID, apiRoleName) if err != nil { - return nil, fmt.Errorf("failed to grant %s role to user %s: %w", roleName, principalID, err) + return outputAnnotations, fmt.Errorf("failed to grant %s role to user %s: %w", roleName, principalID, err) } - return nil, nil + // TODO: outputAnnotations.WithRateLimiting(rateLimitData) once available. + return outputAnnotations, nil }
154-163: Return annotations on revoke (and include rate limit when available).Apply this diff:
func (o *siteResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { + outputAnnotations := annotations.New() principalID := grant.Principal.Id.Resource - err := o.client.UpdateUserSiteRole(ctx, principalID, unlicensed) + err := o.client.UpdateUserSiteRole(ctx, principalID, unlicensed) if err != nil { - return nil, fmt.Errorf("failed to revoke site role from user %s: %w", principalID, err) + return outputAnnotations, fmt.Errorf("failed to revoke site role from user %s: %w", principalID, err) } - return nil, nil + // TODO: outputAnnotations.WithRateLimiting(rateLimitData) once available. + return outputAnnotations, nil }pkg/tableau/client.go (1)
430-451: Approved — PUT to /sites/{siteId}/users/{userId} with {"user":{"siteRole":""}} is correctTableau REST API expects PUT /api/{api-version}/sites/{site-id}/users/{user-id} with JSON body {"user":{"siteRole":""}}; returns 200 and the updated user.
Summary by CodeRabbit
New Features
Bug Fixes