Fix test findings: storage IP range updates, service offering import, project display_text - #332
Open
sudo87 wants to merge 7 commits into
Open
Fix test findings: storage IP range updates, service offering import, project display_text#332sudo87 wants to merge 7 commits into
sudo87 wants to merge 7 commits into
Conversation
CloudStack's updateStorageNetworkIpRange API validates a new IP range against the record's own current start/end IPs without excluding the record being updated, so any in-place edit of these fields fails with a self-overlap error (errorcode 530). Switching the update call to only send changed fields (d.HasChange instead of d.GetOk) did not help — the failure is identical even when only the genuinely-changed field is sent, confirming this is a server-side validation bug rather than something the provider can work around. Mark these fields ForceNew so the plan matches reality: Terraform now proposes a replacement instead of an update that is guaranteed to fail. Reproduced and reverified against ACS 4.23.0.0.
Investigated the reported destroy+recreate-on-import issue (#305): the importer's resulting state is actually fully hydrated correctly, because Terraform automatically calls Read (visible as "Refreshing state...") right after the custom importer function runs, which overwrites whatever partial state the importer itself set. So populating cpu_number/cpu_speed/ memory inside resourceCloudStackServiceOfferingImport would be a no-op — verified by importing a real lab offering with the unmodified importer and confirming cpu_number/cpu_speed/memory were already correct in state. The actual destroy+recreate happens because Terraform diffs the *config* (which a minimal post-import .tf typically leaves blank for these fields) against the now-correct state; since these fields are ForceNew, the config's implicit zero value differs from the real value and forces replacement. This is a doc/workflow gap, not a code bug: documented that all ForceNew fields must be fully specified in config after import. Verified empty terraform plan after import with a fully-specified config, and unchanged (still-forcing) plan with a minimal one, matching this explanation.
…splaytext Every other resource in this provider uses display_text; cloudstack_project was the outlier still on displaytext, even though its own docs already described display_text as the field name. Add display_text additively (displaytext is a real field in existing users' state files and can't be renamed outright) and mark displaytext Deprecated. Create/Update/Read resolve the effective value via projectDisplayText(), preferring display_text when both are set. Read only refreshes whichever of the two fields is actually in use (config already had displaytext set and display_text unset), matching the existing conditional pattern this file already uses for account/accountid/userid. Setting both unconditionally caused a permanent diff for display_text-only configs, since Read would keep populating the deprecated field the config never referenced. Verified against the lab with two standalone configs (one using display_text, one using the legacy displaytext) against a locally-built dev-override binary: both create cleanly, both produce an empty terraform plan, and the legacy field shows the expected deprecation warning. Both test projects destroyed after verification.
sureshanaparti
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR bundles three small fixes aimed at addressing RC1 test findings across the provider: avoiding failing in-place updates for storage network IP ranges, clarifying service offering import behavior with ForceNew fields, and improving the project resource schema by supporting a snake_case display_text field while deprecating displaytext.
Changes:
- Mark
netmask,start_ip, andend_ipasForceNewforcloudstack_storage_network_ip_rangeto avoid CloudStack update self-overlap failures, and adjust update logic to send only changed fields. - Document
cloudstack_service_offeringimport behavior to avoid accidental destroy/recreate whenForceNewfields are omitted from config after import. - Add
display_textsupport (and deprecatedisplaytext) forcloudstack_project, plus docs describing the deprecation and precedence behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/r/storage_network_ip_range.html.markdown | Documents that netmask/start_ip/end_ip changes require replacement due to CloudStack update behavior. |
| website/docs/r/service_offering.html.markdown | Adds an import warning explaining why omitting ForceNew fields after import can plan a replacement. |
| website/docs/r/project.html.markdown | Documents displaytext deprecation and precedence with display_text. |
| cloudstack/resource_cloudstack_storage_network_ip_range.go | Makes key IP range fields ForceNew and updates update logic to use HasChange. |
| cloudstack/resource_cloudstack_project.go | Adds deprecated displaytext, shared display text resolution logic, and conditional read/update handling for display_text. |
Suppressed comments (1)
cloudstack/resource_cloudstack_project.go:58
display_textis Optional-only but is populated from the CloudStack API in Read() (see later in this file). WithoutComputed: true, imported resources or configs that don’t setdisplay_textcan end up with persistent diffs. To match the provider’s established pattern fordisplay_text(e.g., network/template resources), make itOptional: true, Computed: true.
"display_text": {
Type: schema.TypeString,
Optional: true,
},
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+85
to
+93
| // projectDisplayText resolves the effective display text from the new | ||
| // display_text field and the deprecated displaytext field. display_text | ||
| // wins when both are set, since it's the field new configs should use. | ||
| func projectDisplayText(d *schema.ResourceData) string { | ||
| if v, ok := d.GetOk("display_text"); ok { | ||
| return v.(string) | ||
| } | ||
| return d.Get("displaytext").(string) | ||
| } |
Two issues surfaced by CI on the acceptance test matrix: - TestAccCloudStackProject_import failed ImportStateVerify: Read only populated whichever of display_text/displaytext was already set in config, so an import (empty config) only ever populated one of the two, leaving the other missing versus the pre-import state. Fix: mark both fields Optional+Computed and have Read set both unconditionally from the API value - Computed absorbs the field the config didn't reference, so this doesn't reintroduce the permanent-diff problem the conditional logic was working around. - That schema change broke updates: TestAccCloudStackProject_update and _updateUserid failed because projectDisplayText() picked display_text via GetOk, and a Computed field always carries a non-empty value from the last refresh, so it won permanent priority over a genuinely-changed displaytext. Fix: prefer whichever field actually has a pending change (HasChange), falling back to the GetOk-based preference only when neither changed (e.g. on create). Also addresses a Copilot review comment on the PR flagging the missing Computed: true on display_text. Verified: full TestAccCloudStackProject* suite passes (9/9) against the lab. Manually re-ran the ImportStateVerify scenario (apply, capture state, state rm, import, diff - now identical) and an update on both the new and legacy field (value actually changes in CloudStack, not just in state). Test projects destroyed after each run; confirmed no tf-rc1* projects remain.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
website/docs/r/project.html.markdown:39
- The docs mark
display_textas(Required), but the resource schema defines it asOptional+Computed(cloudstack/resource_cloudstack_project.go:56-60). This mismatch can confuse users and makes it unclear whether Terraform will enforce the requirement or if it only applies to certain CloudStack API versions.
* `display_text` - (Required) The display text of the project. Required for API version 4.18 and lower compatibility. This requirement will be removed when support for API versions older than 4.18 is dropped.
| return v.(string) | ||
| } | ||
| return d.Get("displaytext").(string) | ||
| } |
Contributor
There was a problem hiding this comment.
@sudo87 when display_text, displaytext are passed, display_text takes priority, right?
Review feedback on PR #332: the docs claimed "any in-place edit of these fields fails," but probing the API directly shows that's too broad - extending or shrinking the current range fails with a self-overlap error, but moving to a fully disjoint range succeeds in place. The real constraint is overlap with the record's own current IPs, not editing per se. Kept ForceNew as-is: Terraform can't know before attempting the update whether a given edit will land on the disjoint or overlapping case, so forcing replacement for all edits of these fields is still the only safe default. Also documented that replacement is more destructive than the failure it replaces (old range is deleted first; create_before_destroy can't help since the ranges overlap).
Review feedback on PR #332: setting both displaytext and display_text to different values never converges, since Read sets both fields to the API value every time, so whichever one the config didn't reference always looks changed on the next plan and gets reapplied indefinitely. Add ConflictsWith between the two fields so this is caught at plan time with a clear "Conflicting configuration arguments" error instead of a silent infinite-apply loop. Config is only ever allowed to set one of the two, matching the documented deprecation path (use display_text; the deprecated field is for existing state only). Verified against both the CloudStack simulator and the lab with a locally-built dev-override binary: a config setting both fields is rejected at plan time as expected; a config setting only one still creates, updates (value genuinely changes in CloudStack, not just state), and destroys cleanly. Full TestAccCloudStackProject* suite (9/9) passes against the lab.
vishesh92
reviewed
Sep 2, 2026
| @@ -37,6 +37,9 @@ The following arguments are supported: | |||
|
|
|||
| * `name` - (Required) The name of the project. | |||
| * `display_text` - (Required) The display text of the project. Required for API version 4.18 and lower compatibility. This requirement will be removed when support for API versions older than 4.18 is dropped. | |||
Review feedback on PR #332 (vishesh92): the Argument Reference marked display_text as (Required), but the schema has always had this field (displaytext originally) as Optional - this line predates this PR (resource added in #167) and was just never corrected. Fixed the annotation and dropped the inaccurate "required for API 4.18 compatibility" note along with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three small, independent fixes bundled together, separate commits, each revertable on its own.
Verified against a live lab with a local build; no leftover test objects.