-
Notifications
You must be signed in to change notification settings - Fork 48
STACKITRCO-187 - Add option iaas API param agent #1113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ type DataSourceModel struct { | |
| ServerId types.String `tfsdk:"server_id"` | ||
| MachineType types.String `tfsdk:"machine_type"` | ||
| Name types.String `tfsdk:"name"` | ||
| Agent types.Object `tfsdk:"agent"` | ||
| AvailabilityZone types.String `tfsdk:"availability_zone"` | ||
| BootVolume types.Object `tfsdk:"boot_volume"` | ||
| ImageId types.String `tfsdk:"image_id"` | ||
|
|
@@ -52,6 +53,10 @@ var bootVolumeDataTypes = map[string]attr.Type{ | |
| "delete_on_termination": basetypes.BoolType{}, | ||
| } | ||
|
|
||
| var agentDataTypes = map[string]attr.Type{ | ||
| "provisioned": basetypes.BoolType{}, | ||
| } | ||
|
|
||
| // NewServerDataSource is a helper function to simplify the provider implementation. | ||
| func NewServerDataSource() datasource.DataSource { | ||
| return &serverDataSource{} | ||
|
|
@@ -123,6 +128,16 @@ func (d *serverDataSource) Schema(_ context.Context, _ datasource.SchemaRequest, | |
| MarkdownDescription: "Name of the type of the machine for the server. Possible values are documented in [Virtual machine flavors](https://docs.stackit.cloud/products/compute-engine/server/basics/machine-types/)", | ||
| Computed: true, | ||
| }, | ||
| "agent": schema.SingleNestedAttribute{ | ||
| Description: "STACKIT Server Agent as setup on the server", | ||
| Computed: true, | ||
| Attributes: map[string]schema.Attribute{ | ||
| "provisioned": schema.BoolAttribute{ | ||
| Description: "Whether a STACKIT Server Agent is provisioned at the server", | ||
| Computed: true, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's what the API docs say about the "provisioned" server agent field: https://docs.api.stackit.cloud/documentation/iaas/version/v2#tag/Servers/operation/v2CreateServer
Since this is not trivial to solve, we as the STACKIT Developer Tools team decided that the default value for the "provisioned" field should be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be sure I understand it right:
It seems like you want to be even more explicit - and to always default the param to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a product owner of STACKIT Server Agent, I disagree with this to make the agent provisioned false by default. Please note that agent is also a prerequisite to use the server update management service. If we put it by default false, I am sure that we will need to answer on a lot of customer support tickets, why the updates are not working. (other features like basic server monitoring are also planned to work only via the agent). There was a long discussion last year about the default value for agent installation on different levels (even up to the patrons). The final decision which was made is that it is the best is to have the default property configured on the image. This means that on publicly provided by STACKIT images it will be true and for custom images the customers can decide what to do. I agree on this that image resource should return also the agent properties to make it more transparent, but we shouldn't have a default value about agent provisioning on server level. The behavior of this property should be consistent through all available options to create a server - API, Portal, Terraform provider, CLI. That is why please ensure that it follows the already agreed behavior:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @btotev-schwarz We decided to use a default value ( If one decides to not send a "provisioned" flag for a server POST request, the "provisioned" flag in the response can either be
Don't mix up read-only fields and fields marked as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry, you'll think I'm stubborn to continue the discussion and propose some technical solution. I know my code in the PR needs adjustments. I know the IaaS API itself probably could use a better design for this parameter (to use strings, like "yes", "no", "image-default" instead of a bool). I'll follow whatever you guys decide (product owners, terraform team). A small question: is the below solution (keeping the current API model) harder to maintain for the team and harder/unintuitive for the users? Sorry from taking your time with it. It seems the "provisioned" flag is not returned from the API at all when the If you're interested, I can provide further testing with creating a resource with the current code and modifying it (true/false/missing). Sorry for the interruption. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is UseStateForUnknown() a workaround or officially supported plan modifier for such cases (https://developer.hashicorp.com/terraform/plugin/framework/resources/plan-modification#usestateforunknown)? I mean readonly from server point of view - that you can't modify after that for the same resource via API. Of course you should be able to change this in the terraform and it will replace the resource. |
||
| }, | ||
| }, | ||
| }, | ||
| "availability_zone": schema.StringAttribute{ | ||
| Description: "The availability zone of the server.", | ||
| Computed: true, | ||
|
|
@@ -304,6 +319,18 @@ func mapDataSourceFields(ctx context.Context, serverResp *iaas.Server, model *Da | |
| model.BootVolume = types.ObjectNull(bootVolumeDataTypes) | ||
| } | ||
|
|
||
| if serverResp.Agent != nil { | ||
| agent, diags := types.ObjectValue(agentDataTypes, map[string]attr.Value{ | ||
| "provisioned": types.BoolPointerValue(serverResp.Agent.Provisioned), | ||
| }) | ||
| if diags.HasError() { | ||
| return fmt.Errorf("failed to map agent: %w", core.DiagsToError(diags)) | ||
| } | ||
| model.Agent = agent | ||
| } else { | ||
| model.Agent = types.ObjectNull(agentDataTypes) | ||
| } | ||
|
|
||
| if serverResp.UserData != nil && len(*serverResp.UserData) > 0 { | ||
| model.UserData = types.StringValue(string(*serverResp.UserData)) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The acceptance tests don't include your new field yet:
https://github.com/stackitcloud/terraform-provider-stackit/blob/44312a8ebc5b9da9af0608d845b2b733ea349851/stackit/internal/services/iaas/testdata/resource-volume-max.tf
https://github.com/stackitcloud/terraform-provider-stackit/blob/44312a8ebc5b9da9af0608d845b2b733ea349851/stackit/internal/services/iaas/iaas_acc_test.go
Could you also adjust them please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot run the acceptance tests (if I do, it probably costs money). I did not want to add code to files I cannot test.
If this is a strong requirement for this PR, how can I do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, we won't accept this without properly adjusted Acc tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're working at STACKIT it shouldn't be a problem for you I hope? 😅
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you should be using
go-vcror similar for such integration tests - recording/hardcoding API responses. The way they are written right now - it seems like they belong to an internal CI server, maybe even at the repo of a QA team. stackit-cli doesn't have them. The tests themselves and surely are useful - but how can you ask open source contributors to run them? How much does a sample run cost?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you have reached an agreement of a right implementation, I'll update/run the acceptance tests and update the PR.