Skip to content

fix: implement Read/Update/Delete for stub resources#285

Open
kristofer-atlas wants to merge 3 commits intoapache:mainfrom
RunAtlas-is:feature/stub-resource-fixes
Open

fix: implement Read/Update/Delete for stub resources#285
kristofer-atlas wants to merge 3 commits intoapache:mainfrom
RunAtlas-is:feature/stub-resource-fixes

Conversation

@kristofer-atlas
Copy link

Implements missing Read, Update, and Delete funcitons for stub resources in the provider.

Affected resources:

  • cloudstack_account - Added Read and Update
  • cloudstack_disk_offering - Added Read and Update
  • cloudstack_domain - Added Read and Update
  • cloudstack_user - Added Read

Previously these resources had empty stub implementations that would cause state drift and prevent proper resource management.

Added test configurations under tests/ covering comprehensive provider features and networking with egress rules.

All changes pass make build and make test.

Partially addresses #133 (account domain association now supported through Read function).

- Add Read, Update, Delete for account resource
- Add Read, Update for disk_offering resource
- Add Read, Delete for domain resource
- Add Read for user resource
Add comprehensive and networking test configurations for
validating the CloudStack Terraform Provider.
Copy link
Contributor

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 implements previously stubbed CRUD functionality for several CloudStack Terraform provider resources to reduce state drift (notably for account/domain associations), and adds new Terraform configurations under tests/ for manual provider verification.

Changes:

  • Implement Read/Update for cloudstack_account, cloudstack_domain, cloudstack_disk_offering, and cloudstack_user (plus Delete for disk offering).
  • Adjust cloudstack_account schema to force recreation on immutable user/account fields.
  • Add new test configurations under tests/comprehensive and tests/networking (plus documentation) to exercise provider behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tests/networking/variables.tf Adds variables for the networking egress-rule test configuration.
tests/networking/terraform.tfvars.example Example tfvars for the networking test suite.
tests/networking/main.tf Networking test configuration demonstrating security group ingress + egress rules.
tests/comprehensive/versions.tf Defines provider requirements for the comprehensive test suite.
tests/comprehensive/variables.tf Adds variables for the comprehensive test suite inputs.
tests/comprehensive/terraform.tfvars.example Example tfvars for the comprehensive test suite.
tests/comprehensive/main.tf Comprehensive manual test configuration covering many resources.
tests/comprehensive/README.md Documents the comprehensive manual test suite.
tests/README.md Top-level documentation for running the manual test suites.
cloudstack/resource_cloudstack_user.go Implements user Update and Read logic.
cloudstack/resource_cloudstack_domain.go Implements domain Read and Update logic.
cloudstack/resource_cloudstack_disk_offering.go Implements disk offering Read/Update/Delete and adds needed imports.
cloudstack/resource_cloudstack_account.go Adds ForceNew to immutable fields and implements account Read/Update.

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

@vishesh92
Copy link
Member

@kristofer-atlas We already have some tests here: https://github.com/apache/cloudstack-terraform-provider/blob/main/cloudstack/resource_cloudstack_disk_test.go

How are the new tests helping us here?

@kristofer-atlas kristofer-atlas force-pushed the feature/stub-resource-fixes branch from bcc0576 to 17fcf46 Compare March 10, 2026 22:04
- Domain: add Computed+ForceNew to domain_id and parent_domain_id to
  fix perpetual diffs causing 19 acceptance test failures
- Account: remove unnecessary ForceNew from email/first_name/last_name/
  password; implement user-level updates via updateUser API; add
  Sensitive to password field
- Disk offering: mark disk_size as ForceNew since updateDiskOffering
  does not support size changes
- User: mark account and username as ForceNew to prevent silent drift;
  add Sensitive to password; wrap Update error with resource context
- Remove tests/ directory (manual TF configs, not Go acceptance tests)
  which also fixes Apache RAT check failures from missing headers
@kristofer-atlas
Copy link
Author

@vishesh92, the intent of the tests was to be a sort of end-to-end test. Removed for now. Hadn't noticed the tests you point to.

Wondering, though, whether we should have some sort of end-to-end tests similar tor what I had... In some cases I have had issues with create -> destroy -> recreate, where the destroy was incomplete, or some assumptions about state and lingering resources.

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.

3 participants