Don't build and test EOL versions#287
Conversation
There's no reason to build and test EOL versions of Cloudstack, OpenTofu, and Terraform. Also recent supported versions were being excluded.
…r bug - Add getCloudStackVersion() helper function to retrieve CloudStack version from API - Skip TestAccCloudStackLoadBalancerRule_update on version 4.22.0.0 specifically - Version 4.22.0.0 has a known bug causing '530 Internal Server Error' when updating LB rules - Test still runs on all other versions including 4.20.1.0, 4.22.1.0+, and 4.23.0.0+ - Update README with detailed simulator setup instructions including deployDataCenter.py step - Fix state drift issues in CNI configuration and service offering resources - Improve error reporting in load balancer rule updates to show raw API errors
|
@vishesh92 this will be needed to get clean tests for all the other PRs I created. Interestingly it appears the 4.22.0.0 simulator has some bug I had to disable some tests for. 4.21.1.0 and 4.23.0.0-snapshots worked, so there's likely a fix in upstream cloudstack just don't know what it was. |
whoops, posted this on my personal account instead of work account. Replying so you know it was really me :) |
|
@DaanHoogland any comment on this PR? it will fix the test issues on my other PRs once this is merged. |
code looks good @bhouse-nexthop , any reason not to put these changes in the other PRs? |
I usually like to keep PRs focused on a single topic, this is sort of out of scope other than the fact that there are CI/CD issues pre-existing. If you can merge it, I'd appreciate it. |
There was a problem hiding this comment.
Pull request overview
Updates acceptance testing and provider behavior to avoid testing EOL versions and reduce drift/test failures when working against the CloudStack simulator, while clarifying how to run acceptance tests locally.
Changes:
- Avoids provider state drift by conditionally setting fields only when users explicitly configured them.
- Skips a known-bad CloudStack simulator version for a failing acceptance test and improves an update error message.
- Updates acceptance CI matrices and expands README instructions for running simulator-based acceptance tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cloudstack/service_offering_util.go | Prevents disk_offering_id from being populated in state when it wasn’t user-specified (reduces drift). |
| cloudstack/resource_cloudstack_loadbalancer_rule_test.go | Adds version-based skip for a known simulator bug in update acceptance test. |
| cloudstack/resource_cloudstack_loadbalancer_rule.go | Improves update error message by including underlying error. |
| cloudstack/resource_cloudstack_cni_configuration.go | Avoids drift by only setting account/domain/project IDs if user supplied them. |
| cloudstack/provider_test.go | Adds helper to query CloudStack version from capabilities for conditional test behavior. |
| README.md | Clarifies and expands simulator + acceptance test setup steps. |
| .github/workflows/acceptance.yml | Adjusts test matrices and updates GitHub Actions used in acceptance CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@bhouse-nexthop Thanks for the PR. I will try to test and merge this and other PRs next week. I think it should be fine with the extra changes. We will just need to test it before merging once. Can you take a look at GitHub Copilot's comments (not all of them might be valid)? |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Instead of using GetOk conditionals in the Read function to avoid drift, mark account, domain_id, and project_id as Computed: true in the schema. This is the standard Terraform pattern used throughout the codebase and allows Terraform to properly handle values returned by the API that weren't explicitly set in the configuration. Addresses review comment about using Computed instead of GetOk pattern.
bhouse-nexthop
force-pushed
the
update-versions
branch
from
532da05 to
d633eeb
Compare
…ents - Make getCloudStackVersion() call testAccPreCheck() internally so callers don't need to - Fix testAccPreCheckCniSupport() to create its own CloudStack client instead of using testAccProvider.Meta() - Remove redundant testAccPreCheck() call from TestAccCloudStackLoadBalancerRule_update since getCloudStackVersion() now calls it The issue was that PreCheck functions run before the test framework configures the provider, so testAccProvider.Meta() is nil at that point. Functions that need to access CloudStack during PreCheck must create their own client from environment variables, following the pattern used in getCloudStackVersion() and other PreCheck functions like checkCKSEnabled.
Created a reusable newTestClient() helper function in provider_test.go that creates a CloudStack client from environment variables. This eliminates code duplication between getCloudStackVersion() and testAccPreCheckCniSupport(), and provides a consistent pattern for other PreCheck functions that need to access CloudStack before the test framework configures the provider.
Moved the getCloudStackVersion() call from the test function body into the PreCheck function. This ensures the version check only runs during acceptance tests (when TF_ACC is set), not during unit tests. Previously, the version check would fail unit tests when CloudStack environment variables were not set. This follows the standard pattern where all environment-dependent setup happens in PreCheck functions, which are only called by the test framework during acceptance tests.
|
@vishesh92 I think I resolved all outstanding comments. It should be good to go. Once this one is merged I'll rebase my other PRs to ensure they can merge cleanly. |
There's no reason to build and test EOL versions of Cloudstack, OpenTofu, and Terraform. Also recent supported versions were being excluded.
This PR will also corrects test failures in newer versions and updates documentation to be more explicit in how to properly run the acceptance tests.