gridscale / terraform-provider-gridscale

Terraform gridscale provider
https://registry.terraform.io/providers/gridscale/gridscale/latest/docs
Mozilla Public License 2.0
12 stars 11 forks source link

Change gridscale_server resource hardware_profile default for new instances #141

Open bkircher opened 3 years ago

bkircher commented 3 years ago

gridscale_server resource still produces a "default" hardware profile instance. I believe we should switch here to "q35".

Expected Behavior

resource "gridscale_server" "foo" {
  name   = "foo"
  cores  = 1
  memory = 2
  power  = true
}

This should produce an instance with Q35 profile.

Basically this should behave as I would have given q35 explicitly

  # …
  hardware_profile = "q35"
  # …

Actual Behavior

It produces an instance with "Default" profile.

Compatibility

Note, we only want to change the default for new resources here. A TF provider update should not change existing instances.

I don't really know if we should categorize this as a breaking change. If users still rely on having pc-i440fx-2.4 if they don't specify a profile this would break their flow. Maybe we can get away with a major version bump and a notice in the release notes?

nvthongswansea commented 3 years ago

@bkircher it relates to this gsclient-go issue 161, which should be solved in the API level.

bkircher commented 3 years ago

No, I think API is already done here. I guess there's schema default?

Ah, yes, got it here https://github.com/gridscale/terraform-provider-gridscale/blob/master/gridscale/resource_gridscale_server.go#L61

bkircher commented 3 years ago

@itakouna Could you check if this would break compatibility for you guys in GSK provisioning if we change the default here?

nvthongswansea commented 3 years ago

No, I think API is already done here. I guess there's schema default?

Ah, yes, got it here https://github.com/gridscale/terraform-provider-gridscale/blob/master/gridscale/resource_gridscale_server.go#L61

I've just asked @MarcHarriss, he's confirmed that when you set hardware_profile to "Default" (via API, or panel), the q35 hardware profile is used under the hood.

bkircher commented 3 years ago

Erm, not 100% accurate.

The profile named "default" is not the default anymore. I know, sounds confusing. It is confusing :smile: and will be changed by API and front-end guys eventually.

The default now in panel and APIs is "q35". Which is correct. But we in TF provider explicitly set "default" as default here in this schema; which is wrong. We should make it either "q35" as well or just leave it unspecified to use the default dictated by the API (which is "q35").

(Formulating this sentence was fun :smile: )

I guess, this is why having a thing with the name "default" is almost always a bad idea :slightly_smiling_face:

MarcHarriss commented 3 years ago

What I meant was - when you don't specify a hardware profile, the Q35 is used a the "default" -> exactly as Ben describes here @nvthongswansea

itakouna commented 3 years ago

@bkircher we should add hardware_profile = "default" to TF files for keeping backward compatibility.

itakouna commented 3 years ago

@bkircher I strongly recommend bumping the major version of gs TF, when solving the issue.

bkircher commented 3 years ago

OK, thanks. So this will be a breaking change and needs special attention when releasing.

Do we do this in a 1.9.0 or in a 2.0.0? Any thoughts? @itakouna @nvthongswansea

Strictly semantic versioning would make this 2.0.0 I think, but that would mean we deliver this around May because we have other breaking changes in that package.

itakouna commented 3 years ago

Even though we release a major version this still might impact customers who do not pin GS TF version. In this case, all servers will be recreated.

terraform {
  required_providers {
    gridscale = "~> 1.7.4"
}
nvthongswansea commented 3 years ago

@bkircher @itakouna please be patient. I am trying some means to make it back-compat. I will post any updates here.

nvthongswansea commented 3 years ago

@bkircher @itakouna Regarding to #144, the default hardware profile of a server is now determined by the gs backend (which is q35 at the moment of writing) when users don't set hardware_profile. I also tested the back compatibility on my machine, and it works (for me).