Copilot commented on code in PR #287:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/287#discussion_r2930850724
##########
cloudstack/service_offering_util.go:
##########
@@ -78,7 +78,10 @@ func (state *serviceOfferingCommonResourceModel)
commonRead(ctx context.Context,
if cs.Deploymentplanner != "" {
state.DeploymentPlanner =
types.StringValue(cs.Deploymentplanner)
}
- if cs.Diskofferingid != "" {
+ // Only set DiskOfferingId if it was already set in the state (i.e.,
user explicitly provided it)
+ // When using disk_offering block, CloudStack creates an internal disk
offering and returns its ID,
+ // but we should not populate disk_offering_id in that case to avoid
drift
+ if cs.Diskofferingid != "" && !state.DiskOfferingId.IsNull() {
Review Comment:
The comment says 'user explicitly provided it', but the guard only checks
`IsNull()`. If `DiskOfferingId` can be `Unknown` in state, this condition would
still set it and could reintroduce drift. Consider also guarding against
unknown (e.g., `!state.DiskOfferingId.IsUnknown()`) so the condition matches
the intended semantics.
##########
cloudstack/resource_cloudstack_loadbalancer_rule_test.go:
##########
@@ -54,6 +54,15 @@ func TestAccCloudStackLoadBalancerRule_basic(t *testing.T) {
}
func TestAccCloudStackLoadBalancerRule_update(t *testing.T) {
+ // Skip this test on CloudStack 4.22.0.0 due to a known simulator bug
+ // that causes "530 Internal Server Error" when updating load balancer
rules.
+ // This bug does not exist in 4.20.1.0, 4.22.1.0+, or 4.23.0.0+.
+ // See: https://github.com/apache/cloudstack/issues/XXXXX (if
applicable)
Review Comment:
The issue link contains a placeholder (`XXXXX`). Please replace it with the
actual issue/PR URL (or remove the line) so future maintainers can verify the
rationale for the skip.
##########
README.md:
##########
@@ -123,37 +123,51 @@ make test
In order to run the full suite of Acceptance tests you will need to run the
CloudStack Simulator. Please follow these steps to prepare an environment for
running the Acceptance tests:
-```sh
-docker pull apache/cloudstack-simulator
-
-or pull it with a particular build tag
+### Step 1: Start the CloudStack Simulator
+```sh
+# Pull the simulator image (recommended versions: 4.20.1.0 or
4.23.0.0-SNAPSHOT)
docker pull apache/cloudstack-simulator:4.20.1.0
-docker run --name simulator -p 8080:5050 -d apache/cloudstack-simulator
-
-or
-
+# Start the simulator container
docker run --name simulator -p 8080:5050 -d
apache/cloudstack-simulator:4.20.1.0
```
-When Docker started the container you can go to <http://localhost:8080/client>
and login to the CloudStack UI as user `admin` with password `password`. It can
take a few minutes for the container is fully ready, so you probably need to
wait and refresh the page for a few minutes before the login page is shown.
+**Note:** Version 4.22.0.0 has a known bug with updating load balancer rules
and is not recommended for testing.
Review Comment:
The README recommends `4.20.1.0` (and warns against `4.22.0.0`), but the
GitHub Actions matrix is now `4.20.2.0` and `4.22.0.0`. To avoid confusion for
contributors reproducing CI locally, align the README’s recommended versions
with what CI actually runs (or update CI to match the README).
##########
.github/workflows/acceptance.yml:
##########
@@ -30,7 +30,7 @@ permissions:
env:
CLOUDSTACK_API_URL: http://localhost:8080/client/api
- CLOUDSTACK_VERSIONS: "['4.19.0.1', '4.19.1.3', '4.19.2.0', '4.19.3.0',
'4.20.1.0']"
+ CLOUDSTACK_VERSIONS: "['4.20.2.0', '4.22.0.0']"
Review Comment:
This workflow matrix includes `4.22.0.0`, but the PR also introduces a skip
due to a known simulator bug on that exact version (and the README flags it as
not recommended). Keeping it in CI is likely to increase flakiness and reduce
signal. Consider replacing `4.22.0.0` with a fixed version (e.g., `4.22.1.0+`)
or removing it from the CI matrix entirely.
##########
cloudstack/provider_test.go:
##########
@@ -145,3 +145,32 @@ func testAccPreCheck(t *testing.T) {
t.Fatal("CLOUDSTACK_SECRET_KEY must be set for acceptance
tests")
}
}
+
+// getCloudStackVersion returns the CloudStack version from the API
+func getCloudStackVersion(t *testing.T) string {
+ cfg := Config{
+ APIURL: os.Getenv("CLOUDSTACK_API_URL"),
+ APIKey: os.Getenv("CLOUDSTACK_API_KEY"),
+ SecretKey: os.Getenv("CLOUDSTACK_SECRET_KEY"),
+ HTTPGETOnly: true,
+ Timeout: 60,
+ }
+ cs, err := cfg.NewClient()
+ if err != nil {
+ t.Logf("Failed to create CloudStack client: %v", err)
+ return ""
+ }
+
+ p := cs.Configuration.NewListCapabilitiesParams()
+ r, err := cs.Configuration.ListCapabilities(p)
+ if err != nil {
+ t.Logf("Failed to get CloudStack capabilities: %v", err)
+ return ""
+ }
+
+ if r.Capabilities != nil {
+ return r.Capabilities.Cloudstackversion
+ }
+
+ return ""
+}
Review Comment:
Since this helper influences test control flow (skips), returning an empty
string on failure can lead to running tests on versions you intended to skip
(or producing confusing CI output). Consider marking the helper with
`t.Helper()` and returning `(string, error)` (or `t.Fatalf` on failure) so
callers can decide whether to skip, fail, or proceed explicitly.
##########
cloudstack/resource_cloudstack_loadbalancer_rule_test.go:
##########
@@ -54,6 +54,15 @@ func TestAccCloudStackLoadBalancerRule_basic(t *testing.T) {
}
func TestAccCloudStackLoadBalancerRule_update(t *testing.T) {
+ // Skip this test on CloudStack 4.22.0.0 due to a known simulator bug
+ // that causes "530 Internal Server Error" when updating load balancer
rules.
+ // This bug does not exist in 4.20.1.0, 4.22.1.0+, or 4.23.0.0+.
+ // See: https://github.com/apache/cloudstack/issues/XXXXX (if
applicable)
+ version := getCloudStackVersion(t)
+ if version == "4.22.0.0" {
+ t.Skip("Skipping TestAccCloudStackLoadBalancerRule_update on
CloudStack 4.22.0.0 due to known simulator bug (Error 530: Internal Server
Error)")
+ }
Review Comment:
This calls `getCloudStackVersion(t)` before the standard acceptance
`PreCheck` runs (typically inside `resource.TestCase`). If env vars aren’t set,
this helper will log errors and return \"\", which can make failures noisier
and can also prevent the intended skip from triggering. Consider running
`testAccPreCheck(t)` before retrieving the version (or have
`getCloudStackVersion` perform the same validation/fail fast) to keep behavior
deterministic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
