Fix test findings: storage IP range updates, service offering import, project display_text - #332
Open
sudo87 wants to merge 3 commits into
Open
Fix test findings: storage IP range updates, service offering import, project display_text#332sudo87 wants to merge 3 commits into
sudo87 wants to merge 3 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
49
to
+53
| "displaytext": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Deprecated: "use display_text instead", | ||
| }, |
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) | ||
| } |
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.