Conversation
Add support for managing OpenAI Project Groups via Terraform. Groups are collections of users synced from identity providers via SCIM. New resources: - openai_project_group: Manage group access to projects New data sources: - openai_project_group: Look up a specific group by ID or name - openai_project_groups: List all groups in a project The implementation follows the existing project_user pattern and uses the OpenAI Admin API for all operations. https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive group and project-group functionality to the provider: new resources (group, group_user, project_group), multiple data sources (single and list variants, project roles, project group roles), API types, provider registrations, pagination-aware API interactions, examples, and generated documentation; also removes an older research doc. Changes
Sequence Diagram(s)sequenceDiagram
participant TF as Terraform
participant Prov as OpenAI Provider
participant API as OpenAI Admin API
rect rgba(100, 150, 200, 0.5)
note over TF,API: Create project group assignment
TF->>Prov: Create ProjectGroup(project_id, group_id, role_id)
Prov->>API: POST /v1/organization/projects/{project_id}/groups {group_id, role}
API-->>Prov: 201 Created (assignment)
Prov->>TF: Return state (id, group_id, role_id, created_at)
end
rect rgba(150, 100, 200, 0.5)
note over TF,API: Read/List with pagination & matching
TF->>Prov: Read DataSource(project_id [, group_id | group_name])
Prov->>API: GET /v1/organization/projects/{project_id}/groups?limit=100[&after=...]
loop paginate
API-->>Prov: batch of groups (+pagination token)
Prov->>Prov: filter for match OR aggregate lists
end
Prov->>TF: Return computed attributes (groups, ids, counts, owner/member lists)
end
rect rgba(200, 150, 100, 0.5)
note over TF,API: Manage group users
TF->>Prov: Create GroupUser(group_id, user_id)
Prov->>API: POST /v1/organization/groups/{group_id}/users {user_id}
API-->>Prov: 201 Created
Prov->>API: GET /v1/organization/groups/{group_id}/users?limit=100 (populate details)
API-->>Prov: user list
Prov->>TF: Return state (id="group:user", user_name, email, role, added_at)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 11
🤖 Fix all issues with AI agents
In `@examples/resources/openai_project_group/variables.tf`:
- Around line 1-13: The openai_project resources are using hardcoded names
instead of the variables; update each resource "openai_project" (e.g., the dev
and prod project resources) to set name = var.dev_project_name for the
development resource and name = var.prod_project_name for the production
resource so the examples use the defined variables dev_project_name and
prod_project_name rather than literal strings.
In `@internal/provider/data_source_openai_project_group.go`:
- Line 365: The call to url.Parse(reqURL) currently ignores its error; change it
to capture and handle the error (e.g. parsedURL, err := url.Parse(reqURL)) and
return or log an appropriate error if err != nil to avoid nil-pointer usage
later; update any code using parsedURL afterwards to assume a non-nil parsedURL
only after the error check (referencing parsedURL and reqURL in the same block
where url.Parse is called).
- Line 139: The call to url.Parse currently ignores the error (parsedURL, _ :=
url.Parse(reqURL)); replace the blank identifier with an error variable, check
if err != nil and handle it appropriately (e.g., return the error upstream or
log and return a wrapped fmt.Errorf) to avoid using a nil parsedURL; update the
surrounding function (referencing parsedURL and reqURL) to propagate or handle
the parse failure consistently with the function's error-handling pattern.
- Around line 138-198: The pagination loop that searches for a project group
(the for foundGroup == nil { ... } block) currently uses defer
httpResp.Body.Close() inside the loop which leaks open bodies across iterations;
replace the deferred close with an explicit httpResp.Body.Close() call
immediately after you finish using the response (after decoding listResp and
after any error handling) so each page's body is closed before the next request;
ensure you still handle cases where httpResp or httpResp.Body may be nil and
that any early returns close the body when appropriate.
- Around line 364-424: The loop in data_source_openai_project_group.go defers
httpResp.Body.Close() inside the for loop which accumulates deferred calls;
instead, remove the defer and explicitly close httpResp.Body after you're done
with the response for each iteration (i.e., after checking StatusCode and after
json.NewDecoder(httpResp.Body).Decode(&listResp) and processing listResp.Data).
Also ensure you call httpResp.Body.Close() before any early return paths that
occur after httpClient.Do succeeded (for example before returning on non-200
status or decode errors) so the response body is always closed per iteration;
look for the variables/http calls named httpClient.Do, httpResp,
json.NewDecoder, listResp, cursor and the pagination loop to apply the changes.
In `@internal/provider/resource_openai_project_group.go`:
- Around line 150-160: The code silently ignores errors from io.ReadAll when
reading apiResp.Body; change the logic in the function handling the API response
(where apiResp, resp.Diagnostics, and ProjectGroupResponseFramework are used) to
read the body once into respBodyBytes and capture the error (respBodyBytes, err
:= io.ReadAll(apiResp.Body)); if err != nil call resp.Diagnostics.AddError with
a clear message including err.Error() and return; then use respBodyBytes for
both the API error branch (apiResp.Status) and JSON unmarshal (json.Unmarshal
into ProjectGroupResponseFramework), removing the second unchecked io.ReadAll.
- Line 116: The call to json.Marshal(reqMap) currently ignores its error; update
the code around reqBody, json.Marshal(reqMap) to capture the returned (reqBody,
err) and handle err (log and return the error or wrap it and propagate) so
marshaling failures aren't silenced; adjust subsequent code that uses reqBody
accordingly (in the function that builds the OpenAI project/group request in
resource_openai_project_group.go).
- Around line 142-146: Replace use of http.DefaultClient.Do(apiReq) with a
dedicated http.Client that sets a reasonable Timeout (e.g., 10s) and call
client.Do(apiReq) instead; update the code around the apiReq/apiResp handling
and ensure the "time" package is imported. Specifically, where apiResp, err :=
http.DefaultClient.Do(apiReq) appears in resource_openai_project_group.go,
create an http.Client variable with a Timeout and use that client's Do method so
the request cannot hang indefinitely.
- Line 202: The call to url.Parse(baseURL) ignores its error; change the
parsedURL, _ := url.Parse(baseURL) to capture the error (e.g. parsedURL, err :=
url.Parse(baseURL)) and handle it appropriately in the surrounding function
(return the error or wrap/log it) so that invalid baseURL values are detected;
update any callers or return signatures if needed to propagate the error from
this function where parsedURL is used (reference parsedURL, url.Parse, baseURL).
- Around line 285-322: The Delete method on ProjectGroupResource currently
swallows errors causing state drift; update Delete to validate the split ID
(from ProjectGroupResourceModel.ID) and append a diagnostic error to
resp.Diagnostics when the format is invalid instead of returning silently;
handle errors from http.NewRequest by appending a diagnostic and returning;
perform the HTTP call via http.DefaultClient.Do(apiReq), check for errors and
non-2xx status codes, read/close the response body, and append a detailed
diagnostic including status and body on failure so Terraform does not silently
remove state (use symbols Delete, ProjectGroupResource,
ProjectGroupResourceModel, apiReq, reqURL, apiKey, resp.Diagnostics).
- Around line 201-259: Defer inside the pagination loop leaves HTTP bodies open;
change the logic in the loop that uses apiResp and listResp (the block that
creates apiReq, calls http.DefaultClient.Do, and decodes into
ProjectGroupListResponse) to explicitly close apiResp.Body after you've finished
reading/decoding each page (e.g., read/decode, then call apiResp.Body.Close()
immediately before breaking/continuing), instead of using defer
apiResp.Body.Close(); ensure you still close the body on all error-return paths
and before each loop iteration advances the cursor or exits so no response
bodies accumulate.
🧹 Nitpick comments (4)
examples/data-sources/openai_project_group/variables.tf (1)
1-13: Variables are not referenced in the data-source.tf example.These variables define
project_idandgroup_id, but the correspondingdata-source.tfin this PR uses hardcoded values ("proj-abc123","group-xyz789") instead ofvar.project_idandvar.group_id. Consider updatingdata-source.tfto use these variables for consistency and to demonstrate their purpose.docs/data-sources/project_group.md (1)
56-60: Consider addinggroup_idandgroup_nameto Read-Only outputs.When looking up by
group_name, thegroup_idbecomes a useful output. Similarly, when looking up bygroup_id, thegroup_nameis populated. Consider documenting these as Read-Only outputs for completeness:### Read-Only - `id` (String) The ID of the resource (composite of project_id:group_id). +- `group_id` (String) The ID of the group (useful when looking up by name). +- `group_name` (String) The display name of the group (useful when looking up by ID). - `role` (String) The role of the group in the project (e.g., 'owner' or 'member'). - `created_at` (Number) Timestamp when the group was added to the project.internal/provider/resource_openai_project_group.go (1)
118-124: Consider extracting URL construction to a helper function.The URL construction logic is duplicated in
Create,Read, andDelete. Extracting it to a helper would improve maintainability and reduce the risk of inconsistencies.Example helper function
func (r *ProjectGroupResource) buildAPIURL(path string) string { apiURL := r.client.OpenAIClient.APIURL if strings.Contains(apiURL, "/v1") { return strings.TrimSuffix(apiURL, "/v1") + "/v1" + path } return strings.TrimSuffix(apiURL, "/") + "/v1" + path }Then use as:
reqURL := r.buildAPIURL("/organization/projects/" + data.ProjectID.ValueString() + "/groups")internal/provider/data_source_openai_project_group.go (1)
223-229: Add interface assertion for consistency.The singular data source has an interface assertion (
var _ datasource.DataSource = &ProjectGroupDataSource{}), but this list data source is missing one. Adding it ensures compile-time verification.Proposed fix
+var _ datasource.DataSource = &ProjectGroupsDataSource{} + func NewProjectGroupsDataSource() datasource.DataSource { return &ProjectGroupsDataSource{} }
Add full support for managing OpenAI organization groups via Terraform:
New resources:
- openai_group: Create, update, and delete organization groups
- openai_group_user: Add and remove users from groups
New data sources:
- openai_group: Look up a single group by ID or name
- openai_groups: List all groups in the organization
- openai_group_user: Look up a user in a group by ID or email
- openai_group_users: List all users in a group
The implementation follows the OpenAI Admin API spec:
- POST/DELETE /organization/groups for group CRUD
- POST/GET/DELETE /organization/groups/{group_id}/users for user membership
All resources include documentation and examples.
https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
- Remove manually written docs that conflict with auto-generated ones - Apply terraform fmt to example files - Let tfplugindocs generate docs during CI https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/resources/group_user.md`:
- Around line 53-57: Update the docs entry for the added_at field in the
GroupUser response resource: change the description for `added_at` to explicitly
state the unit by using "Unix timestamp (in seconds) when the user was added to
the organization" so it matches other timestamp docs (e.g., `created_at`); edit
the `added_at` line in docs/resources/group_user.md accordingly, leaving other
fields (`id`, `user_name`, `email`, `role`) unchanged.
In `@examples/resources/openai_group_user/resource.tf`:
- Around line 1-16: The example references openai_group.engineering but never
defines it, making the snippet non-functional; add a minimal openai_group
resource definition (or replace with a data source) and update the
openai_group_user blocks to reference that defined group. Specifically, define a
simple openai_group (e.g., name "engineering") or use a data "openai_group"
lookup, and ensure the openai_group_user resources (openai_group_user.engineer,
openai_group_user.support_agent_1, openai_group_user.support_agent_2) reference
the defined group ID instead of an undefined symbol.
In `@internal/provider/data_source_openai_group_user.go`:
- Around line 137-201: The pagination loop that searches for a group user (the
for foundUser == nil loop in internal/provider/data_source_openai_group_user.go)
currently defers httpResp.Body.Close(), which leaks connections across
iterations; remove the defer and instead call httpResp.Body.Close() explicitly
after using the body (i.e., after
json.NewDecoder(httpResp.Body).Decode(&listResp) and after any early return
paths that occur after a response is obtained), and ensure you also call
httpResp.Body.Close() on non-200 responses and error branches before returning
to fully release the connection; update the same pattern in the single-user
search location if present.
In `@internal/provider/data_source_openai_group.go`:
- Around line 121-185: In ReadDataSourceOpenaiGroup (and likewise
ReadDataSourceOpenaiGroups) replace the defer httpResp.Body.Close() inside the
pagination loop with explicit httpResp.Body.Close() calls immediately after
you're done using the response body: call Close() after any error handling that
returns, right after successful json.NewDecoder(httpResp.Body).Decode(&listResp)
(or after decode error handling), and before any loop break or continue so each
iteration releases the connection; ensure every path that exits the iteration
(error, non-200 status, decode success, or early break) invokes
httpResp.Body.Close() so bodies aren’t left open across iterations.
In `@internal/provider/resource_openai_group_user.go`:
- Around line 184-233: In findUserInGroup (and similarly in Read) remove the
defer apiResp.Body.Close() inside the pagination loop and instead call
apiResp.Body.Close() explicitly at each iteration: after handling a non-200
status (before returning), after a JSON decode error (before returning), and
after successfully processing the response data but before the loop continues to
the next page; this ensures each http.Response body is released per iteration
and prevents connection leaks during pagination.
- Around line 378-392: The Delete implementation currently discards errors and
the HTTP response; capture and handle errors from both http.NewRequest and
http.DefaultClient.Do and report failures via the resource's diag collection
(resp.Diagnostics). Specifically, in the Delete method (where apiReq is created
and http.DefaultClient.Do is called) replace the silent returns with adding a
Diagnostic on request creation error, then call resp, err :=
http.DefaultClient.Do(apiReq); if err add a Diagnostic and return, defer
resp.Body.Close(), and if resp.StatusCode is not 2xx read the response body (or
include status text) and add a Diagnostic describing the failure so Terraform
state isn't removed when the remote DELETE failed. Ensure you reference apiReq,
http.DefaultClient.Do, resp and resp.Diagnostics when implementing these checks.
In `@internal/provider/resource_openai_group.go`:
- Around line 335-350: The DELETE request currently ignores both network errors
and non-success HTTP statuses; capture the response and error from
http.DefaultClient.Do(apiReq), check for err and for non-2xx status codes,
read/close the response body, and return a diagnostic error (same pattern as the
Update handler) so Terraform surfaces the failure instead of removing state;
update the delete function around apiReq, http.DefaultClient.Do(apiReq), and use
r.client.OpenAIClient.APIKey / r.client.AdminAPIKey logic as shown to build the
Authorization header when constructing the diagnostic message.
- Around line 200-216: The loop currently defers apiResp.Body.Close(), causing
open connections to pile up across pagination iterations; remove the defer and
close apiResp.Body explicitly after processing each response (i.e., after
decoding into listResp), and also ensure you call apiResp.Body.Close() before
any early returns on non-200 responses or JSON decode errors; look for the HTTP
request/response loop using variables apiReq, apiResp, apiResp.Body, and the
json.NewDecoder(...).Decode(&listResp) call and move/insert explicit
apiResp.Body.Close() calls accordingly.
Add documentation files in tfplugindocs format for: - openai_group resource - openai_group_user resource - openai_project_group resource - openai_group data source - openai_groups data source - openai_group_user data source - openai_group_users data source - openai_project_group data source - openai_project_groups data source https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
…ources - Handle json.Marshal errors in Create - Handle io.ReadAll errors with proper diagnostics - Use dedicated http.Client with 30s timeout instead of http.DefaultClient - Handle url.Parse errors in pagination loops - Fix defer in pagination loops - explicitly close response bodies - Improve Delete method to properly report errors - Use variables in resource.tf example instead of hardcoded names https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/provider/data_source_openai_project_group.go`:
- Around line 163-164: The http.Client is created without a timeout which can
hang indefinitely; update the creation of httpClient (used to Do(httpRequest))
to include a 30s timeout (e.g., set Timeout: 30 * time.Second) and add the
"time" import; ensure the modified httpClient variable is used in the existing
Do(httpRequest) call so behavior matches the resource's 30s timeout.
- Around line 394-395: The HTTP client is created without a timeout (httpClient
:= &http.Client{}) which can cause indefinite hangs; update the creation of
httpClient (the variable used where httpClient.Do(httpRequest) is called) to
include a sensible timeout, e.g. set Timeout: 30 * time.Second on the
http.Client, and add "time" to the imports so the time package is available.
- Update project_group.md example to use variables instead of hardcoded names - Remove research folder from docs (not part of tfplugindocs output) https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
Add 30 second timeout to http.Client instances in data_source_openai_project_group.go to prevent indefinite hangs on network issues. https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
- Remove role field from ProjectGroup (not returned by API)
- Add role_id as write-only field for creating group membership
- Add openai_project_roles data source to list project roles
- Add openai_project_group_roles data source for group role assignments
- Add role-related types (RoleResponseFramework, GroupRoleAssignment)
- Update examples and docs to match actual API behavior
The OpenAI API returns only object, project_id, group_id, group_name,
and created_at for ProjectGroup. Roles are managed separately via the
/projects/{project_id}/groups/{group_id}/roles endpoint.
https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docs/data-sources/project_roles.md`:
- Around line 1-69: The generated docs file docs/data-sources/project_roles.md
has formatting drift; re-run the docs generator (e.g., run `go generate ./...`
or `tfplugindocs generate`) to regenerate the file (this will update the
header/formatting for the openai_project_roles data source and the nested
`roles` schema), then stage and commit the updated docs so the CI formatting
check passes.
In `@examples/data-sources/openai_project_roles/data-source.tf`:
- Around line 1-27: Run terraform fmt on the
examples/data-sources/openai_project_roles/data-source.tf to fix formatting
issues; ensure the data block named openai_project_roles "all" and the outputs
all_role_ids, role_count, and role_details are reformatted by terraform fmt,
then commit the formatted file.
In `@internal/provider/data_source_openai_project_group_roles.go`:
- Around line 253-258: The code leaves allAssignments nil and roleIDs empty
which yields a null list in state; initialize both slices before populating to
guarantee empty lists instead of nil. Update the block that sets
data.RoleAssignments and data.RoleIDs so allAssignments is created with
make(..., 0) (and roleIDs similarly) prior to the loop that appends entries,
then assign data.RoleAssignments = allAssignments and populate data.RoleIDs
using types.StringValue(v) as before; ensure no slice stays nil so Terraform
state shows an empty list rather than null.
- Around line 161-169: The project group roles endpoint path is inconsistent:
current code in data_source_openai_project_group_roles.go builds suffix :=
fmt.Sprintf("/projects/%s/groups/%s/roles", projectID, groupID) but other
group-related code (data_source_openai_project_group.go and
resource_openai_project_group.go) uses the "/organization/projects/{id}/groups"
prefix; update the suffix to match the canonical path (e.g.
"/organization/projects/%s/groups/%s/roles") so reqURL construction (using
apiURL, strings.Contains(apiURL, "/v1"), and TrimSuffix logic) builds the same
API path as other group endpoints, and adjust any related variable names if
needed to keep consistency with projectID and groupID usage.
In `@internal/provider/data_source_openai_project_group.go`:
- Around line 89-108: The current Read method (ProjectGroupDataSource.Read)
allows both groupID and groupName to be set and can fall back from an ID
mismatch to a name match; change the logic so that if groupID
(data.GroupID.ValueString()) is non-empty you only attempt lookup by ID and
never test groupName, returning a not-found error if no match; otherwise
(groupID empty) perform the name-based search using groupName. Alternatively,
enforce schema-level exclusivity by updating ProjectGroupDataSourceModel's
schema to use ExactlyOneOf/ConflictsWith for group_id and group_name so the
caller cannot provide both—pick one of these two fixes and apply it
consistently.
- Around line 120-128: The URL construction for reqURL using
d.client.OpenAIClient.APIURL and projectID can produce duplicated "/v1" when
apiURL ends with "/v1/"; update the logic in data_source_openai_project_group.go
to robustly trim both trailing "/" and "/v1" (e.g., apply TrimSuffix("/v1") then
TrimSuffix("/")) before appending "/v1" + suffix, mirroring the safe double-trim
approach used in internal/client/client.go; also apply the same fix to the
equivalent implementation in the list data source (the block around lines
336–340) so both code paths build a correct, non-duplicated reqURL.
In `@internal/provider/data_source_openai_project_role.go`:
- Around line 155-156: The variables allRoles and roleIDs are left nil when no
roles exist, causing Terraform to see null instead of an empty list; initialize
them to empty slices (e.g., make([]ProjectRoleResultModel, 0) and make([]string,
0)) where they are declared or before any potential early return in the function
so the data source always returns empty lists instead of nulls; update
references to allRoles and roleIDs in the same function in
data_source_openai_project_role.go to use these initialized slices.
In `@internal/provider/resource_openai_project_group.go`:
- Around line 76-83: The schema's "role_id" attribute is Required and uses
stringplanmodifier.RequiresReplace() but is never populated by Read (it's
write-only), which will cause any imported resource to be flagged for
replacement; update the "role_id" definition (and the other occurrence around
the second mention at the bottom of the file) to avoid this: either make role_id
Optional and Computed so import leaves a known/unknown state instead of null, or
remove the RequiresReplace plan modifier and document the import limitation, or
switch to the framework's write-only attribute support (plugin framework v1.13+)
to mark it write-only; adjust the attribute definition where
stringplanmodifier.RequiresReplace() is set and ensure Read/ReadResource (or the
method that populates state) and the schema reflect the chosen approach.
In `@internal/provider/types_project_org.go`:
- Around line 88-95: ProjectGroupListResponse currently defines FirstID/LastID
and will fail to unmarshal the API's "next" cursor; change the struct
ProjectGroupListResponse to have HasMore bool `json:"has_more"` and Next *string
`json:"next"` (keep Object and Data as-is), remove FirstID/LastID; then update
any pagination logic that reads/writes LastID (e.g., where you check or set
LastID to drive subsequent list calls) to use the new Next pointer (treat nil as
no next page and dereference safely) so calls that previously used LastID now
use Next to pass the cursor.
🧹 Nitpick comments (10)
examples/resources/openai_project_group/resource.tf (1)
1-45: Example file foropenai_project_groupdoesn't demonstrate the resource.This is the example file for the
openai_project_groupresource, but everyopenai_project_groupblock is commented out. Users browsing docs/examples will only seeopenai_projectresources being created, which doesn't help them understand how to use the group resource.Consider showing at least one uncommented
openai_project_groupblock with clearly marked placeholder values (e.g.,"REPLACE_WITH_GROUP_ID"), so the example is useful as a template. Terraform examples don't need to be directly runnable — they need to be instructive.internal/provider/data_source_openai_project_group.go (4)
134-200: Consider extracting the shared HTTP request/pagination logic.Both
ProjectGroupDataSource.Read(lines 134–200) andProjectGroupsDataSource.Read(lines 346–405) duplicate the URL building, request construction, header setting, HTTP execution, status check, JSON decoding, and pagination cursor management. This is a significant amount of copy-paste.A shared helper like
func fetchProjectGroups(apiURL, adminKey, orgID, projectID string) ([]ProjectGroupResponseFramework, error)would eliminate the duplication and make both data sources easier to maintain.
159-159:http.Clientis re-created on every loop iteration.Move the client instantiation before the loop (applies to both Read methods at lines 159 and 370). The client is safe to reuse across requests and benefits from connection pooling.
222-230: Missing interface compliance assertion forProjectGroupsDataSource.
ProjectGroupDataSourcehasvar _ datasource.DataSource = &ProjectGroupDataSource{}(line 19), butProjectGroupsDataSourcelacks the equivalent check. Add it for consistency and to catch interface drift at compile time.+var _ datasource.DataSource = &ProjectGroupsDataSource{} + func NewProjectGroupsDataSource() datasource.DataSource {
166-170: API error message lacks response body detail.When the API returns a non-200 status, the error only reports the status line (e.g.,
"403 Forbidden"). Reading and including a snippet of the response body would significantly ease debugging — the OpenAI API typically returns a JSON error message. Same applies to the list data source at lines 377-381.Sketch
if httpResp.StatusCode != 200 { + body, _ := io.ReadAll(io.LimitReader(httpResp.Body, 1024)) httpResp.Body.Close() - resp.Diagnostics.AddError("API Error", fmt.Sprintf("Status: %s", httpResp.Status)) + resp.Diagnostics.AddError("API Error", fmt.Sprintf("Status: %s, Body: %s", httpResp.Status, string(body))) return }internal/provider/data_source_openai_project_group_roles.go (2)
189-189: Usehttp.NewRequestWithContextto propagate the Terraform context.The
ctxparameter is available but not passed to the HTTP request. If Terraform cancels the operation (e.g., timeout or user interrupt), the in-flight HTTP call won't be cancelled.♻️ Proposed fix
- httpRequest, err := http.NewRequest("GET", parsedURL.String(), nil) + httpRequest, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil)
164-169: URL construction with/v1detection is fragile and duplicated.The
strings.Contains(apiURL, "/v1")/TrimSuffixpattern is repeated across every data source and resource file. Consider extracting a shared helper (e.g.,buildAdminURL(apiURL, path string) string) to reduce duplication and the risk of inconsistent fixes.internal/provider/data_source_openai_project_role.go (2)
173-173: Usehttp.NewRequestWithContextto respect cancellation.Same issue as in
data_source_openai_project_group_roles.go— thectxfrom theReadmethod signature is not propagated to the HTTP request.♻️ Proposed fix
- httpRequest, err := http.NewRequest("GET", parsedURL.String(), nil) + httpRequest, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil)
127-243: Significant code duplication with other data sources.The
Readmethod here is nearly identical todata_source_openai_project_group_roles.go(admin key check, URL construction, HTTP client creation, pagination loop, header setup, error handling). This pattern is repeated across at least two data sources in this PR.Consider extracting a shared pagination helper, e.g.:
func (c *OpenAIClient) paginatedGet(ctx context.Context, urlPath string, out interface{}) error { ... }This would centralize timeout, auth headers, org header,
/v1normalization, and cursor-based iteration.internal/provider/resource_openai_project_group.go (1)
132-132: Usehttp.NewRequestWithContextacross Create, Read, and Delete.All three lifecycle methods create HTTP requests via
http.NewRequestwhile thectxparameter is available. Pass the context so Terraform timeouts and cancellations propagate to in-flight API calls.♻️ Proposed fix (apply to all three call sites)
// Create (line 132) - apiReq, err := http.NewRequest("POST", reqURL, bytes.NewReader(reqBody)) + apiReq, err := http.NewRequestWithContext(ctx, "POST", reqURL, bytes.NewReader(reqBody)) // Read (line 230) - apiReq, err := http.NewRequest("GET", parsedURL.String(), nil) + apiReq, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil) // Delete (line 331) - apiReq, err := http.NewRequest("DELETE", reqURL, nil) + apiReq, err := http.NewRequestWithContext(ctx, "DELETE", reqURL, nil)Also applies to: 230-230, 331-331
- Fix ProjectGroupListResponse to use Next pointer instead of FirstID/LastID - Fix URL construction with safer double-trim approach - Fix group lookup logic to only search by ID when groupID is provided - Make role_id Optional+Computed to handle import properly - Initialize slices to avoid nil (prevent null in Terraform state) - Use consistent /organization/projects path prefix for all endpoints - Update pagination to use Next pointer for cursor https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
Simplify the role data source examples to use simpler for expressions that won't cause terraform fmt formatting drift. https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
- Fix gofmt formatting issue in data_source_openai_project_group_roles.go - Add missing variables.tf for openai_project_roles example - Add missing variables.tf for openai_project_group_roles example https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/provider/resource_openai_project_group.go`:
- Around line 76-83: The role_id attribute is defined as Optional+Computed with
stringplanmodifier.UseStateForUnknown() but lacks a RequiresReplace() modifier,
so changing it triggers the resource Update path which currently unconditionally
errors (see the Update method that returns an error); either add
planmodifier.RequiresReplace() to the role_id schema attribute (the "role_id"
schema in resource_openai_project_group.go) to force destroy/recreate on
changes, or implement patch/update logic inside the resource's Update method to
handle role_id changes (update the API call and state accordingly) so Update no
longer returns the error.
🧹 Nitpick comments (6)
internal/provider/data_source_openai_project_group_roles.go (2)
188-209: Usehttp.NewRequestWithContextto propagate the Terraform context.Using
http.NewRequestignores thectxpassed by Terraform. If a user cancels the operation (e.g.,Ctrl+C), the in-flight HTTP request won't be cancelled. Additionally, the error diagnostic on non-200 responses is sparse — reading the response body would help operators debug API failures.Suggested fix
- httpRequest, err := http.NewRequest("GET", parsedURL.String(), nil) + httpRequest, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil)And for the error path:
if httpResp.StatusCode != 200 { - httpResp.Body.Close() - resp.Diagnostics.AddError("API Error", fmt.Sprintf("Status: %s", httpResp.Status)) + bodyBytes, _ := io.ReadAll(httpResp.Body) + httpResp.Body.Close() + resp.Diagnostics.AddError("API Error", fmt.Sprintf("Status: %s - %s", httpResp.Status, string(bodyBytes))) return }Note:
ioimport would be needed.
142-260: Significant code duplication withdata_source_openai_project_role.go.The
Readmethods in this file anddata_source_openai_project_role.goshare nearly identical pagination, URL construction, HTTP request setup, and error handling logic. Consider extracting a shared helper (e.g.,paginatedAdminGet[T any](ctx, client, path, httpClient) ([]T, error)) to reduce duplication and centralize bug fixes.internal/provider/data_source_openai_project_role.go (1)
172-193: Usehttp.NewRequestWithContextto respect cancellation.Same as in
data_source_openai_project_group_roles.go—http.NewRequestdoesn't propagate the Terraformctx, so cancellation signals are ignored. Also, the non-200 error path discards the response body which often contains useful error details from the API.Suggested fix
- httpRequest, err := http.NewRequest("GET", parsedURL.String(), nil) + httpRequest, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil)internal/provider/resource_openai_project_group.go (3)
132-132: Usehttp.NewRequestWithContextthroughout for cancellation support.All three HTTP request creation points (
CreateLine 132,ReadLine 230,DeleteLine 331) usehttp.NewRequestwithout propagatingctx. This means Terraform cancellation signals (Ctrl+C) won't abort in-flight requests.Suggested fix (apply to all three locations)
- apiReq, err := http.NewRequest("POST", reqURL, bytes.NewReader(reqBody)) + apiReq, err := http.NewRequestWithContext(ctx, "POST", reqURL, bytes.NewReader(reqBody))- apiReq, err := http.NewRequest("GET", parsedURL.String(), nil) + apiReq, err := http.NewRequestWithContext(ctx, "GET", parsedURL.String(), nil)- apiReq, err := http.NewRequest("DELETE", reqURL, nil) + apiReq, err := http.NewRequestWithContext(ctx, "DELETE", reqURL, nil)Also applies to: 230-230, 331-331
124-130: URL construction pattern differs from data sources and is fragile.The resource uses
strings.Contains(apiURL, "/v1")which could match/v1appearing anywhere in the URL (e.g., a hostname). The data sources use a saferTrimSuffixapproach. Consider aligning for consistency:- var reqURL string - if strings.Contains(apiURL, "/v1") { - reqURL = strings.TrimSuffix(apiURL, "/v1") + "/v1/organization/projects/" + data.ProjectID.ValueString() + "/groups" - } else { - reqURL = strings.TrimSuffix(apiURL, "/") + "/v1/organization/projects/" + data.ProjectID.ValueString() + "/groups" - } + baseURL := strings.TrimSuffix(apiURL, "/v1") + baseURL = strings.TrimSuffix(baseURL, "/") + reqURL := baseURL + "/v1/organization/projects/" + data.ProjectID.ValueString() + "/groups"Same applies to the
Read(Lines 201-206) andDelete(Lines 323-329) methods.
184-297: Read paginates through all groups to find one — consider documenting the performance implication.Since no single-group GET endpoint exists (Line 199 comment),
Readiterates all groups page by page. For projects with many groups, this will be slow during every refresh. The comment at Line 199 is good, but consider adding atflog.Warnortflog.Debugwhen pagination exceeds the first page so operators can identify performance bottlenecks.
The role_id attribute was changed from Required to Optional+Computed to support imports where the API does not return the role_id value. https://claude.ai/code/session_01DtgMTbRXemrjcVjdtfcDR1
- Replace defer in loops with explicit Body.Close() after consumption - Add 30s HTTP client timeout (was using http.DefaultClient without timeout) - Handle url.Parse errors instead of ignoring them - Handle json.Marshal and io.ReadAll errors properly - Add proper error handling in Delete methods (was silently ignoring errors) - Initialize slices with make() to avoid nil/null in Terraform state - Add missing group resource definition in group_user example - Regenerate docs
Fodoj
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#43
Summary
Comprehensive group and role management for the OpenAI Terraform provider. Groups are collections of users that can be assigned roles at the organization or project level.
New Resources (7)
openai_groupopenai_group_useropenai_project_grouprole_idsopenai_project_userrole_ids(changed from singlerole)openai_organization_roleopenai_organization_group_roleopenai_organization_user_roleNew Data Sources (10)
openai_roleopenai_rolesopenai_group_rolesopenai_group_useropenai_group_usersopenai_project_roleopenai_project_rolesopenai_project_group_rolesopenai_organization_user_rolesopenai_project_user_rolesBreaking Changes
openai_project_user:role(string) replaced withrole_ids(set of role IDs)openai_project_group:role_id(string) replaced withrole_ids(set of role IDs)Deleted (legacy/redundant)
openai_project_user_role— redundant withopenai_project_user.role_idsopenai_project_group_role— redundant withopenai_project_group.role_idsresource_openai_user_role.go— legacy SDKv2, never registereddata_source_openai_user_role.go— legacy SDKv2, never registeredType of change
How Has This Been Tested?
Checklist
go build ./...passesgo vet ./...passesgofmtclean