[Feature] Terrafron Provider for Open Cloud Datacenter API - #205
[Feature] Terrafron Provider for Open Cloud Datacenter API#205sameera-87 wants to merge 2 commits into
Conversation
|
|
📝 WalkthroughWalkthroughThis PR introduces a complete Terraform provider for the DC-API, including Go module/build setup, HTTP client methods, Terraform resources and data sources, provider wiring, asynchronous lifecycle handling, examples, tests, and extensive architecture and API documentation. ChangesDC-API Terraform Provider
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
docs/dc-api-reference.md-27-27 (1)
27-27: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken
PrivateDnsZonetable-of-contents link.Line 27 points to
#resource-privatednszoned, so Markdown renderers will not jump to thePrivateDnsZonesection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dc-api-reference.md` at line 27, The table-of-contents entry for PrivateDnsZone uses a broken anchor, so update the markdown link target to match the actual section heading slug generated for PrivateDnsZone. Fix the TOC entry itself rather than the section, and verify the anchor resolves correctly in the docs renderer.Source: Linters/SAST tools
examples/vm/main.tf-82-85 (1)
82-85: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
subnet_uuidhere as well.This output is documented as the subnet UUID, but it currently returns
dcapi_subnet.app.id. The same file already usesdcapi_subnet.app.subnet_uuidon Line 60 for the actual API identifier, so this output will otherwise expose the wrong value to downstream consumers.Suggested fix
output "subnet_id" { - value = dcapi_subnet.app.id + value = dcapi_subnet.app.subnet_uuid description = "UUID of the created Subnet." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/vm/main.tf` around lines 82 - 85, The subnet output is returning the Terraform resource ID instead of the API UUID, so update the output block named subnet_id in main.tf to use dcapi_subnet.app.subnet_uuid just like the existing subnet_uuid reference in the same configuration. Keep the description aligned with the value being exported so downstream consumers receive the actual subnet UUID rather than the provider-generated id.examples/subnet/main.tf-49-52 (1)
49-52: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOutput the subnet UUID instead of Terraform's state ID.
This block says it returns the created subnet UUID, but
dcapi_subnet.app.idis the Terraform resource ID. The provider already exposes the API UUID separately (subnet_uuid), and the VM example consumes that field for downstream wiring. Returning.idhere will hand callers the wrong identifier shape.Suggested fix
output "subnet_id" { - value = dcapi_subnet.app.id + value = dcapi_subnet.app.subnet_uuid description = "UUID of the created Subnet." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/subnet/main.tf` around lines 49 - 52, The subnet output is currently returning Terraform’s resource ID instead of the API UUID. Update the `output "subnet_id"` block in the subnet example to use the `dcapi_subnet.app.subnet_uuid` attribute, matching the provider’s UUID field and the way the VM example wires subnet values downstream. Keep the output name and description as-is, but ensure the value references the UUID-exposing symbol on `dcapi_subnet.app`.internal/resources/vnet.go-253-260 (1)
253-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe 409 guidance points users at the wrong attribute.
dcapi_subnet.vnet_idexpects the bare UUID, but this message tells users to referencedcapi_vnet.example.id, which is the compositetenant/project/uuidstate ID. Following that advice will build an invalid subnet path; the example should usedcapi_vnet.example.vnet_uuid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/vnet.go` around lines 253 - 260, The 409 delete guidance in the VNet error path is pointing users to the wrong reference attribute. Update the message in the VNet deletion logic (the strings.Contains(err.Error(), "HTTP 409") branch) so the example Terraform reference uses dcapi_vnet.example.vnet_uuid instead of dcapi_vnet.example.id, since dcapi_subnet.vnet_id requires the bare UUID and not the composite state ID.internal/resources/tenant.go-14-19 (1)
14-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
Importerwiring for these resources.schema.Resourcedoesn’t setImporterhere, soterraform importwon’t work fordcapi_tenantor the other new resources ininternal/resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/tenant.go` around lines 14 - 19, The tenant resource is missing import support, so terraform import cannot work for this and the other new resources. Update ResourceTenant and the corresponding Resource constructors in internal/resources to wire schema.Resource.Importer, using the appropriate StateContext importer helper already used by other resources. Make sure the import setup is added alongside CreateContext, ReadContext, UpdateContext, and DeleteContext so dcapi_tenant and related resources can be imported consistently.internal/resources/node_pool.go-276-281 (1)
276-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat HTTP 404 as a successful delete.
internal/client/node_pool.go:103-111bubbles any DELETE error, and Lines 276-281 return immediately on that error before the poller runs. If the pool was already removed out of band, destroy fails instead of converging on the desired absent state. NormalizeHTTP 404to success in the delete path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/node_pool.go` around lines 276 - 281, Treat HTTP 404 as a successful delete in the node pool destroy flow. Update the DeleteNodePool path used by node pool deletion so a not-found response is normalized to nil instead of bubbling an error, and make the delete handler in internal/client/node_pool.go return success when the pool is already gone. Keep the existing waitForNodePoolDeleted poller behavior in internal/resources/node_pool.go so the resource still converges on absent state when DeleteNodePool is called.
🧹 Nitpick comments (4)
docs/dc-api-reference.md (1)
419-429: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMark the VM and Cluster networking selectors as user inputs, not computed fields.
network_name,vnet_id, andsubnet_idappear in the create request bodies and must be set by configuration for the chosen networking mode. Labeling themCOMPUTED_OPTIONALflips that contract and makes these tables inconsistent with the examples above.Also applies to: 661-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/dc-api-reference.md` around lines 419 - 429, The field classification table is labeling VM and Cluster networking selectors as computed, but `network_name`, `vnet_id`, and `subnet_id` are user-provided inputs in the create request bodies. Update the Field Classification entries in the relevant documentation sections so these selectors are marked as user inputs for their respective networking modes, keeping the wording consistent with the request examples and the related Cluster table.Architecture.md (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate this architecture doc to match the resource set shipped in
0.1.0.These sections still describe only the tenant/project/vnet/subnet/vm slice, but this PR also ships Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation, NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount. The resource hierarchy is also incomplete for VPC-backed workloads. That makes the architecture doc stale on merge day.
Also applies to: 11-28, 30-46, 370-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Architecture.md` at line 3, Update the architecture documentation to reflect the full 0.1.0 resource set instead of only the tenant/project/vnet/subnet/vm slice. In the Architecture.md sections currently describing the provider scope and resource hierarchy, add Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation, NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount, and expand the VPC-backed workload hierarchy accordingly. Use the existing architecture headings and resource overview areas in Architecture.md so the doc stays aligned with the shipped provider surface.internal/client/project.go (1)
87-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftStop classifying 404s by parsing error strings.
doRequestonly returns formattederrors today, so the read paths are forced to inspect message text. Add a typed/status-bearing error and branch on that instead ofstrings.Contains(err.Error(), "HTTP 404"); otherwise a wording change or unrelated wrapped error can misclassify drift. The same pattern exists ininternal/client/vnet.go,internal/client/subnet.go,internal/client/service_account.go,internal/client/vm.go,internal/client/nsg.go,internal/client/route_table.go,internal/client/node_pool.go,internal/client/cluster.go, andinternal/client/bastion.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/project.go` around lines 87 - 92, The 404 handling in GetProjectByID is still parsing err.Error() text, which should be replaced with a typed/status-bearing error from doRequest and checked directly in the read path. Update doRequest to return an error that exposes the HTTP status, then change GetProjectByID to branch on that status instead of strings.Contains(err.Error(), "HTTP 404"), preserving the nil,nil behavior for not-found. Apply the same pattern to the other client read methods that currently inspect error strings, including the ones in vnet, subnet, service_account, vm, nsg, route_table, node_pool, cluster, and bastion.internal/client/vm.go (1)
91-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a status-based helper for 404 handling.
GetVMand the other getters ininternal/clientall depend ondoRequestemitting"HTTP 404"in its error text. A small shared helper ininternal/client/client.gowould make the 404→nilcontract explicit and avoid breaking drift detection if the formatter changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/vm.go` around lines 91 - 96, The 404 handling in GetVM currently depends on matching the text from doRequest, which is brittle across getters in internal/client. Add a shared status-based helper in internal/client/client.go that detects a 404 from the request error or status and returns the nil/not-found result explicitly, then update GetVM and the other getter methods to use that helper instead of strings.Contains on err.Error(). Keep the existing nil-on-404 contract and preserve the existing wrapping for non-404 errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dc-api-reference.md`:
- Line 66: The auth example in docs/dc-api-reference.md still shows unsupported
provider arguments (`tenant_id` and `project_id`) even though the provider
examples only take `endpoint` and `token`; update the example to use the same
supported auth fields as the rest of the docs. Also tighten the credential
guidance in the Terraform section by removing the “member” minimum for tenant
operations and reflecting that tenant actions are admin-only, using the relevant
provider example blocks and tenant auth text as the source of truth.
In `@GNUmakefile`:
- Around line 4-12: The install target currently copies the provider as
terraform-provider-dcapi, but Terraform discovers plugins by the versioned
binary name; update the GNUmakefile install flow so the copy step in install
uses the Terraform versioned filename for the built artifact, while keeping
BINARY and INSTALL_DIR aligned with the provider version. Use the existing build
and install targets to locate the change and ensure the installed filename
includes v0.1.0.
In `@go.mod`:
- Around line 70-78: The dependency graph still contains vulnerable pinned
versions in go.mod, so refresh the affected modules before release. Update the
transitive dependencies pulled in through terraform-plugin-sdk/v2 that bring in
golang.org/x/crypto and google.golang.org/grpc, then run go mod tidy to
regenerate the module graph and lock in the upgraded versions.
In `@internal/client/client.go`:
- Around line 6-14: The shared HTTP client used by doRequest currently relies on
the default client with no timeout, which can leave plan/apply blocked on hung
connections. Update the client initialization in client.go to use a bounded
timeout, or accept a timeout from provider configuration and thread it into the
client used by doRequest. Keep the change centered around the shared HTTP client
path and the request flow that all resource calls use.
In `@internal/client/node_pool.go`:
- Around line 28-35: The NodePoolUpdateRequest shape in node_pool.go cannot use
nil slices/maps to mean “omit” because encoding/json will emit null for Taints
and Labels, which can unintentionally clear server state on count-only updates.
Update the request model and its PATCH serialization path so omitted fields are
truly excluded from the JSON payload, or add custom marshaling/pointer fields in
NodePoolUpdateRequest to preserve the intended omit-vs-clear behavior for Taints
and Labels.
- Around line 72-73: The node-pool request path is interpolating poolName
directly in DCAPIClient.GetNodePool, which can break GET/PATCH/DELETE when the
name contains reserved characters. Update the node-pool URL construction to
escape the poolName segment before formatting the path, and apply the same fix
anywhere the node-pool path is built so the full name remains intact even
without a ValidateFunc on name.
In `@internal/client/subnet.go`:
- Around line 51-56: CreateSubnet currently returns wrapper.Resource without
verifying it was actually present, which can let a missing 202 response body
slip through as (nil, nil). Update CreateSubnet in internal/client/subnet.go to
validate the unmarshaled SubnetCreateResponse has a non-nil Resource before
returning, and if it is missing, return an explicit error from the CreateSubnet
path instead of nil; keep the existing JSON parsing flow but add the check right
after json.Unmarshal and before returning wrapper.Resource.
In `@internal/client/vm.go`:
- Around line 77-82: The CreateVM response handling currently returns a parsed
VMCreateResponse even when the accepted payload is structurally incomplete.
After json.Unmarshal in CreateVM, validate that the returned resp includes a
non-empty resource.id before returning it, and if it is missing, fail fast with
a clear error instead of returning a nil/empty state that will break later
polling.
In `@internal/client/vnet.go`:
- Around line 49-54: The CreateVNet response handling currently returns
wrapper.Resource unchecked, which can turn an empty 202 response into (nil,
nil). Update the CreateVNet logic in vnet.go to validate that the unmarshaled
VNetCreateResponse contains a non-nil Resource before returning; if it is
missing, return a descriptive error from this path instead of a nil result so
callers get a clear diagnostic.
In `@internal/provider/provider.go`:
- Around line 45-59: The provider registration in ResourcesMap is missing the
dcapi_vnet_peering resource, so add the corresponding entry alongside the other
dcapi_* resources in provider initialization. Use the existing resource factory
pattern from symbols like ResourceVNet, ResourceCluster, and
ResourceRouteTableAssociation, and ensure the new map key exactly matches
dcapi_vnet_peering so the resource can be declared and imported.
- Around line 31-38: The provider token resolution currently only uses the
provider block and DCAPI_TOKEN env fallback, but the advertised dcctl credential
fallback is missing. Update the token handling in provider initialization and
related validation paths in the provider schema/config code so that when token
is empty it also attempts to load existing dcctl credentials before failing. Use
the existing token schema field and the provider setup/validation logic
referenced by the token attribute to wire this fallback in cleanly.
In `@internal/resources/cluster.go`:
- Around line 84-89: The system_pool.disk_gb schema in the cluster resource is
causing perpetual ForceNew diffs when omitted from config because Read writes
the API value into state without a matching config value. Update the disk_gb
field definition in the cluster schema to either mark it as Computed or set an
explicit default that matches the API behavior, and make sure the resource’s
Read/update flow stays consistent with that choice so omitted config does not
trigger replacement.
In `@internal/resources/node_pool.go`:
- Around line 74-79: The `disk_gb` handling in `node_pool.go` is silently
treating invalid values like 0 as “unset,” which causes the API default to be
applied instead of failing. Update the `disk_gb` schema on the node pool
resource to validate a minimum of 40, and adjust the create/update path in
`nodePool` so `disk_gb` is only omitted when the attribute is truly not set
rather than when it is non-positive. Make sure the logic around the current
`disk_gb` handling preserves explicit user input and does not coerce invalid
values into defaults.
In `@internal/resources/route_table.go`:
- Around line 79-92: The route table schema allows an invalid
`virtual_appliance` route to proceed without `next_hop_ip`, and
`routeTableResource` only copies `next_hop_ip` opportunistically during apply.
Add validation in the `routeTableResource`/schema path so `next_hop_ip` is
required when `next_hop_type` is `virtual_appliance`, and reject it for the
non-`virtual_appliance` hop types as well. Keep the check close to the existing
`next_hop_type` and `next_hop_ip` fields so the bad combination is caught before
apply.
In `@internal/resources/service_account.go`:
- Around line 19-25: Add import support to the Resource schemas by setting an
Importer on ResourceServiceAccount and the other affected resource constructors
(node_pool, nsg, nsg_attachment, route_table, route_table_association). Also
update each Read handler, especially resourceServiceAccountRead, to parse the
composite import ID and repopulate every ForceNew parent field from it (for
example both tenant_id and project_id) so imported state is complete.
In `@internal/resources/tenant.go`:
- Around line 229-233: The tenant delete path currently reports success while
only removing the resource from Terraform state, which leaves the remote tenant
orphaned. Update resourceTenantDelete to stop pretending deletion succeeded:
instead of clearing the ID and returning nil, return a diagnostic indicating
tenant deletion is unsupported until the DC-API provides a real delete endpoint.
Keep the change localized to resourceTenantDelete so terraform destroy surfaces
a failure rather than silently succeeding.
- Around line 51-67: The quota cap schema fields in tenant resource are causing
perpetual diffs because the Terraform defaults of 0 conflict with the
API-resolved values written back into state. Update the resource schema entries
for cpu_cores_cap, memory_gb_cap, and storage_gb_cap in the tenant resource to
use Optional plus Computed (or add equivalent diff suppression/normalization) so
omitted or zero-valued configs do not constantly plan changes against the
resolved 80/256/2000 values.
In `@test/main.tf`:
- Around line 12-16: The dcapi_tenant resource example is using the wrong
attribute name, which causes the config to fail validation. Update the tenant
resource block in test/main.tf to use tenant_id instead of id, matching the
tenant examples and docs used elsewhere in this PR. Keep the rest of the
resource fields unchanged.
---
Minor comments:
In `@docs/dc-api-reference.md`:
- Line 27: The table-of-contents entry for PrivateDnsZone uses a broken anchor,
so update the markdown link target to match the actual section heading slug
generated for PrivateDnsZone. Fix the TOC entry itself rather than the section,
and verify the anchor resolves correctly in the docs renderer.
In `@examples/subnet/main.tf`:
- Around line 49-52: The subnet output is currently returning Terraform’s
resource ID instead of the API UUID. Update the `output "subnet_id"` block in
the subnet example to use the `dcapi_subnet.app.subnet_uuid` attribute, matching
the provider’s UUID field and the way the VM example wires subnet values
downstream. Keep the output name and description as-is, but ensure the value
references the UUID-exposing symbol on `dcapi_subnet.app`.
In `@examples/vm/main.tf`:
- Around line 82-85: The subnet output is returning the Terraform resource ID
instead of the API UUID, so update the output block named subnet_id in main.tf
to use dcapi_subnet.app.subnet_uuid just like the existing subnet_uuid reference
in the same configuration. Keep the description aligned with the value being
exported so downstream consumers receive the actual subnet UUID rather than the
provider-generated id.
In `@internal/resources/node_pool.go`:
- Around line 276-281: Treat HTTP 404 as a successful delete in the node pool
destroy flow. Update the DeleteNodePool path used by node pool deletion so a
not-found response is normalized to nil instead of bubbling an error, and make
the delete handler in internal/client/node_pool.go return success when the pool
is already gone. Keep the existing waitForNodePoolDeleted poller behavior in
internal/resources/node_pool.go so the resource still converges on absent state
when DeleteNodePool is called.
In `@internal/resources/tenant.go`:
- Around line 14-19: The tenant resource is missing import support, so terraform
import cannot work for this and the other new resources. Update ResourceTenant
and the corresponding Resource constructors in internal/resources to wire
schema.Resource.Importer, using the appropriate StateContext importer helper
already used by other resources. Make sure the import setup is added alongside
CreateContext, ReadContext, UpdateContext, and DeleteContext so dcapi_tenant and
related resources can be imported consistently.
In `@internal/resources/vnet.go`:
- Around line 253-260: The 409 delete guidance in the VNet error path is
pointing users to the wrong reference attribute. Update the message in the VNet
deletion logic (the strings.Contains(err.Error(), "HTTP 409") branch) so the
example Terraform reference uses dcapi_vnet.example.vnet_uuid instead of
dcapi_vnet.example.id, since dcapi_subnet.vnet_id requires the bare UUID and not
the composite state ID.
---
Nitpick comments:
In `@Architecture.md`:
- Line 3: Update the architecture documentation to reflect the full 0.1.0
resource set instead of only the tenant/project/vnet/subnet/vm slice. In the
Architecture.md sections currently describing the provider scope and resource
hierarchy, add Bastion, Cluster, NodePool, RouteTable, RouteTableAssociation,
NetworkSecurityGroup, NSGAttachment, VNetPeering, and ServiceAccount, and expand
the VPC-backed workload hierarchy accordingly. Use the existing architecture
headings and resource overview areas in Architecture.md so the doc stays aligned
with the shipped provider surface.
In `@docs/dc-api-reference.md`:
- Around line 419-429: The field classification table is labeling VM and Cluster
networking selectors as computed, but `network_name`, `vnet_id`, and `subnet_id`
are user-provided inputs in the create request bodies. Update the Field
Classification entries in the relevant documentation sections so these selectors
are marked as user inputs for their respective networking modes, keeping the
wording consistent with the request examples and the related Cluster table.
In `@internal/client/project.go`:
- Around line 87-92: The 404 handling in GetProjectByID is still parsing
err.Error() text, which should be replaced with a typed/status-bearing error
from doRequest and checked directly in the read path. Update doRequest to return
an error that exposes the HTTP status, then change GetProjectByID to branch on
that status instead of strings.Contains(err.Error(), "HTTP 404"), preserving the
nil,nil behavior for not-found. Apply the same pattern to the other client read
methods that currently inspect error strings, including the ones in vnet,
subnet, service_account, vm, nsg, route_table, node_pool, cluster, and bastion.
In `@internal/client/vm.go`:
- Around line 91-96: The 404 handling in GetVM currently depends on matching the
text from doRequest, which is brittle across getters in internal/client. Add a
shared status-based helper in internal/client/client.go that detects a 404 from
the request error or status and returns the nil/not-found result explicitly,
then update GetVM and the other getter methods to use that helper instead of
strings.Contains on err.Error(). Keep the existing nil-on-404 contract and
preserve the existing wrapping for non-404 errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ead4a6d-bb46-4aef-b4da-9824a528de03
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
Architecture.mdGNUmakefiledocs/dc-api-reference.mdexamples/bastion/main.tfexamples/cluster/main.tfexamples/node_pool/main.tfexamples/project/main.tfexamples/provider/main.tfexamples/service_account/main.tfexamples/subnet/main.tfexamples/tenant/main.tfexamples/vm/main.tfexamples/vnet/main.tfgo.modinternal/client/bastion.gointernal/client/client.gointernal/client/cluster.gointernal/client/node_pool.gointernal/client/nsg.gointernal/client/project.gointernal/client/route_table.gointernal/client/service_account.gointernal/client/subnet.gointernal/client/tenant.gointernal/client/vm.gointernal/client/vnet.gointernal/provider/provider.gointernal/resources/bastion.gointernal/resources/cluster.gointernal/resources/node_pool.gointernal/resources/nsg.gointernal/resources/nsg_attachment.gointernal/resources/project.gointernal/resources/route_table.gointernal/resources/route_table_association.gointernal/resources/service_account.gointernal/resources/subnet.gointernal/resources/tenant.gointernal/resources/vm.gointernal/resources/vnet.gomain.gotest/main.tf
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (18)
docs/Open-api-sprc.yaml (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFilename typo:
Open-api-sprc.yaml→Open-api-spec.yaml."sprc" is a transposition of "spec". Renaming also requires updating the two references in
docs/data-source-implementation-plan.md(lines 18 and 184).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/Open-api-sprc.yaml` at line 1, Rename the OpenAPI document from Open-api-sprc.yaml to Open-api-spec.yaml, and update both references in docs/data-source-implementation-plan.md to use the corrected filename.docs/grpc-to-resourcedata.md (2)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced blocks (markdownlint MD040).
Four fences have no language specified;
textsuits all of them.📝 Example fix
-``` +```text Terraform Core --gRPC (tfplugin5)--> terraform-plugin-go --> ...Also applies to: 189-189, 199-199, 303-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/grpc-to-resourcedata.md` at line 21, Specify the text language on all four unlabeled fenced code blocks in the documentation, including the blocks near the referenced sections, by changing each opening fence to use text while preserving their contents.Source: Linters/SAST tools
142-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hard-coded SDK and
cluster.goline numbers in the docs.The SDK citations like
helper/schema/schema.go:629depend on thev2.34.0pin, and provider calls likecluster.go:312,cluster.go:96-168,cluster.go:439, :456will shift with future edits. Use function names such asresourceClusterRead,ResourceCluster,expandSystemPool, andexpandWorkerPoolsunless the exact line is essential.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/grpc-to-resourcedata.md` around lines 142 - 144, Update the references in docs/grpc-to-resourcedata.md to remove hard-coded SDK versions and file line numbers, replacing them with stable function or symbol names such as resourceClusterRead, ResourceCluster, expandSystemPool, and expandWorkerPools. Retain a line citation only where the exact location is essential, and keep the documented call flow unchanged.docs/Open-api-spec.md (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the "uploaded file" framing and label the bare fences.
Line 3's "The uploaded file is an OpenAPI Specification…" reads as a leftover from a chat session; in-repo this should name the file directly. The server-URL fences at 135 and 141 also trip markdownlint MD040.
📝 Proposed fix
-The uploaded file is an **OpenAPI Specification (Swagger)** for the **DC-API**. - -Instead of the raw YAML, here's what it describes in plain English. +`docs/Open-api-sprc.yaml` is the **OpenAPI Specification** for the **DC-API**. +This document summarises it in plain English.-``` +```text https://dcapi.lk.internal.wso2.comAlso applies to: 135-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/Open-api-spec.md` around lines 3 - 5, Update the opening description to refer directly to the DC-API OpenAPI specification rather than an “uploaded file.” Add an explicit text language tag to both bare server-URL fenced code blocks near the server URL documentation, preserving their contents.Source: Linters/SAST tools
examples/vm/vm-legacy.tf (1)
1-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEntire file is commented out — move it to its own example directory instead.
Terraform merges every
.tfin a directory, so this can't be uncommented in place: it would collide withmain.tf'sterraform/providerblocks and its identically namedvm_ip/vm_private_key/vm_console_passwordoutputs. Shipping it fully commented means it is never parsed, formatted, or validated, and it will silently rot.The legacy bridge path is worth documenting (it demonstrates the
network_namevsvnet_id+subnet_idmutual exclusivity fromdocs/Open-api-sprc.yaml719-722), so relocating beats deleting: move it toexamples/vm_legacy/main.tfas live HCL, keeping the explanatory comment block at lines 21-25 as real comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/vm/vm-legacy.tf` around lines 1 - 56, The entire commented-out legacy VM example should be relocated rather than left inert. Create a standalone examples/vm_legacy/main.tf containing live Terraform configuration for the dcapi provider, project, dcapi_virtual_machine.web_legacy resource, and its outputs; preserve the explanatory legacy bridge comments, and remove the commented content from vm-legacy.tf to avoid collisions with the existing main.tf.docs/data-source-implementation-plan.md (1)
132-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlan doc describes a pre-implementation state that this same PR already changes.
Line 134 states the provider "currently has no
DataSourcesMapfield at all", butinternal/provider/provider.go:74-88in this PR already wires all 12 entries listed here. Likewise §3.4 presents theappendSetsharing decision as open, while the PR shipsinternal/datasources/helpers.go. Shipping the plan as-is will read as current-state documentation and mislead.Suggest either adding a status header (e.g. "Status: implemented in PR
#X— retained for design rationale") or converting the present-tense claims to past tense.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/data-source-implementation-plan.md` around lines 132 - 176, The plan document describes already-implemented provider and helper changes as pending work. Update sections 3.3 and 3.4 to clearly mark them as implemented while retaining design rationale, or rewrite the present-tense claims in past tense to reflect the existing DataSourcesMap wiring in New and appendSet placement in internal/datasources/helpers.go.examples/routeTableAssociation/main.tf (1)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
terraform fmthas not been run across the examples tree. Shared root cause: the example configs were hand-written without a formatting pass, leaving inconsistent indentation and trailing whitespace. Adding afmt -check -recursivestep to the GNUmakefile/CI would keep this from recurring.
examples/routeTableAssociation/main.tf#L1-L8: normalise theterraformblock from 4/6-space to 2-space indentation to match the other examples, and strip trailing whitespace on lines 5 and 39.examples/vm/main.tf#L41-L69: strip trailing whitespace on lines 22, 23, 32, 33, 34, 43, 44, 50, and 51.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/routeTableAssociation/main.tf` around lines 1 - 8, The example Terraform files need consistent formatting and whitespace cleanup. In examples/routeTableAssociation/main.tf lines 1-8, normalize the terraform block indentation to two spaces and remove trailing whitespace on lines 5 and 39; in examples/vm/main.tf lines 41-69, remove trailing whitespace on lines 22, 23, 32, 33, 34, 43, 44, 50, and 51. Add a recursive terraform fmt check to the GNUmakefile/CI to prevent future drift.examples/key_vault_secret/main.tf (1)
25-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose
kv_uuidfromdcapi_key_vault. Both examples parse an internal composite Terraform state ID to obtain the API UUID; make that UUID a computed resource attribute instead.
examples/key_vault_secret/main.tf#L25-L27: replacesplit()/element()parsing withdcapi_key_vault.prod_secrets.kv_uuid.examples/private_endpoint/main.tf#L71-L75: replacesplit()/element()parsing withdcapi_key_vault.pe-secrets-s87.kv_uuid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/key_vault_secret/main.tf` around lines 25 - 27, Expose and use the computed kv_uuid attribute instead of parsing composite state IDs: update examples/key_vault_secret/main.tf lines 25-27 to reference dcapi_key_vault.prod_secrets.kv_uuid, and examples/private_endpoint/main.tf lines 71-75 to reference dcapi_key_vault.pe-secrets-s87.kv_uuid.internal/resources/vnet_peering.go (1)
228-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
resource.StateChangeConfwithhelper/retry.StateChangeConf.Same deprecated API (SA1019) as flagged in
private_dns_zone.go— this file imports"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"and usesresource.StateChangeConfin bothwaitForVNetPeeringActiveandwaitForVNetPeeringDeleted.key_vault.goalready demonstrates the drop-inhelper/retry.StateChangeConfreplacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/vnet_peering.go` around lines 228 - 274, Replace the deprecated resource.StateChangeConf usage with helper/retry.StateChangeConf in both waitForVNetPeeringActive and waitForVNetPeeringDeleted. Update the import accordingly, preserving each function’s existing Pending, Target, Refresh, timeout, and error behavior.Source: Linters/SAST tools
internal/resources/dns_record.go (2)
97-126: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCreate is an upsert keyed on name+type — no existence check before adoption.
resourceDnsRecordCreatecallsUpsertDnsRecord, which per builds the records collection URL and POSTs the DnsRecordUpsertRequest, returning the parsed record response keyed on name+type (per the function's own doc comment). If a record with the samename+typealready exists (created by anotherdcapi_dns_recordresource or outside Terraform), this "create" call will silently overwrite its values and this resource will adopt that record's ID, breaking Terraform's create-should-fail-if-exists invariant and risking two state entries fighting over one record.Consider a pre-create existence check (
GetDnsRecordby name+type, if such a lookup exists) that fails fast with a clear "already exists — useterraform import" error, or document this upsert caveat explicitly for users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/dns_record.go` around lines 97 - 126, Update resourceDnsRecordCreate to check for an existing DNS record by name and type before calling UpsertDnsRecord; if found, return a clear error instructing users to use terraform import, while preserving the create flow when no record exists. Reuse an existing GetDnsRecord lookup if available and distinguish not-found errors from other lookup failures.
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce the documented
typeenum withvalidation.StringInSlice.The description enumerates the valid values ("A"|"AAAA"|"CNAME"|"SRV"|"TXT"|"MX") but nothing validates them client-side, unlike the analogous
rolefield intenant_member.go(line 50) which usesvalidation.StringInSlice. Adding the same here catches typos atplantime instead of a round-trip API error.💡 Proposed fix
"type": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - Description: "Record type: \"A\"|\"AAAA\"|\"CNAME\"|\"SRV\"|\"TXT\"|\"MX\". Part of the record's upsert identity — immutable.", + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validation.StringInSlice([]string{"A", "AAAA", "CNAME", "SRV", "TXT", "MX"}, false), + Description: "Record type: \"A\"|\"AAAA\"|\"CNAME\"|\"SRV\"|\"TXT\"|\"MX\". Part of the record's upsert identity — immutable.", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/dns_record.go` around lines 58 - 63, Update the `type` schema field in the DNS record resource to add `validation.StringInSlice` with the documented allowed values `A`, `AAAA`, `CNAME`, `SRV`, `TXT`, and `MX`, matching the validation pattern used by the analogous `role` field in `tenant_member.go`.internal/resources/key_vault_secret.go (1)
91-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCreate is a PUT upsert keyed on
key— no existence check before adoption.
resourceKeyVaultSecretCreatecallsWriteKeyVaultSecret, which per PUTs to the keyvault secret URL and returns the parsed KeyVaultSecretResponse (create or update). If a secret with the samekeyalready exists under this KeyVault, Create silently overwrites its value/metadata and adopts it into this resource's state instead of failing, the same collision risk flagged indns_record.go's Create handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/key_vault_secret.go` around lines 91 - 117, Update resourceKeyVaultSecretCreate to check whether the secret identified by tenantID, projectID, keyVaultID, and key already exists before calling WriteKeyVaultSecret. Return a diagnostic error for an existing secret, and only perform the PUT upsert and set the composite resource ID when no matching secret is found.internal/resources/private_dns_zone.go (1)
201-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
resource.StateChangeConfwithhelper/retry.StateChangeConf.Static analysis flags
resource.StateChangeConfas deprecated (SA1019: usehelper/retryinstead).key_vault.goin this same PR already uses the non-deprecatedhelper/retry.StateChangeConfwith an identical field API (Pending/Target/Timeout/MinTimeout/Refresh/WaitForStateContext), so this should be a straightforward import swap.♻️ Proposed fix
import ( "context" "fmt" "strings" "time" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "terraform-provider-dcapi/internal/client" ) ... - conf := &resource.StateChangeConf{ + conf := &retry.StateChangeConf{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/resources/private_dns_zone.go` around lines 201 - 247, Replace the deprecated resource.StateChangeConf usage in waitForPrivateDnsZoneActive and waitForPrivateDnsZoneDeleted with helper/retry.StateChangeConf, updating the import accordingly. Preserve the existing Pending, Target, Timeout, MinTimeout, Refresh, and WaitForStateContext behavior.Source: Linters/SAST tools
internal/datasources/dns_record.go (1)
93-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo ambiguity check on (name, type) match.
The loop returns on the first record matching
(name, type)without checking whether more than one record matches. If the API ever returns duplicates for this composite key, the wrong record is silently selected. See consolidated comment for the shared fix across similar data sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/datasources/dns_record.go` around lines 93 - 106, The record lookup loop must detect duplicate matches instead of returning the first record for a given (name, type) pair. Update the matching logic in the records loop to count or collect all matching records, return an appropriate diagnostic when more than one matches, and retain the existing field population only for the unique match.internal/datasources/vnet_peering.go (1)
105-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo ambiguity check on name match.
First match on
namewins silently. See consolidated comment for the shared fix across similar data sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/datasources/vnet_peering.go` around lines 105 - 123, Update the peering lookup loop in the data source read method to detect when multiple entries share the requested name instead of returning the first match. Track matches while iterating, return an explicit ambiguity diagnostic when more than one match is found, and only populate the resource fields and return success for a unique match.internal/datasources/vnet.go (1)
99-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo ambiguity check on name match.
First match on
namewins silently;vnet_uuidfrom a wrong match would misdirect child resources likedcapi_subnet.vnet_id. See consolidated comment for the shared fix across similar data sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/datasources/vnet.go` around lines 99 - 117, The vnet lookup loop must reject ambiguous name matches instead of returning the first result. Update the surrounding data-source read logic to count all VNETs matching name, return an error when more than one matches, and only set the ID and fields from the unique match; preserve the not-found behavior for zero matches and use the existing diagnostic/error conventions.internal/datasources/key_vault.go (1)
104-122: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo ambiguity check on name match.
First match on
namewins silently;kv_uuidfrom a wrong match would misdirectdcapi_private_endpoint.kv_id. See consolidated comment for the shared fix across similar data sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/datasources/key_vault.go` around lines 104 - 122, Update the name-matching logic in the key-vault read function to detect and reject ambiguous results when multiple entries share the requested name, rather than returning the first match. Only populate the resource fields and return diagnostics after confirming exactly one matching key vault; preserve the existing not-found behavior and diagnostic handling.internal/datasources/image.go (1)
58-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo ambiguity check on display_name match.
First match on
display_namewins silently. See consolidated comment for the shared fix across similar data sources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/datasources/image.go` around lines 58 - 69, Update the image lookup loop in the data source read function to detect multiple images with the same display_name instead of returning the first match. Track the matching image count, return the existing not-found behavior when none match, and emit an ambiguity diagnostic/error when more than one matches; only set the resource fields and return after confirming exactly one match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/data-source-implementation-plan.md`:
- Around line 192-198: Update the implementation plan steps 6 and 8 to reference
documentation that actually exists in the repository, replacing the missing
docs/manual-testing-plan.md and unavailable per-resource README sections;
alternatively, add and track those missing documents before retaining the
references. Keep the smoke-test and README-update requirements intact, anchored
to steps 6 and 8.
In `@docs/Open-api-sprc.yaml`:
- Around line 724-725: Update the VM, bastion, and VNet endpoint prose in
docs/Open-api-sprc.yaml at lines 724-725, 1061-1062, 2801-2802, 761, 1086, 2828,
and 2844 to use tenant/project-scoped paths, or consistently use the established
abbreviated convention. Also update the four VM endpoint headings in
docs/Open-api-spec.md lines 265-310 to the fully scoped
/v1/tenants/{tenant_id}/projects/{project_id}/virtual-machines[/{id}] forms.
- Around line 2346-2366: Resolve the service-account scope consistently across
the OpenAPI specification, using the
`/v1/tenants/{tenant_id}/projects/{project_id}/service-accounts` path and its
project scope as the source of truth unless the implementation proves otherwise.
Update createServiceAccount and listServiceAccounts descriptions, the
introductory and bearerAuth references, and listTenantRoleAssignments
cross-references to use the same scope and path. Align the provider’s
dcapi_service_account identity requirements with the confirmed scope, including
project_id when applicable.
In `@examples/cluster/main.tf`:
- Line 76: Update the kubeconfig retrieval command comment to instruct users to
create the output file with restrictive permissions, using umask 077 or install
-m 600 before writing the sensitive Terraform output.
In `@examples/key_vault_secret/main.tf`:
- Line 30: Replace the committed secret literal in the key vault secret resource
with a sensitive input variable that has no default value. Update the example to
consume that variable and ensure its state backend is configured securely,
without embedding production secret values in HCL.
In `@examples/nsg/main.tf`:
- Around line 71-81: Remove the duplicate NSG rule named allow-ssh from the
rules block in main.tf, unless it serves a distinct purpose; in that case,
update its name and corresponding rule settings to represent that purpose while
preserving unique rule names.
- Around line 59-69: Update the inbound “allow-ssh” rule to replace
source_address_prefix = "*" with the narrowly scoped administrator CIDR input
exposed by the example configuration, preserving the existing SSH port and rule
behavior.
In `@examples/routeTableAssociation/main.tf`:
- Around line 72-77: Add route validation for the routes block so
virtual_appliance next_hop_ip must belong to a subnet within the parent VNet,
and reject destination_cidr values that are unreachable from that VNet. Ensure
the routeTableAssociation example is rejected during apply for both 10.0.1.5 and
192.168.1.0/24.
In `@examples/tenant_member/main.tf`:
- Around line 15-17: Update dcapi_tenant_member and its example to use the
DC-API contract field role_definition backed by the role catalog, removing the
obsolete role/member tenant-rank and owner/member/viewer auth0 placeholder
usage. Replace user_sub with the same Asgardeo-compatible subject format used by
the other examples, and remove trailing whitespace from the role-assignment
line.
In `@examples/vnet_peering/main.tf`:
- Around line 41-64: Remove the dcapi_vnet_peering resource named b-to-a-s87,
keeping a-to-b-s87 as the sole peering for the VNet pair. Update the
b_to_a_status expectation or reference to reflect that createPeering installs
reciprocal routes through the remaining resource, without defining a second
peering.
In `@internal/client/private_dns_zone.go`:
- Around line 46-50: Reject create responses that omit the resource instead of
returning nil without an error. In internal/client/private_dns_zone.go:46-50,
update the create response handling after json.Unmarshal to return an error when
wrapper.Resource is nil; apply the same validation in
internal/client/vnet_peering.go:50-54. Preserve successful responses that
contain a resource.
In `@internal/client/tenant_member.go`:
- Around line 67-71: Update DeleteTenantMember to apply url.PathEscape to
principalID before interpolating it into the DELETE route, while leaving
tenantID handling and the existing endpoint structure unchanged.
In `@internal/datasources/project.go`:
- Around line 31-35: Rename the required lookup schema field from “id” to
“project_id” in the project data source schema, and update the associated lookup
logic to read project_id. Preserve the separate composite tenant_id/project_id
state identity and do not redefine Terraform’s implicit id field.
---
Nitpick comments:
In `@docs/data-source-implementation-plan.md`:
- Around line 132-176: The plan document describes already-implemented provider
and helper changes as pending work. Update sections 3.3 and 3.4 to clearly mark
them as implemented while retaining design rationale, or rewrite the
present-tense claims in past tense to reflect the existing DataSourcesMap wiring
in New and appendSet placement in internal/datasources/helpers.go.
In `@docs/grpc-to-resourcedata.md`:
- Line 21: Specify the text language on all four unlabeled fenced code blocks in
the documentation, including the blocks near the referenced sections, by
changing each opening fence to use text while preserving their contents.
- Around line 142-144: Update the references in docs/grpc-to-resourcedata.md to
remove hard-coded SDK versions and file line numbers, replacing them with stable
function or symbol names such as resourceClusterRead, ResourceCluster,
expandSystemPool, and expandWorkerPools. Retain a line citation only where the
exact location is essential, and keep the documented call flow unchanged.
In `@docs/Open-api-spec.md`:
- Around line 3-5: Update the opening description to refer directly to the
DC-API OpenAPI specification rather than an “uploaded file.” Add an explicit
text language tag to both bare server-URL fenced code blocks near the server URL
documentation, preserving their contents.
In `@docs/Open-api-sprc.yaml`:
- Line 1: Rename the OpenAPI document from Open-api-sprc.yaml to
Open-api-spec.yaml, and update both references in
docs/data-source-implementation-plan.md to use the corrected filename.
In `@examples/key_vault_secret/main.tf`:
- Around line 25-27: Expose and use the computed kv_uuid attribute instead of
parsing composite state IDs: update examples/key_vault_secret/main.tf lines
25-27 to reference dcapi_key_vault.prod_secrets.kv_uuid, and
examples/private_endpoint/main.tf lines 71-75 to reference
dcapi_key_vault.pe-secrets-s87.kv_uuid.
In `@examples/routeTableAssociation/main.tf`:
- Around line 1-8: The example Terraform files need consistent formatting and
whitespace cleanup. In examples/routeTableAssociation/main.tf lines 1-8,
normalize the terraform block indentation to two spaces and remove trailing
whitespace on lines 5 and 39; in examples/vm/main.tf lines 41-69, remove
trailing whitespace on lines 22, 23, 32, 33, 34, 43, 44, 50, and 51. Add a
recursive terraform fmt check to the GNUmakefile/CI to prevent future drift.
In `@examples/vm/vm-legacy.tf`:
- Around line 1-56: The entire commented-out legacy VM example should be
relocated rather than left inert. Create a standalone examples/vm_legacy/main.tf
containing live Terraform configuration for the dcapi provider, project,
dcapi_virtual_machine.web_legacy resource, and its outputs; preserve the
explanatory legacy bridge comments, and remove the commented content from
vm-legacy.tf to avoid collisions with the existing main.tf.
In `@internal/datasources/dns_record.go`:
- Around line 93-106: The record lookup loop must detect duplicate matches
instead of returning the first record for a given (name, type) pair. Update the
matching logic in the records loop to count or collect all matching records,
return an appropriate diagnostic when more than one matches, and retain the
existing field population only for the unique match.
In `@internal/datasources/image.go`:
- Around line 58-69: Update the image lookup loop in the data source read
function to detect multiple images with the same display_name instead of
returning the first match. Track the matching image count, return the existing
not-found behavior when none match, and emit an ambiguity diagnostic/error when
more than one matches; only set the resource fields and return after confirming
exactly one match.
In `@internal/datasources/key_vault.go`:
- Around line 104-122: Update the name-matching logic in the key-vault read
function to detect and reject ambiguous results when multiple entries share the
requested name, rather than returning the first match. Only populate the
resource fields and return diagnostics after confirming exactly one matching key
vault; preserve the existing not-found behavior and diagnostic handling.
In `@internal/datasources/vnet_peering.go`:
- Around line 105-123: Update the peering lookup loop in the data source read
method to detect when multiple entries share the requested name instead of
returning the first match. Track matches while iterating, return an explicit
ambiguity diagnostic when more than one match is found, and only populate the
resource fields and return success for a unique match.
In `@internal/datasources/vnet.go`:
- Around line 99-117: The vnet lookup loop must reject ambiguous name matches
instead of returning the first result. Update the surrounding data-source read
logic to count all VNETs matching name, return an error when more than one
matches, and only set the ID and fields from the unique match; preserve the
not-found behavior for zero matches and use the existing diagnostic/error
conventions.
In `@internal/resources/dns_record.go`:
- Around line 97-126: Update resourceDnsRecordCreate to check for an existing
DNS record by name and type before calling UpsertDnsRecord; if found, return a
clear error instructing users to use terraform import, while preserving the
create flow when no record exists. Reuse an existing GetDnsRecord lookup if
available and distinguish not-found errors from other lookup failures.
- Around line 58-63: Update the `type` schema field in the DNS record resource
to add `validation.StringInSlice` with the documented allowed values `A`,
`AAAA`, `CNAME`, `SRV`, `TXT`, and `MX`, matching the validation pattern used by
the analogous `role` field in `tenant_member.go`.
In `@internal/resources/key_vault_secret.go`:
- Around line 91-117: Update resourceKeyVaultSecretCreate to check whether the
secret identified by tenantID, projectID, keyVaultID, and key already exists
before calling WriteKeyVaultSecret. Return a diagnostic error for an existing
secret, and only perform the PUT upsert and set the composite resource ID when
no matching secret is found.
In `@internal/resources/private_dns_zone.go`:
- Around line 201-247: Replace the deprecated resource.StateChangeConf usage in
waitForPrivateDnsZoneActive and waitForPrivateDnsZoneDeleted with
helper/retry.StateChangeConf, updating the import accordingly. Preserve the
existing Pending, Target, Timeout, MinTimeout, Refresh, and WaitForStateContext
behavior.
In `@internal/resources/vnet_peering.go`:
- Around line 228-274: Replace the deprecated resource.StateChangeConf usage
with helper/retry.StateChangeConf in both waitForVNetPeeringActive and
waitForVNetPeeringDeleted. Update the import accordingly, preserving each
function’s existing Pending, Target, Refresh, timeout, and error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e41821fd-5be2-485e-a301-a730b2a97307
⛔ Files ignored due to path filters (1)
examples/project/tf-debug-plan.logis excluded by!**/*.log
📒 Files selected for processing (77)
.gitignoreREADME.mddc-api/dc-apidocs/Open-api-spec.mddocs/Open-api-sprc.yamldocs/data-source-implementation-plan.mddocs/grpc-to-resourcedata.mdexamples/bastion/main.tfexamples/cluster/main.tfexamples/data_sources/main.tfexamples/dns_record/main.tfexamples/key_vault/main.tfexamples/key_vault_secret/main.tfexamples/node_pool/main.tfexamples/nsg/main.tfexamples/private_dns_zone/main.tfexamples/private_endpoint/main.tfexamples/project/main.tfexamples/provider/main.tfexamples/routeTable/main.tfexamples/routeTableAssociation/main.tfexamples/subnet/main.tfexamples/tenant/main.tfexamples/tenant_member/main.tfexamples/vm/main.tfexamples/vm/vm-legacy.tfexamples/vnet/main.tfexamples/vnet_peering/main.tfinternal/client/bastion.gointernal/client/client.gointernal/client/dns_record.gointernal/client/image.gointernal/client/key_vault.gointernal/client/key_vault_secret.gointernal/client/nsg.gointernal/client/private_dns_zone.gointernal/client/private_endpoint.gointernal/client/region.gointernal/client/route_table.gointernal/client/subnet.gointernal/client/tenant.gointernal/client/tenant_member.gointernal/client/vm.gointernal/client/vnet.gointernal/client/vnet_peering.gointernal/datasources/dns_record.gointernal/datasources/helpers.gointernal/datasources/image.gointernal/datasources/key_vault.gointernal/datasources/nsg.gointernal/datasources/private_dns_zone.gointernal/datasources/project.gointernal/datasources/region.gointernal/datasources/route_table.gointernal/datasources/subnet.gointernal/datasources/tenant.gointernal/datasources/vnet.gointernal/datasources/vnet_peering.gointernal/provider/provider.gointernal/resources/bastion.gointernal/resources/cluster.gointernal/resources/dns_record.gointernal/resources/helpers.gointernal/resources/key_vault.gointernal/resources/key_vault_secret.gointernal/resources/node_pool.gointernal/resources/private_dns_zone.gointernal/resources/private_endpoint.gointernal/resources/project.gointernal/resources/service_account.gointernal/resources/subnet.gointernal/resources/tenant.gointernal/resources/tenant_member.gointernal/resources/vm.gointernal/resources/vnet.gointernal/resources/vnet_peering.gomain.go
💤 Files with no reviewable changes (2)
- examples/provider/main.tf
- README.md
🚧 Files skipped from review as they are similar to previous changes (21)
- main.go
- examples/vnet/main.tf
- examples/bastion/main.tf
- examples/node_pool/main.tf
- examples/project/main.tf
- internal/client/subnet.go
- examples/subnet/main.tf
- internal/resources/service_account.go
- internal/resources/subnet.go
- internal/client/route_table.go
- internal/resources/bastion.go
- internal/client/nsg.go
- internal/client/tenant.go
- internal/resources/vm.go
- internal/resources/tenant.go
- internal/client/vm.go
- internal/resources/project.go
- internal/client/bastion.go
- internal/resources/vnet.go
- internal/resources/cluster.go
- internal/resources/node_pool.go
| description = "Current cluster status." | ||
| } | ||
|
|
||
| # Retrieve with: terraform output -raw kubeconfig > ~/.kube/prod.yaml |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Create the kubeconfig with restrictive permissions.
This command can write credentials to a shell-default-permission file. Use umask 077 (or install -m 600) before redirecting the sensitive output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/cluster/main.tf` at line 76, Update the kubeconfig retrieval command
comment to instruct users to create the output file with restrictive
permissions, using umask 077 or install -m 600 before writing the sensitive
Terraform output.
| key_vault_id = element(split("/", dcapi_key_vault.prod_secrets.id), 2) | ||
|
|
||
| key = "db-password" | ||
| value = "super-secret-value" # sensitive — updating this bumps `version` in place |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not model secrets as committed HCL literals.
Use a sensitive input variable with no default (and a secured state backend) so copied examples do not normalize committing production secret values.
Proposed change
- value = "super-secret-value" # sensitive — updating this bumps `version` in place
+ value = var.db_password # sensitive — updating this bumps `version` in place🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/key_vault_secret/main.tf` at line 30, Replace the committed secret
literal in the key vault secret resource with a sensitive input variable that
has no default value. Update the example to consume that variable and ensure its
state backend is configured securely, without embedding production secret values
in HCL.
| user_sub = "auth0|abc123" | ||
| role = "member" | ||
| display_alias = "Alice" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'tenant_member.go' internal/resources internal/client --exec rg -n '"role"|role_definition|RoleDefinition' {}Repository: wso2/open-cloud-datacenter
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -t f 'tenant_member.go|Open-api-sprc.yaml|main.tf' . | sed 's#^\./##' | sort
echo
echo "== examples/tenant_member/main.tf =="
cat -n examples/tenant_member/main.tf
echo
echo "== internal/resources tenant member outline/content =="
for f in $(fd -t f 'tenant_member.go' internal/resources internal/client); do
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
done
echo
echo "== relevant OpenAPI lines =="
if [ -f docs/Open-api-sprc.yaml ]; then
wc -l docs/Open-api-sprc.yaml
sed -n '6880,7040p' docs/Open-api-sprc.yaml | cat -n
echo "--- role_definition occurrences ---"
rg -n -i 'role_definition|reader|contributor|owner/member/viewer|user_sub|01abc123' docs/Open-api-sprc.yaml -C 2
else
echo "docs/Open-api-sprc.yaml not found"
fi
echo
echo "== all user_sub examples in docs/main.tf =="
rg -n -i 'user_sub|auth0\||01abc123' docs examples -C 2Repository: wso2/open-cloud-datacenter
Length of output: 44023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/dc-api-reference.md tenant member section =="
sed -n '1720,1820p' docs/dc-api-reference.md | cat -n
echo
echo "== terminal whitespace check in examples/tenant_member/main.tf =="
python3 - <<'PY'
from pathlib import Path
p=Path("examples/tenant_member/main.tf")
for i,l in enumerate(p.read_text().splitlines(),1):
if l != l.rstrip() or line_no in [15,16]:
if i in [15,16]:
print(f"{i}: {l!r} len={len(l)} rstripped={len(l.rstrip())}")
print(f" has whitespace: {'tabs' if chr(9) in l else ''} {'trailing_spaces' if l.endswith(' ') else ''}")
PY
echo "end whitespace check"
echo
echo "== tenant member provider endpoint refs =="
rg -n 'tenant.*member|/members|tenant_member|CreateTenantMember|ResourceTenantMember|dcapi_tenant_member|role_definition|role' terraform-provider-dcapi internal docs examples -C 2Repository: wso2/open-cloud-datacenter
Length of output: 3709
Align the tenant member example with the DC-API role-assignment contract.
dcapi_tenant_member currently passes the role value as JSON role, but the matching OpenAPI contract uses user_sub + role_definition; the member tenant rank is also documented as removed. Update this provider/resource to accept a role_definition catalog key and remove the obsolete owner/member/viewer user_sub/auth0|... placeholder, while keeping the example user_sub compatible with the Asgardeo-sub examples elsewhere. Also remove the trailing whitespace on line 16.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/tenant_member/main.tf` around lines 15 - 17, Update
dcapi_tenant_member and its example to use the DC-API contract field
role_definition backed by the role catalog, removing the obsolete role/member
tenant-rank and owner/member/viewer auth0 placeholder usage. Replace user_sub
with the same Asgardeo-compatible subject format used by the other examples, and
remove trailing whitespace from the role-assignment line.
| # Direction 1: routes FROM vnet-a TOWARDS vnet-b. | ||
| resource "dcapi_vnet_peering" "a-to-b-s87" { | ||
| tenant_id = dcapi_vnet.peer-vnet-a-s87.tenant_id | ||
| project_id = dcapi_vnet.peer-vnet-a-s87.project_id | ||
| vnet_id = dcapi_vnet.peer-vnet-a-s87.vnet_uuid | ||
|
|
||
| name = "a-to-b-s87" | ||
| peer_vnet_id = dcapi_vnet.peer-vnet-b-s87.vnet_uuid | ||
|
|
||
| allow_forwarded_traffic = false | ||
| } | ||
|
|
||
| # Direction 2: routes FROM vnet-b TOWARDS vnet-a — required for full bidirectional | ||
| # connectivity. Without this second resource, only vnet-a could reach vnet-b. | ||
| resource "dcapi_vnet_peering" "b-to-a-s87" { | ||
| tenant_id = dcapi_vnet.peer-vnet-b-s87.tenant_id | ||
| project_id = dcapi_vnet.peer-vnet-b-s87.project_id | ||
| vnet_id = dcapi_vnet.peer-vnet-b-s87.vnet_uuid | ||
|
|
||
| name = "b-to-a-s87" | ||
| peer_vnet_id = dcapi_vnet.peer-vnet-a-s87.vnet_uuid | ||
|
|
||
| allow_forwarded_traffic = false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate relevant files"
fd -a . . | sed 's#^\./##' | rg '(^|/)(main\.tf|Open-api-sprc\.ya?ml)$' || true
echo
echo "## git status/stat"
git diff --stat || true
echo
echo "## inspect examples/vnet_peering/main.tf around peering"
if [ -f examples/vnet_peering/main.tf ]; then
nl -ba examples/vnet_peering/main.tf | sed -n '1,140p'
fi
echo
echo "## inspect OpenAPI peer spec sections"
if [ -f docs/Open-api-sprc.yaml ]; then
echo "## lines around createPeering"
nl -ba docs/Open-api-sprc.yaml | sed -n '3330,3390p'
fi
echo
echo "## search for vnet peering docs/spec/provider comments"
rg -n "createPeering| Only one peering|mutual|reciprocal|bidi|bidirectional|forwarded_traffic|vnet_peering" . --glob '!*.lock' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | head -200Repository: wso2/open-cloud-datacenter
Length of output: 1368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
import yaml
root = Path('.')
print("## YAML parse check")
p = root / 'docs' / 'Open-api-sprc.yaml'
if p.exists():
data = yaml.safe_load(p.read_text())
print("root keys sample:", list(data.keys())[:10])
op = data.get("paths", {}).get("/dc/api/v1/tenants/{tenant_id}/vpcs/{vpc_id}/peerings", {})
post = op.get("post")
print("createPeering path exists:", "/dc/api/v1/tenants/{tenant_id}/vpcs/{vpc_id}/peerings" in data.get("paths", {}))
if post:
print("createPeering summary:", post.get("operationId"), post.get("summary"))
txt = (post.get("description") or "").strip()
print("--- createPeering description ---")
for i, line in enumerate(txt.splitlines(),1):
print(f"{i:03}: {line}")
code = post.get("responses", {}).get(409)
print("has 409 response:", code is not None)
print("409 description:", (code.get("description") or "" if code else ""))
print("has 400 response:", post.get("responses", {}).get(400))
print("400 description:", (post.get("responses", {}).get(400, {}).get("description") or ""))
PYRepository: wso2/open-cloud-datacenter
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## files matching vnet peering names"
find . -maxdepth 5 -type f \( -iname '*vnet*peer*' -o -iname 'main.tf' \) | sort
echo
echo "## example file stats/contents"
wc -l examples/vnet_peering/main.tf docs/Open-api-sprc.yaml
awk 'NR>=1 && NR<=220 {printf "%6d %s\n", NR, $0}' examples/vnet_peering/main.tf
echo
echo "## search candidate file paths in OpenAPI"
rg -n 'dc/api/v1/tenants/.*/vpcs/.*/peerings|createPeering|create.*Peer|peerings|peering|mutual|reciprocal|only one|one peering|409' docs/Open-api-sprc.yaml
echo
echo "## broader repo search for vnet peering guidance"
rg -n 'b-to-a|a-to-b|vnet peering|peerings|create.*Peer|mutual|reciprocal|only one|one peering|bidirectional|forwarded_traffic' . --glob '!**/.git/**' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | head -200Repository: wso2/open-cloud-datacenter
Length of output: 15550
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
import yaml
for p in [Path('docs/Open-api-sprc.yaml')]:
if not p.exists():
print(f"missing {p}")
continue
data = yaml.safe_load(p.read_text())
print(f"## paths containing peer")
for path, methods in data.get("paths", {}).items():
if "peer" in path.lower():
print(path)
for m, details in methods.items():
print(f" {m.upper()}: {details.get('operationId')}")
txt = (details.get("description") or details.get("summary") or "")
# print first 20 lines for peer paths
for i, line in enumerate(txt.splitlines(), 1):
if i > 20: break
print(f" {i}: {line.strip()}")
print("## parse peer resource names in example")
p = Path('examples/vnet_peering/main.tf')
if p.exists():
res = re.findall(r'resource\s+"([^"]+)"\s+"([^"]+)"', p.read_text())
print(res)
PYRepository: wso2/open-cloud-datacenter
Length of output: 387
Remove the second peering resource
createPeering installs reciprocal routes and the API allows only one peering per VNet pair. Defining both a-to-b-s87 and b-to-a-s87 will reject the second creation with 409 Conflict; keep one resource and update b_to_a_status accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/vnet_peering/main.tf` around lines 41 - 64, Remove the
dcapi_vnet_peering resource named b-to-a-s87, keeping a-to-b-s87 as the sole
peering for the VNet pair. Update the b_to_a_status expectation or reference to
reflect that createPeering installs reciprocal routes through the remaining
resource, without defining a second peering.
| var wrapper PrivateDnsZoneCreateResponse | ||
| if err := json.Unmarshal(respBytes, &wrapper); err != nil { | ||
| return nil, fmt.Errorf("CreatePrivateDnsZone: failed to parse response: %w", err) | ||
| } | ||
| return wrapper.Resource, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject create responses without resource.
Both methods return a nil resource with no error when the API omits resource; fail immediately instead.
internal/client/private_dns_zone.go#L46-L50: return an error whenwrapper.Resource == nil.internal/client/vnet_peering.go#L50-L54: return an error whenwrapper.Resource == nil.
📍 Affects 2 files
internal/client/private_dns_zone.go#L46-L50(this comment)internal/client/vnet_peering.go#L50-L54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/client/private_dns_zone.go` around lines 46 - 50, Reject create
responses that omit the resource instead of returning nil without an error. In
internal/client/private_dns_zone.go:46-50, update the create response handling
after json.Unmarshal to return an error when wrapper.Resource is nil; apply the
same validation in internal/client/vnet_peering.go:50-54. Preserve successful
responses that contain a resource.
| // DeleteTenantMember sends DELETE /v1/tenants/{tenantID}/members/{principalID}. | ||
| // principalID is the OIDC sub string (TenantMemberResponse.PrincipalID) — NOT the | ||
| // role_assignment UUID returned in the "id" field. | ||
| func (c *DCAPIClient) DeleteTenantMember(ctx context.Context, tenantID, principalID string) error { | ||
| path := fmt.Sprintf("/v1/tenants/%s/members/%s", tenantID, principalID) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'tenant_member\.go$' .
echo "== file excerpt =="
if [ -f internal/client/tenant_member.go ]; then
nl -ba internal/client/tenant_member.go | sed -n '1,120p'
fi
echo "== related tenants/member endpoints/search =="
rg -n "DeleteTenantMember|/tenants/.*/members/|PrincipalID|TenantMemberResponse|members/" internal -S || true
echo "== Go url.PathEscape/import usage in client package =="
rg -n "net/url|PathEscape|fmt\\.Sprintf\\(\"/v1/tenants/%s/members/%s\"" internal/client || true
echo "== deterministic Go PathEscape behavior for subject-like values =="
python3 - <<'PY'
# Minimal illustration of characters that PathEscape would avoid embedding literally in paths.
samples = ["alice@example.com", "user/admin", "user?query=1", "user%hash", "user#frag", "user[slash]/child"]
encoding_table = {chr(i): None for i in range(32)} | {i: chr(i) for i in range(127, 256)}
import urllib.parse
for s in samples:
print(f"{s!r}: {urllib.parse.quote(s, safe='')!r}")
PYRepository: wso2/open-cloud-datacenter
Length of output: 351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
awk '{printf "%6d\t%s\n", NR, $0}' internal/client/tenant_member.go | sed -n '1,120p'
echo "== related tenant member endpoint/search =="
rg -n "DeleteTenantMember|/tenants/.*/members/|PrincipalID|TenantMemberResponse|members/" internal -S || true
echo "== client imports and path construction =="
rg -n 'net/url|PathEscape|fmt\.Sprintf\("/v1/tenants/%s/members/%s"|fmt\.Sprintf\("/v1/tenants/%s/members/%s"' internal/client || true
echo "== deterministic PathEscape/quote behavior for subject-like values =="
awk '{printf "%6d\t%s\n", NR, $0}' /tmp/coderabbit-shell-logs/shell-output-*.log 2>/dev/null || true
cat <<'PY' | python3
import urllib.parse
samples = ["alice@example.com", "user\admin", "user?query=1", "user#frag", "user[slash]/child"]
for s in samples:
print(f"{s!r}: {urllib.parse.quote(s, safe='')!r}")
PYRepository: wso2/open-cloud-datacenter
Length of output: 6262
Escape the opaque OIDC subject before using it as a path segment.
principalID is interpolated directly into the DELETE URL and can contain URL-significant characters, so deletion can target a malformed route. Encode it with url.PathEscape(principalID) when building this path.
Proposed fix
import (
"context"
"encoding/json"
"fmt"
+ "net/url"
)
- path := fmt.Sprintf("/v1/tenants/%s/members/%s", tenantID, principalID)
+ path := fmt.Sprintf("/v1/tenants/%s/members/%s", tenantID, url.PathEscape(principalID))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DeleteTenantMember sends DELETE /v1/tenants/{tenantID}/members/{principalID}. | |
| // principalID is the OIDC sub string (TenantMemberResponse.PrincipalID) — NOT the | |
| // role_assignment UUID returned in the "id" field. | |
| func (c *DCAPIClient) DeleteTenantMember(ctx context.Context, tenantID, principalID string) error { | |
| path := fmt.Sprintf("/v1/tenants/%s/members/%s", tenantID, principalID) | |
| // DeleteTenantMember sends DELETE /v1/tenants/{tenantID}/members/{principalID}. | |
| // principalID is the OIDC sub string (TenantMemberResponse.PrincipalID) — NOT the | |
| // role_assignment UUID returned in the "id" field. | |
| func (c *DCAPIClient) DeleteTenantMember(ctx context.Context, tenantID, principalID string) error { | |
| path := fmt.Sprintf("/v1/tenants/%s/members/%s", tenantID, url.PathEscape(principalID)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/client/tenant_member.go` around lines 67 - 71, Update
DeleteTenantMember to apply url.PathEscape to principalID before interpolating
it into the DELETE route, while leaving tenantID handling and the existing
endpoint structure unchanged.
| "id": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| Description: "Slug of the project to look up.", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sdk_version="$(awk '$1 == "github.com/hashicorp/terraform-plugin-sdk/v2" { print $2 }' go.mod)"
test -n "$sdk_version"
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/${sdk_version}/helper/schema/core_schema.go" |
rg -n 'implicitly required "id"|block.Attributes\["id"\]'
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/${sdk_version}/helper/schema/resource.go" |
rg -n 'Data source|SetId|validateResourceID'Repository: wso2/open-cloud-datacenter
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod sdk version =="
awk '$1 == "github.com/hashicorp/terraform-plugin-sdk/v2" { print $2 }' go.mod || true
echo
echo "== locate project.go =="
fd -a 'project\.go$' . | sed 's#^\./##' | head -20
echo
echo "== inspect internal/datasources/project.go relevant sections =="
wc -l internal/datasources/project.go
sed -n '1,170p' internal/datasources/project.go | cat -n
echo
echo "== Search for SDK code/files if present =="
find . -path './vendor/**' -o -path './.terraform/**' | head -5 || true
rg -n 'core_schema|ImplicitlyRequireOptionalID|Read|SetId|create.*Schema|Data Source|DataSource' go.mod go.sum internal go 2>/dev/null | head -80 || trueRepository: wso2/open-cloud-datacenter
Length of output: 14040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod and go.sum provider blocks =="
sed -n '1,120p' go.mod
echo
grep -n 'terraform-provider-dcapi\|terraform-plugin-sdk/v2\|terraform-plugin-go' go.sum | head -40
echo
echo "== inspect other parent-scoped data sources for lookup naming pattern =="
fd -a '\.go$' internal/datasources internal | sort
rg -n 'DataSource[A-Z]|tenant_id|SetId\(|Required: true|Computed: true' internal/datasources internal/resources -g '*.go' | head -200 || trueRepository: wso2/open-cloud-datacenter
Length of output: 22598
🌐 Web query:
terraform-plugin-sdk v2 data source top-level id reserved SetId implicitly required
💡 Result:
In the terraform-plugin-sdk v2, the id field is not strictly reserved in the schema as a hard-coded language keyword, but it holds a special, implicit status due to the legacy design of the ResourceData type [1][2]. When using SetId(string) within the SDK, the framework performs two actions: it sets the internal resource identifier (InstanceState.ID) and implicitly writes or updates an id attribute in the resource state [1][2]. Because of this historical tight coupling, the id field has traditionally been treated as a mandatory attribute for managed resources and data sources to ensure proper state serialization and provider compatibility [3][2][4][5]. Key technical implications include: 1. Implicit Attribute Management: If you define an id attribute in your schema, the SDK's SetId() method will automatically populate it [1]. Historically, this led to requirements that the id attribute be marked as Computed: true in the schema if it were to be handled correctly without causing validation or consistency errors [3][5]. 2. Evolution and Deprecation: Over time, the SDK introduced methods like GetResourceId() and SetResourceId() to allow developers to manage the resource identifier independently of the id attribute, attempting to decouple the two [2][4]. 3. Best Practices: While SDK v2 continues to support this pattern for compatibility, modern development strongly encourages migrating to the terraform-plugin-framework [6][5]. In the framework, id is a standard, explicit attribute that must be managed like any other field, removing the implicit, "magic" behavior found in the legacy SDK [7]. If you are currently working within the v2 SDK, it is recommended to ensure your schema includes "id": { Type: schema.TypeString, Computed: true } if you are using SetId() to prevent state consistency issues [3][5].
Citations:
- 1: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource_data.go
- 2: helper/schema: Introduce ResourceData GetResourceId and SetResourceId receiver methods and only passthrough id attribute state if present hashicorp/terraform-plugin-sdk#542
- 3: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource.go
- 4: Ability to Get/Set Only the State Resource Identifier (Without id Attribute Handling) hashicorp/terraform-plugin-sdk#541
- 5: Prevent Invalid id Attribute Schema (Missing Computed) hashicorp/terraform-plugin-sdk#654
- 6: Data source schema incorrectly allows identity to be defined hashicorp/terraform-plugin-sdk#1511
- 7: https://developer.hashicorp.com/terraform/plugin/framework/data-sources
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SDK v2.34.0 core/resource data sources for id handling =="
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/resource_data.go" | rg -n -C 3 'func \[Mm]arkResourceTypeFullyComputedForDataSources|ImplicitlyRequireOptionalID|block.Attributes\["id"\]|SetResourceID|SetId|Read|ResourceData' || true
echo
curl -fsCL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/core_schema.go" | rg -n -C 4 'ExplicitResourceID|ImplicitlyRequireOptionalID|top-level' || true
echo "== SDK v2.34.0 data source read path schema validation =="
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/resource.go" | rg -n -C 5 'Data Source|dataSource|SetStateID|Read|Validate|SetId|markResourceTypeFullyComputedForDataSources' || trueRepository: wso2/open-cloud-datacenter
Length of output: 27430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ReadDataApply implementation =="
sed -n '1028,1120p' "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/resource.go" 2>/dev/null || curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/resource.go" | sed -n '1028,1120p'
echo
echo "== SchemaMap DataData and SetResource implementations =="
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/schema.go" | rg -n -C 4 'func \(.*SchemaMap\).*Data|SetResource|SetResourceID|ReservedDataSourceFields|ImplicitlyRequireOptionalID|markResourceType' || true
echo
echo "== core_schema behavior with explicit id via Go parser/text check =="
curl -fsSL "https://raw.githubusercontent.com/hashicorp/terraform-plugin-sdk/v2.34.0/helper/schema/core_schema.go" | sed -n '1,260p' | nlRepository: wso2/open-cloud-datacenter
Length of output: 2981
🌐 Web query:
github.com/hashicorp/terraform-plugin-sdk/v2.34.0 InternalValidate id required data source
💡 Result:
In the Terraform Plugin SDK (including version v2.34.0), the "id" attribute is implicitly treated as a reserved field for resources and data sources [1]. The SDK's InternalValidate method performs validation to ensure that any custom definition of the "id" attribute, if provided, adheres to specific requirements [2][3]. Key points regarding the "id" attribute and InternalValidate: 1. Implicit Handling: If an "id" attribute is not explicitly defined in your schema, the SDK implicitly adds it as Computed: true and Optional: true [4]. This is because the SDK relies on the "id" attribute to track state via SetId() [1][5]. 2. Validation Requirements: If you choose to explicitly define an "id" attribute in your schema (e.g., to add a custom description), InternalValidate will verify it [6][2]. If defined, it must typically be configured as Computed: true to align with the SDK's internal expectations [1]. Defining it as Required: true or with other conflicting constraints will cause InternalValidate to fail [1][2]. 3. Debugging Errors: If you encounter an "id" related error during InternalValidate, ensure that your schema definition does not conflict with the SDK's requirements [1][6]. Generally, you should not need to explicitly define "id" unless you are adding a custom description or specific documentation, in which case it should be marked as Computed: true [6]. 4. Recommended Practice: For general development, omit the "id" attribute from your schema definition entirely and let the SDK handle it implicitly [1][4]. If you are receiving a validation error, it is likely because your manual definition of "id" violates the requirements for resource identity [2]. Ensure any explicit "id" schema does not set Required: true [2].
Citations:
- 1: Prevent Invalid id Attribute Schema (Missing Computed) hashicorp/terraform-plugin-sdk#654
- 2: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource.go
- 3: https://github.com/hashicorp/terraform-plugin-sdk/blob/8984e1e6/helper/schema/resource.go
- 4: https://github.com/hashicorp/terraform-plugin-sdk/blob/master/helper/schema/core_schema.go
- 5: https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/import
- 6: Relax validation of ID attribute hashicorp/terraform-plugin-sdk#613
Do not define the data source’s lookup argument as Terraform’s id field.
id is reserved by the SDK for state identity; in a data source it is added implicitly as Optional: true/Computed: true and a custom Required: true schema field conflicts with that behavior. Use project_id for the lookup input and keep the composite tenant_id/project_id identity separate.
Proposed fix
- "id": {
+ "project_id": {
Type: schema.TypeString,
Required: true,
Description: "Slug of the project to look up.",
},
...
- id := d.Get("id").(string)
+ projectID := d.Get("project_id").(string)
- project, err := c.GetProjectByID(ctx, tenantID, id)
+ project, err := c.GetProjectByID(ctx, tenantID, projectID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "id": { | |
| Type: schema.TypeString, | |
| Required: true, | |
| Description: "Slug of the project to look up.", | |
| }, | |
| "project_id": { | |
| Type: schema.TypeString, | |
| Required: true, | |
| Description: "Slug of the project to look up.", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/datasources/project.go` around lines 31 - 35, Rename the required
lookup schema field from “id” to “project_id” in the project data source schema,
and update the associated lookup logic to read project_id. Preserve the separate
composite tenant_id/project_id state identity and do not redefine Terraform’s
implicit id field.
| func main() { | ||
|
|
||
| plugin.Serve(&plugin.ServeOpts{ | ||
|
|
There was a problem hiding this comment.
Let's remove unintended new lines?
| // included here with omitempty so GetTenantByID's list-and-scan still captures it without | ||
| // affecting the Create/Update unmarshal, which simply leaves it empty. | ||
| type TenantResponse struct { | ||
| ID string `json:"id"` |
|
let's add golint for this |
3 participants
What this changes
There was no Terraform provider for DC-API — provisioning was imperative, via
dcctland direct REST calls, with no plan step, no dependency ordering, and no drift detection. This adds a provider (registry.terraform.io/wso2/dcapi) that manages DC-API resources declaratively through Terraform's plan/apply lifecycle. Design and scope were agreed in #200.Behaviour
resource.StateChangeConfuntil the resource reaches a known ready state; only known terminal-failure statuses error, unknown/intermediate statuses keep polling.d.SetId()is set only after create is confirmed, so a failed poll produces a clean failure rather than a tainted resource in state.404, treating404as success (already gone). A409on VNet/Project delete while children still exist is surfaced as an actionable error rather than a silent retry.endpoint/tokenresolve from the provider block, falling back toDCAPI_ENDPOINT/DCAPI_TOKENenv vars; missing either fails configuration with an explicit diagnostic. The provider never becomes a store of long-lived secrets.tenant/project/vnet_uuid/subnet_uuid) so attribute references resolve and Terraform Core can infer create/destroy ordering. Sensitive outputs (private_key,console_password) are markedsensitive.url.PathEscaped.dcctl.Surface
registry.terraform.io/wso2/dcapi; resources nameddcapi_<resource>(e.g.dcapi_vnet,dcapi_virtual_machine).required_providersblock, provider configuration with optional explicitendpoint/token(both default to env vars).docs/dc-api-reference.md) and a runnable example per resource underexamples/<resource>/main.tf, covering the full tenant → project → vnet → subnet → vm flow plus each standalone resource type.golang.org/x/*,google.golang.org/grpc, etc.).Verification
go build ./...andgo vet ./...pass. Manual exercise viamake installagainst a local DC-API server (make run), covering create/read/update/delete/import for the core hierarchy.No automated test suite exists in this repo yet — there are currently no
_test.gofiles. Unit tests (httptest-backed fakes of DC-API for DTO decode / CRUD handler logic) and acceptance tests are still outstanding and should land as a follow-up before this is relied on beyond manual verification.