service offering. sdk framework rewrite #138
Conversation
|
@poddm Can you add the updated docs to this PR as well? I believe you had docs on the other PR you closed, but there doesn't appear to be updated docs for this. I have pulled down your branch to test, and am still having issues with needing to specify the I get the following error: I'm using it like this: my locals |
|
@CodeBleu , I fixed the domain ids issue. Try now. Also, regarding the documentation. I was looking at using the generator but it might be better to scope all that work to its own PR. https://github.com/hashicorp/terraform-plugin-docs |
@poddm Thanks!!
I just did a you can see above that the Also, why is storage_tags avail in both at the 'root' level of the resource and also as
|
I think it would be better to add a data resource for domains. The problem I see with this approach is the resource will never become tainted if it was recreated as the same name. Subsequent resources that may modify or depend on the domain would never detect the newly re/created domain. ex. data "cloudstack_domain" "ace" {
name = "/ROOT/ACE"
}
resource "cloudstack_service_offering_fixed" "standard_offering" {
...
domain_ids = [data.cloudstack_domain.ace.id]
}
Let me check. I believe
Thats a bug. storage tags should be in the |
|
@poddm Thank you so much for looking into this. Once this offering stuff is complete, I still need a way to implement this with the Is this something you can fix with a new PR for |
|
@poddm I haven' tested but PR looks good. Left some minor comments. |
You can provide the values in the details field. resource "cloudstack_instance" "this" {
name = cloud
display_name = "cloud details"
details = {cpuNumber = "4", memory = "2048"}
} |
@poddm This is not currently part of the provider. Do you have this in your own branch? If so, Is there a PR for this too? https://registry.terraform.io/providers/cloudstack/cloudstack/latest/docs/resources/instance I tried to use what you show anyway, and I get this: |
|
@poddm Not sure if you saw that I'm still having issues with this or not. Wondering what I need to do? |
…to null, moved storage_tags field to nested block disk_offering
I fixed the duplicate storage_offering field.
This usually means the management server api returned an html error response and not json like the client library is expecting. I've been using this in |
Thanks, I will test this later. My main issue was the
What should I do?
|
|
@poddm Was wondering if you could help me with this ☝️ ? I need the |
fixed. |
@poddm Thanks, I do see that it is showing the
|
|
The error is being thrown from cloudstacks API. Try setting this global var to 0. |
Not really sure where you are talking about. Before the last change, this wasn't an issue and would create the offering with a In my I have this: |
|
@poddm Reviewing this PR again, I believe the only issue remaining ( that I had) was the Update: I went ahead and committed my suggestion for the |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the service offering resources to support the full CloudStack API specification by implementing a complete SDK framework rewrite. The main purpose is to split the service offering API into three distinct resource types based on different Quality of Service (QoS) types, making it clearer for users to understand the different offering configurations.
- Implements three new service offering resource types: constrained, unconstrained, and fixed
- Updates the Terraform plugin framework to version 1.12.0 and related dependencies
- Adds comprehensive test coverage for all three resource types with various disk configurations
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates Terraform plugin framework and related dependencies to newer versions |
| cloudstack/service_offering_util.go | Implements common utility functions for CRUD operations across all service offering types |
| cloudstack/service_offering_schema.go | Defines shared schema attributes for service offering resources with nested disk configurations |
| cloudstack/service_offering_models.go | Defines Go struct models for the three service offering types and their nested objects |
| cloudstack/service_offering_*_resource.go | Implements the three service offering resource types with full CRUD operations |
| cloudstack/service_offering_*_resource_test.go | Comprehensive test cases for all three resource types |
| cloudstack/provider_v6.go | Registers the new service offering resources with the provider |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| state.DisplayText = types.StringValue(cs.Displaytext) | ||
| } | ||
| if cs.Domainid != "" { | ||
| state.DomainIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Domainid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting domain IDs. The error should be handled properly or at least logged.
Copilot uses AI. Check for mistakes.
| state.Name = types.StringValue(cs.Name) | ||
| } | ||
| if cs.Zoneid != "" { | ||
| state.ZoneIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Zoneid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting zone IDs. The error should be handled properly or at least logged.
Copilot uses AI. Check for mistakes.
| p.SetDisplaytext(plan.DisplayText.ValueString()) | ||
| } | ||
| if !plan.DomainIds.IsNull() { | ||
| p.SetDomainid(plan.DomainIds.String()) |
There was a problem hiding this comment.
Using plan.DomainIds.String() directly may not provide the correct format for the API. This will likely result in a string representation like '["id1", "id2"]' instead of a comma-separated list. Consider extracting the actual string values and joining them with commas.
Copilot uses AI. Check for mistakes.
| p.SetName(plan.Name.ValueString()) | ||
| } | ||
| if !plan.ZoneIds.IsNull() && len(plan.ZoneIds.Elements()) > 0 { | ||
| p.SetZoneid(plan.ZoneIds.String()) |
There was a problem hiding this comment.
Using plan.ZoneIds.String() directly may not provide the correct format for the API. This will likely result in a string representation like '["id1", "id2"]' instead of a comma-separated list. Consider extracting the actual string values and joining them with commas.
| p.SetZoneid(plan.ZoneIds.String()) | |
| zoneIds := plan.ZoneIds.Elements() | |
| zoneIdStrs := make([]string, len(zoneIds)) | |
| for i, v := range zoneIds { | |
| zoneIdStrs[i] = v.(types.String).ValueString() | |
| } | |
| p.SetZoneid(strings.Join(zoneIdStrs, ",")) |
Copilot uses AI. Check for mistakes.
| state.DisplayText = types.StringValue(cs.Displaytext) | ||
| } | ||
| if cs.Domainid != "" { | ||
| state.DomainIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Domainid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting domain IDs. The error should be handled properly or at least logged.
Copilot uses AI. Check for mistakes.
| state.NetworkRate = types.Int32Value(int32(cs.Networkrate)) | ||
| } | ||
| if cs.Zoneid != "" { | ||
| state.ZoneIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Zoneid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting zone IDs. The error should be handled properly or at least logged.
Copilot uses AI. Check for mistakes.
| p.SetHypervisorsnapshotreserve(int(plan.HypervisorSnapshotReserve.ValueInt32())) | ||
| } | ||
| if !plan.MaxIops.IsNull() { | ||
| p.SetMaxiops(int64(plan.MaxIops.ValueInt64())) |
There was a problem hiding this comment.
Redundant type conversion from int64 to int64. The ValueInt64() method already returns int64, so the conversion is unnecessary.
| p.SetMaxiops(int64(plan.MaxIops.ValueInt64())) | |
| p.SetMaxiops(plan.MaxIops.ValueInt64()) |
Copilot uses AI. Check for mistakes.
| // // | ||
| // // 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. | ||
| // // |
There was a problem hiding this comment.
The license header has extra comment slashes. Lines 1-18 have double comment prefixes ('//' instead of a single '//'). This should be consistent with other files in the codebase.
| // // | |
| // // 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. | |
| // // | |
| // | |
| // 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. | |
| // |
Copilot uses AI. Check for mistakes.
|
|
||
| disk_offering = { | ||
| storage_type = "local" | ||
| sdfjklsdf = "sdfjks" |
There was a problem hiding this comment.
This appears to be test data with a nonsensical attribute name 'sdfjklsdf'. This will likely cause the test to fail as this is not a valid schema attribute. This should be removed or replaced with a valid attribute.
| sdfjklsdf = "sdfjks" |
Copilot uses AI. Check for mistakes.
| if !plan.CpuSpeed.IsNull() { | ||
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) | ||
| } |
There was a problem hiding this comment.
Missing validation that CpuSpeed is required. The schema shows this field as required, but there's a null check here. Either remove the null check or make the schema field optional.
| if !plan.CpuSpeed.IsNull() { | |
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) | |
| } | |
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) |
Copilot uses AI. Check for mistakes.
Enhanced the following resources to support the full api spec.
I split the service offering API up into multiple resources to make it a bit clearer of the different qos types