Skip to content

Conversation

@toptobes
Copy link

@toptobes toptobes commented Oct 16, 2025

STATUS: Currently all of the implementation code is written and in theory working, but is not tested as of yet (no actual access to PCU stuff yet). Also a lot of minor TODOs to come back to.

Adds support for the following:

  • pcu_group data source
  • pcu_groups data source
  • pcu_group_associations data source
  • resolve_datacenter function (warning: requires terraform v1.8+)
  • pcu_group resource
  • pcu_group_association resource

Had to comment out the implementations of a few other resources/data sources as the modified astra-go-client used in this branch has unintended breaking changes.


TODOs before merging

  • Inconsistent return for cloud provider value (observed AZURE vs Azure coming back from API?) -> should sanitize the case of return values?
  • Fix non-backwards-compatible changes in astra-go-client on related PR

…oking, but untested and quite a few TODOs to come back to)
@toptobes toptobes marked this pull request as draft October 16, 2025 06:20
@toptobes toptobes linked an issue Oct 16, 2025 that may be closed by this pull request
@bdunn313 bdunn313 requested a review from Copilot November 6, 2025 21:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds comprehensive support for PCU (Provisioned Capacity Units) operations, enabling users to manage dedicated compute capacity for databases through Terraform. The implementation includes data sources for querying PCU groups and associations, resources for managing them, and a provider function to simplify datacenter ID resolution.

Key changes:

  • Adds PCU group management resources and data sources for provisioning and querying dedicated compute capacity
  • Introduces a resolve_datacenter function to simplify datacenter ID lookups from database resources
  • Updates test infrastructure to properly export environment variables for acceptance tests

Reviewed Changes

Copilot reviewed 32 out of 34 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
test/run_tests.sh Exports TF_ACC environment variable to enable acceptance tests
internal/provider/validators.go Adds Int32 validator for greater-than-or-equal-to comparisons
internal/provider/utils_pcu.go Implements core PCU service interfaces and API interaction logic
internal/provider/util_terraform.go Adds helper functions for Terraform attribute merging and HTTP response handling
internal/provider/util_misc.go Adds generic utility functions for nil handling and type conversions
internal/provider/types_pcus.go Defines PCU data models and schema attribute builders
internal/provider/resource_table.go Updates import order (cosmetic change)
internal/provider/resource_pcu_group_test.go Adds acceptance tests for PCU group lifecycle
internal/provider/resource_pcu_group_assocation.go Implements PCU group association resource with transfer support
internal/provider/resource_pcu_group.go Implements PCU group resource with park/unpark functionality
internal/provider/resource_database.go Updates comments from "All" to "FindById"
internal/provider/provider_framework_test.go Adds test helper for creating Astra client
internal/provider/provider_framework.go Registers new PCU data sources, resources, and functions
internal/provider/function_resolve_datacenter.go Implements datacenter ID resolution function
internal/provider/data_source_pcu_groups.go Implements data source for listing PCU groups
internal/provider/data_source_pcu_group_associations.go Implements data source for PCU group associations
internal/provider/data_source_pcu_group.go Implements data source for single PCU group lookup
go.mod Updates Go version and dependency versions
examples/ and docs/ files Adds documentation and examples for new PCU features

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

resp.Diagnostics.AddAttributeError(
v.path,
"Invalid Attribute Value",
fmt.Sprintf("Must be less than or equal to %s value (%d)", req.Path, req.ConfigValue.ValueInt32()),
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is misleading. It should say 'Must be greater than or equal to' since this validator checks if the config value is >= the compared value. The current message suggests the opposite relationship.

Suggested change
fmt.Sprintf("Must be less than or equal to %s value (%d)", req.Path, req.ConfigValue.ValueInt32()),
fmt.Sprintf("Must be greater than or equal to %s value (%d)", req.Path, req.ConfigValue.ValueInt32()),

Copilot uses AI. Check for mistakes.
it := astra.InstanceType(spec.InstanceType.ValueString()) // todo should be optional
pt := astra.ProvisionType(spec.ProvisionType.ValueString()) // todo should be optional

reservedCap := int(spec.Reserved.ValueInt32()) // todo should be optional
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These TODO comments indicate incomplete implementation. Either handle optional values properly (checking for null/unknown before converting) or remove the comments if the fields are actually required.

Suggested change
reservedCap := int(spec.Reserved.ValueInt32()) // todo should be optional
var reservedCap *int
if !spec.Reserved.IsNull() && !spec.Reserved.IsUnknown() {
val := int(spec.Reserved.ValueInt32())
reservedCap = &val
}

Copilot uses AI. Check for mistakes.
res, err := s.client.PcuDelete(ctx, id.ValueString())

if res != nil && res.StatusCode == 404 {
return nil // whatever
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'whatever' is unprofessional and unclear. Replace with a proper explanation like '// Resource already deleted or does not exist'

Suggested change
return nil // whatever
return nil // Resource already deleted or does not exist

Copilot uses AI. Check for mistakes.
res, err := s.client.PcuAssociationDelete(ctx, groupId.ValueString(), datacenterId.ValueString())

if res != nil && res.StatusCode == 404 {
return nil // whatever
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment 'whatever' is unprofessional and unclear. Replace with a proper explanation like '// Resource already deleted or does not exist'

Suggested change
return nil // whatever
return nil // Resource already deleted or does not exist

Copilot uses AI. Check for mistakes.
Comment on lines +72 to +78
func fillPCUGroupIds(data pcuGroupsDataSourceModel) {
data.PCUGroupIds = make([]types.String, len(data.PCUGroups))

for i, pcu := range data.PCUGroups {
data.PCUGroupIds[i] = pcu.Id
}
}
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function modifies a copy of the struct instead of the original. It should accept a pointer *pcuGroupsDataSourceModel to properly update the PCUGroupIds field.

Copilot uses AI. Check for mistakes.
PcuAttrCacheType: schema.StringAttribute{
Optional: true,
Computed: true,
Default: stringdefault.StaticString(string(astra.PcuInstanceTypeStandard)), // TODO what's the default when the types change? Should we even have a default for this?
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO comment raises valid concerns about the default value. Either resolve the decision about whether a default is appropriate or remove the comment if the current implementation is acceptable.

Suggested change
Default: stringdefault.StaticString(string(astra.PcuInstanceTypeStandard)), // TODO what's the default when the types change? Should we even have a default for this?
Default: stringdefault.StaticString(string(astra.PcuInstanceTypeStandard)),

Copilot uses AI. Check for mistakes.
PcuAttrProvisionType: schema.StringAttribute{
Optional: true,
Computed: true,
Default: stringdefault.StaticString(string(astra.PcuProvisionTypeShared)), // TODO do we validate the enum? Or let any string go?
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO indicates missing validation for enum values. Consider adding a validator to ensure only valid provision types are accepted, or remove the comment if validation is handled elsewhere.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for PCU operations

3 participants