Copilot commented on code in PR #301:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/301#discussion_r3557497149
##########
cloudstack/resource_cloudstack_disk_offering.go:
##########
@@ -52,16 +110,36 @@ func resourceCloudStackDiskOffering() *schema.Resource {
func resourceCloudStackDiskOfferingCreate(d *schema.ResourceData, meta
interface{}) error {
cs := meta.(*cloudstack.CloudStackClient)
name := d.Get("name").(string)
- display_text := d.Get("display_text").(string)
- disk_size := d.Get("disk_size").(int)
+ displayText := d.Get("display_text").(string)
// Create a new parameter struct
- p := cs.DiskOffering.NewCreateDiskOfferingParams(name, display_text)
- p.SetDisksize(int64(disk_size))
+ p := cs.DiskOffering.NewCreateDiskOfferingParams(displayText, name)
+
+ if v, ok := d.GetOk("disk_size"); ok {
+ p.SetDisksize(int64(v.(int)))
+ }
+
+ if v, ok := d.GetOk("customized"); ok {
+ p.SetCustomized(v.(bool))
+ }
Review Comment:
The schema now allows creating a disk offering with neither `disk_size` set
nor `customized = true`. In that case, the Create call sends neither `disksize`
nor `customized` to CloudStack (because `GetOk("customized")` is false when
it’s unset/default false), which is very likely to fail server-side with a less
helpful error. Add a provider-side validation (or derive `customized` like the
service_offering resource does) so users must either set `disk_size` or set
`customized` to true.
##########
website/docs/r/disk_offering.html.markdown:
##########
@@ -27,7 +27,20 @@ The following arguments are supported:
* `name` - (Required) The name of the disk offering.
* `display_text` - (Required) The display text of the disk offering.
-* `disk_size` - (Required) The size of the disk offering in GB.
+* `disk_size` - (Optional) The size of the disk offering in GB. Conflicts with
+ `customized`. Changing this forces a new resource to be created.
+* `customized` - (Optional) Whether the disk offering allows a custom disk size
+ to be specified at deployment time. Conflicts with `disk_size`. Defaults to
+ `false`. Changing this forces a new resource to be created.
Review Comment:
The docs describe `disk_size` as optional and `customized` as optional with
default `false`, but don’t explain that one of them must be used. Given the
implementation, a config that omits both will be invalid/ambiguous. Please
clarify that `disk_size` is required unless `customized` is set to `true` (and
that exactly one of them should be used).
##########
cloudstack/data_source_cloudstack_disk_offering.go:
##########
@@ -0,0 +1,168 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package cloudstack
+
+import (
+ "encoding/json"
+ "fmt"
+ "log"
+ "regexp"
+ "strings"
+ "time"
+
+ "github.com/apache/cloudstack-go/v2/cloudstack"
+ "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func dataSourceCloudstackDiskOffering() *schema.Resource {
+ return &schema.Resource{
+ Read: datasourceCloudStackDiskOfferingRead,
+ Schema: map[string]*schema.Schema{
+ "filter": dataSourceFiltersSchema(),
+
+ // Computed values
+ "name": {
+ Type: schema.TypeString,
+ Computed: true,
+ },
+ "display_text": {
+ Type: schema.TypeString,
+ Computed: true,
+ },
+ "disk_size": {
+ Type: schema.TypeInt,
+ Computed: true,
+ },
+ "customized": {
+ Type: schema.TypeBool,
+ Computed: true,
+ },
+ "storage_type": {
+ Type: schema.TypeString,
+ Computed: true,
+ },
+ "provisioning_type": {
+ Type: schema.TypeString,
+ Computed: true,
+ },
+ "tags": {
+ Type: schema.TypeString,
+ Computed: true,
+ },
+ "display_offering": {
+ Type: schema.TypeBool,
+ Computed: true,
+ },
+ },
+ }
+}
+
+func datasourceCloudStackDiskOfferingRead(d *schema.ResourceData, meta
interface{}) error {
+ cs := meta.(*cloudstack.CloudStackClient)
+ p := cs.DiskOffering.NewListDiskOfferingsParams()
+ csDiskOfferings, err := cs.DiskOffering.ListDiskOfferings(p)
+ if err != nil {
+ return fmt.Errorf("Failed to list disk offerings: %s", err)
+ }
+
+ filters := d.Get("filter")
+ var diskOfferings []*cloudstack.DiskOffering
+
+ for _, o := range csDiskOfferings.DiskOfferings {
+ match, err := applyDiskOfferingFilters(o, filters.(*schema.Set))
+ if err != nil {
+ return err
+ }
+ if match {
+ diskOfferings = append(diskOfferings, o)
+ }
+ }
+
+ if len(diskOfferings) == 0 {
+ return fmt.Errorf("No disk offering is matching with the
specified regex")
+ }
+
+ diskOffering, err := latestDiskOffering(diskOfferings)
+ if err != nil {
+ return err
+ }
+ log.Printf("[DEBUG] Selected disk offering: %s\n",
diskOffering.Displaytext)
+
+ return diskOfferingDescriptionAttributes(d, diskOffering)
+}
+
+func diskOfferingDescriptionAttributes(d *schema.ResourceData, diskOffering
*cloudstack.DiskOffering) error {
+ d.SetId(diskOffering.Id)
+ d.Set("name", diskOffering.Name)
+ d.Set("display_text", diskOffering.Displaytext)
+ d.Set("disk_size", int(diskOffering.Disksize))
+ d.Set("customized", diskOffering.Iscustomized)
+ d.Set("storage_type", diskOffering.Storagetype)
+ d.Set("provisioning_type", diskOffering.Provisioningtype)
+ d.Set("tags", diskOffering.Tags)
+ d.Set("display_offering", diskOffering.Displayoffering)
+
+ return nil
+}
+
+func latestDiskOffering(diskOfferings []*cloudstack.DiskOffering)
(*cloudstack.DiskOffering, error) {
+ var latest time.Time
+ var diskOffering *cloudstack.DiskOffering
+
+ for _, o := range diskOfferings {
+ created, err := time.Parse("2006-01-02T15:04:05-0700",
o.Created)
+ if err != nil {
+ return nil, fmt.Errorf("Failed to parse creation date
of a disk offering: %s", err)
+ }
+
+ if created.After(latest) {
+ latest = created
+ diskOffering = o
+ }
+ }
+
+ return diskOffering, nil
+}
+
+func applyDiskOfferingFilters(diskOffering *cloudstack.DiskOffering, filters
*schema.Set) (bool, error) {
+ var diskOfferingJSON map[string]interface{}
+ k, _ := json.Marshal(diskOffering)
+ err := json.Unmarshal(k, &diskOfferingJSON)
+ if err != nil {
+ return false, err
+ }
+
+ for _, f := range filters.List() {
+ m := f.(map[string]interface{})
+ r, err := regexp.Compile(m["value"].(string))
+ if err != nil {
+ return false, fmt.Errorf("Invalid regex: %s", err)
+ }
+ updatedName := strings.ReplaceAll(m["name"].(string), "_", "")
+ diskOfferingField, ok := diskOfferingJSON[updatedName].(string)
+ if !ok {
+ return false, fmt.Errorf("Field %s is not a string and
cannot be filtered", m["name"].(string))
+ }
Review Comment:
`applyDiskOfferingFilters` currently only supports filtering on fields that
marshal as JSON strings. However, this data source exports several non-string
attributes (e.g., `disk_size` int, `customized` bool, `display_offering` bool),
and the docs say filters can be applied to any exported attribute. As written,
filtering on those fields will always error. Consider stringifying primitive
values (or explicitly document/validate that only string fields are filterable).
##########
cloudstack/resource_cloudstack_disk_offering_test.go:
##########
@@ -0,0 +1,174 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package cloudstack
+
+import (
+ "fmt"
+ "testing"
+
+ "github.com/apache/cloudstack-go/v2/cloudstack"
+ "github.com/hashicorp/terraform-plugin-testing/helper/resource"
+ "github.com/hashicorp/terraform-plugin-testing/terraform"
+)
+
+func TestAccCloudStackDiskOffering_basic(t *testing.T) {
+ var do cloudstack.DiskOffering
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackDiskOfferingDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackDiskOffering_basic,
+ Check: resource.ComposeTestCheckFunc(
+
testAccCheckCloudStackDiskOfferingExists("cloudstack_disk_offering.test1", &do),
+
resource.TestCheckResourceAttr("cloudstack_disk_offering.test1", "name",
"disk_offering_1"),
+
resource.TestCheckResourceAttr("cloudstack_disk_offering.test1",
"display_text", "Test"),
+
resource.TestCheckResourceAttr("cloudstack_disk_offering.test1", "disk_size",
"10"),
+
resource.TestCheckResourceAttr("cloudstack_disk_offering.test1",
"storage_type", "shared"),
+ ),
+ },
+ },
Review Comment:
The resource now has an Importer configured, but the new acceptance tests
don’t exercise import. Many other resources in this provider include an
`ImportState`/`ImportStateVerify` step; adding one here would prevent
regressions in the importer/read mapping.
--
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]
