Skip to content
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

resource/server: support boot_type #59

Merged
merged 1 commit into from
May 6, 2018
Merged

resource/server: support boot_type #59

merged 1 commit into from
May 6, 2018

Conversation

nicolai86
Copy link
Contributor

@nicolai86 nicolai86 commented Apr 25, 2018

this PR adds support for the newly introduced boot_type attribute for scaleway server resources:

resource "scaleway_server" "base" {
  name = "test"
  image = "e20532c4-1fa0-4c97-992f-436b8d372c07"
  type = "VC1S"
  tags = [ "terraform-test" ]
  boot_type = "local"
}

valid values are local and bootscript for x86_64, as well as bootscript for arm instance types.

Tests are green:

=== RUN   TestAccScalewayDataSourceBootscript_Filtered
--- PASS: TestAccScalewayDataSourceBootscript_Filtered (8.64s)
=== RUN   TestAccScalewayDataSourceImage_Basic
--- PASS: TestAccScalewayDataSourceImage_Basic (26.29s)
=== RUN   TestAccScalewayDataSourceImage_Filtered
--- PASS: TestAccScalewayDataSourceImage_Filtered (23.28s)
=== RUN   TestValidateBootType
=== RUN   TestValidateBootType/local
=== RUN   TestValidateBootType/bootscript
=== RUN   TestValidateBootType/remote
--- PASS: TestValidateBootType (0.00s)
    --- PASS: TestValidateBootType/local (0.00s)
    --- PASS: TestValidateBootType/bootscript (0.00s)
    --- PASS: TestValidateBootType/remote (0.00s)
=== RUN   TestAccScalewayIP_importBasic
--- PASS: TestAccScalewayIP_importBasic (12.73s)
=== RUN   TestAccScalewaySecurityGroup_importBasic
--- PASS: TestAccScalewaySecurityGroup_importBasic (14.63s)
=== RUN   TestAccScalewayServer_importBasic
--- PASS: TestAccScalewayServer_importBasic (151.10s)
=== RUN   TestAccScalewayToken_importBasic
--- PASS: TestAccScalewayToken_importBasic (22.30s)
=== RUN   TestAccScalewayUserData_importBasic
--- PASS: TestAccScalewayUserData_importBasic (18.98s)
=== RUN   TestAccScalewayVolume_importBasic
--- PASS: TestAccScalewayVolume_importBasic (8.19s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccScalewayIP_Count
--- PASS: TestAccScalewayIP_Count (25.56s)
=== RUN   TestAccScalewayIP_Basic
--- PASS: TestAccScalewayIP_Basic (61.75s)
=== RUN   TestAccScalewaySecurityGroupRule_Basic
--- PASS: TestAccScalewaySecurityGroupRule_Basic (32.84s)
=== RUN   TestAccScalewaySecurityGroupRule_Count
--- PASS: TestAccScalewaySecurityGroupRule_Count (23.28s)
=== RUN   TestAccScalewaySecurityGroup_Basic
--- PASS: TestAccScalewaySecurityGroup_Basic (11.70s)
=== RUN   TestAccScalewayServer_Basic
--- PASS: TestAccScalewayServer_Basic (174.10s)
=== RUN   TestAccScalewayServer_BootType
--- PASS: TestAccScalewayServer_BootType (69.84s)
=== RUN   TestAccScalewayServer_ExistingIP
--- PASS: TestAccScalewayServer_ExistingIP (137.15s)
=== RUN   TestAccScalewayServer_Volumes
--- PASS: TestAccScalewayServer_Volumes (160.96s)
=== RUN   TestAccScalewayServer_SecurityGroup
--- PASS: TestAccScalewayServer_SecurityGroup (227.32s)
=== RUN   TestGetSSHKeyFingerprint
--- PASS: TestGetSSHKeyFingerprint (0.00s)
=== RUN   TestAccScalewaySSHKey_Basic
--- PASS: TestAccScalewaySSHKey_Basic (32.16s)
=== RUN   TestAccScalewayToken_Basic
--- PASS: TestAccScalewayToken_Basic (34.44s)
=== RUN   TestAccScalewayToken_Expiry
--- PASS: TestAccScalewayToken_Expiry (19.44s)
=== RUN   TestAccScalewayUserData_Basic
--- PASS: TestAccScalewayUserData_Basic (25.95s)
=== RUN   TestAccScalewayVolumeAttachment_Basic
--- PASS: TestAccScalewayVolumeAttachment_Basic (160.50s)
=== RUN   TestAccScalewayVolume_Basic
--- PASS: TestAccScalewayVolume_Basic (9.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-scaleway/scaleway	1493.039s

@meyskens
Copy link
Contributor

👍

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor 🤔 we should think about but this otherwise looks good 👍🏼

ForceNew: true,
Description: "The boot_type of the server",
ValidateFunc: validateBootType,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this value would have been set to “bootscript” in the past, I think we can remove this in favour of a default value or a State migration which sets a default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Infact - is this computed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

come to think about it, Computed and Optional I'd say. I'll adjust accordingly.

Optional: true,
ForceNew: true,
Description: "The boot_type of the server",
ValidateFunc: validateBootType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a built-in validation function provided by Terraform Core that we can use here:

validation.StringInSlice([]string{
“bootscript”,
“local”,
}, true)

Where the true stands for case sensitive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! didn't know about this.

@nicolai86
Copy link
Contributor Author

ty for the review @tombuildsstuff . I've updated the PR accordingly.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment but this otherwise LGTM 👍

@@ -34,6 +34,7 @@ The following arguments are supported:
* `image` - (Required) base image of server
* `type` - (Required) type of server
* `bootscript` - (Optional) server bootscript
* `boot_type` - (Optional) server boot_type (`local`, `bootscript`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth making this the boot mechanism for this server. Possible values include bootscriptandlocal`

@nicolai86 nicolai86 merged commit ee51d80 into master May 6, 2018
@nicolai86 nicolai86 deleted the boot-type branch May 6, 2018 05:25
@remyleone remyleone added the instance Instance issues, bugs and feature requests label Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement instance Instance issues, bugs and feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants