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

New Resource: azurerm_lab_service_plan #19312

Merged
merged 21 commits into from
Dec 16, 2022

Conversation

neil-yechenwei
Copy link
Contributor

@neil-yechenwei neil-yechenwei commented Nov 16, 2022

This PR is to support new resource azurerm_lab_service_lab_plan.

image

@neil-yechenwei
Copy link
Contributor Author

@stephybun , thanks for your comment. I've updated code. Please take another look. Thanks.

@neil-yechenwei neil-yechenwei changed the title New Resource: azurerm_lab_service_lab_plan New Resource: azurerm_lab_service_plan Dec 13, 2022
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Test failure

------- Stdout: -------
=== RUN   TestAccLabServicePlan_update
=== PAUSE TestAccLabServicePlan_update
=== CONT  TestAccLabServicePlan_update
=== CONT  TestAccLabServicePlan_update
testcase.go:110: Step 3/4 error: Error running apply: exit status 1
Error: updating Lab Plan (Subscription: "*******"
Resource Group Name: "acctestRG-lslp-221213171606146854"
Lab Plan Name: "acctest-lslp-221213171606146854"): performing CreateOrUpdate: labplan.LabPlanClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="ValidationError" Message="The request is not valid." Details=[{"additionalInfo":[],"details":[],"message":"Property value must be a subset of supported regions: australiaeast, australiasoutheast, brazilsouth, canadacentral, canadaeast, centralindia, centralus, eastasia, eastus, eastus2, eastus2euap, francecentral, germanywestcentral, japaneast, koreacentral, northcentralus, northeurope, norwayeast, southafricanorth, southcentralus, southeastasia, switzerlandnorth, uaenorth, uksouth, ukwest, westcentralus, westeurope, westus","target":"properties.allowedRegions[1]"}] AdditionalInfo=[]
with azurerm_lab_service_plan.test,
on terraform_plugin_test.tf line 69, in resource "azurerm_lab_service_plan" "test":
69: resource "azurerm_lab_service_plan" "test" {
updating Lab Plan (Subscription: "*******"
Resource Group Name: "acctestRG-lslp-221213171606146854"
Lab Plan Name: "acctest-lslp-221213171606146854"): performing CreateOrUpdate:
labplan.LabPlanClient#CreateOrUpdate: Failure sending request: StatusCode=0
-- Original Error: Code="ValidationError" Message="The request is not valid."
Details=[{"additionalInfo":[],"details":[],"message":"Property value must be
a subset of supported regions: australiaeast, australiasoutheast,
brazilsouth, canadacentral, canadaeast, centralindia, centralus, eastasia,
eastus, eastus2, eastus2euap, francecentral, germanywestcentral, japaneast,
koreacentral, northcentralus, northeurope, norwayeast, southafricanorth,
southcentralus, southeastasia, switzerlandnorth, uaenorth, uksouth, ukwest,
westcentralus, westeurope, westus","target":"properties.allowedRegions[1]"}]
AdditionalInfo=[]
--- FAIL: TestAccLabServicePlan_update (276.43s)

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Dec 14, 2022

@stephybun , seems the secondary test location in your test environment is incorrect. Could you check if your secondary test location is correct? If not, please rerun test case after the secondary test location is set correctly in your test environment. Thanks.

@stephybun
Copy link
Member

It's running in teamcity with the following values for locations
image
Tests passed on 5th Dec using the same values and now they're failing.

@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented Dec 14, 2022

Hi @stephybun , not all locations support lab service. So I limited the test location in service.kt(https://github.com/hashicorp/terraform-provider-azurerm/pull/19312/files#diff-c9a4f5ece9fe921f274cae0220ba39a13027c503c6bd80e3cc9ee926565a5c94) for Teamcity. But seems the change in service.kt doesn't take effect in Teamcity. May I ask when the change in service.kt would take effect? If the change in service.kt would take effect after PR merge, I assume test case failing is expected.

Hence, for now I tried to set env variable env.ARM_PROVIDER_DYNAMIC_TEST to false in Teamcity and all TCs passed. For example, TCs can pass on 5th Dec since this run is with this additional env variable. I just now reran TCs with this variable in Teamcity and all TCs passed.

image

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Have another comment on the schema while I was going through the docs.

Comment on lines 61 to 71
* `disconnect_delay` - (Required) The amount of time a VM will stay running after a user disconnects if this behavior is enabled. This value must be formatted as an ISO 8601 string.

* `idle_delay` - (Required) The amount of time a VM will idle before it is shutdown if this behavior is enabled. This value must be formatted as an ISO 8601 string.

* `no_connect_delay` - (Required) The amount of time a VM will stay running before it is shutdown if no connection is made and this behavior is enabled. This value must be formatted as an ISO 8601 string.

* `shutdown_on_disconnect_enabled` - (Required) Is shutdown on disconnect enabled? Possible values are `true` and `false`.

* `shutdown_when_not_connected_enabled` - (Required) Will a VM get shutdown when it hasn't been connected to after a period of time? Possible values are `true` and `false`.

* `shutdown_on_idle` - (Optional) Will a VM get shutdown when it has idled for a period of time? Possible values are `LowUsage` and `UserAbsence`.
Copy link
Member

Choose a reason for hiding this comment

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

disconnect_delay, idle_delay and no_connect_delay only come into effect when their corresponding flag is set to true i.e.shutdown_on_disconnect_enabled, shutdown_when_not_connected_enabled and shutdown_on_idle which seems redundant and like a bit of a janky user experience.

disconnect_delay, idle_delay and no_connect_delay should become Optional, and if they are set then they are enabled, if they are absent then they are disabled instead of having to specify that with an additional property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stephybun , yes, you're right. For shutdown_on_disconnect_enabled and shutdown_when_not_connected_enabled, I've removed them to improve user experience. For shutdown_on_idle, we may not remove it since it's enum value.

The test result after rerun for this new update:
image

@katbyte
Copy link
Collaborator

katbyte commented Dec 15, 2022

@neil-yechenwei - settings.kt will only take affect once the PR is merged into main, until then its required to manually change the params on the build

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @neil-yechenwei LGTM 🥟

@stephybun stephybun merged commit dace4ca into hashicorp:main Dec 16, 2022
stephybun added a commit that referenced this pull request Dec 16, 2022
@stephybun stephybun added this to the v3.37.0 milestone Dec 16, 2022
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

This functionality has been released in v3.37.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

favoretti pushed a commit to favoretti/terraform-provider-azurerm that referenced this pull request Jan 12, 2023
* New Resource: azurerm_lab_service_lab_plan

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update code

* update tc

* update code

* update code

* update code
favoretti pushed a commit to favoretti/terraform-provider-azurerm that referenced this pull request Jan 12, 2023
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants