New resource: azurerm_managed_lustre_file_system_auto_export_job#29908
New resource: azurerm_managed_lustre_file_system_auto_export_job#29908jiaweitao001 wants to merge 20 commits intohashicorp:mainfrom
azurerm_managed_lustre_file_system_auto_export_job#29908Conversation
d1f4c26 to
5ce6138
Compare
WodansSon
left a comment
There was a problem hiding this comment.
@jiaweitao001, this is looking good so far, just a few comments I have left which I think should be addressed. Outside of those this LGTM! 🚀
| Type: pluginsdk.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: validation.StringIsNotEmpty, |
There was a problem hiding this comment.
Could we have a more robust validation for the name here? The documentation mentions the following requierments for the name field:
- minLength: 2
- maxLength: 80
- pattern: ^[0-9a-zA-Z][-0-9a-zA-Z_]{0,78}[0-9a-zA-Z]$
There was a problem hiding this comment.
Sure, will fix.
| "auto_export_prefixes": { | ||
| Type: pluginsdk.TypeList, | ||
| Required: true, | ||
| Elem: &pluginsdk.Schema{ | ||
| Type: pluginsdk.TypeString, | ||
| ValidateFunc: validation.StringIsNotEmpty, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Number of maximum allowed paths for now is 1, so we should enforce that in the schema. Since this is an array of blob paths/prefixes which defaults to / can we also add a better validation function here as well?
| "auto_export_prefixes": { | |
| Type: pluginsdk.TypeList, | |
| Required: true, | |
| Elem: &pluginsdk.Schema{ | |
| Type: pluginsdk.TypeString, | |
| ValidateFunc: validation.StringIsNotEmpty, | |
| }, | |
| }, | |
| "auto_export_prefixes": { | |
| Type: pluginsdk.TypeList, | |
| Required: true, | |
| MaxItems: 1, | |
| Elem: &pluginsdk.Schema{ | |
| Type: pluginsdk.TypeString, | |
| ValidateFunc: validation.StringIsNotEmpty, | |
| }, | |
| }, |
| type ManagedLustreFileSystemAutoExportJobResource struct{} | ||
|
|
||
| func (r ManagedLustreFileSystemAutoExportJobResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) { | ||
| id, err := autoexportjobs.ParseAutoExportJobID(state.ID) |
There was a problem hiding this comment.
Can we add some more tests here, at least a requiresImport and an update?
| * `auto_export_prefixes` - (Required) A list of prefixes that get auto exported to the cluster namespace. | ||
|
|
||
| * `admin_status` - (Optional) The administrative status of the Auto Export Job. Possible values are `Enable` and `Disable`. Defaults to `Enable`. | ||
|
|
There was a problem hiding this comment.
I think we need to also add the Attributes Reference here as well, WDYT?
| ## Attributes Reference | |
| In addition to the arguments listed above - the following attributes are exported: | |
| * `id` - The Azure Managed Lustre File System Auto Export Job ID. | |
There was a problem hiding this comment.
Sure. Will add .
0a40211 to
c105ea4
Compare
catriona-m
left a comment
There was a problem hiding this comment.
Thanks @jiaweitao001, it looks like there are test failures on this in tc, could you take a look please?
------- Stdout: -------
=== RUN TestAccManagedLustreFileSystemExportJob_requiresImport
=== PAUSE TestAccManagedLustreFileSystemExportJob_requiresImport
=== CONT TestAccManagedLustreFileSystemExportJob_requiresImport
testcase.go:173: Step 1/2 error: Error running apply: exit status 1
Error: creating Aml Filesystem (Subscription: "*******"
Resource Group Name: "acctestRG-amlfs-250724040016680629"
Aml Filesystem Name: "acctest-amlfs-250724040016680629"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with error: BadRequest: Storage Account acctestsa0bdzx is not found or is inaccessible.
with azurerm_managed_lustre_file_system.test,
on terraform_plugin_test.tf line 147, in resource "azurerm_managed_lustre_file_system" "test":
147: resource "azurerm_managed_lustre_file_system" "test" {
creating Aml Filesystem (Subscription: "*******"
Resource Group Name: "acctestRG-amlfs-250724040016680629"
Aml Filesystem Name: "acctest-amlfs-250724040016680629"): performing
CreateOrUpdate: unexpected status 400 (400 Bad Request) with error:
BadRequest: Storage Account acctestsa0bdzx is not found or is inaccessible.
--- FAIL: TestAccManagedLustreFileSystemExportJob_requiresImport (264.50s)
FAIL
|
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
sreallymatt
left a comment
There was a problem hiding this comment.
Thanks @jiaweitao001, I've left some comments inline
| "resource_group_name": commonschema.ResourceGroupName(), | ||
|
|
||
| "aml_file_system_name": { | ||
| Type: pluginsdk.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| ValidateFunc: validate.ManagedLustreFileSystemName, | ||
| }, |
There was a problem hiding this comment.
When there are multiple properties required to create the parent resource's ID, we generally expose it as a single resource ID property, e.g.
"managed_lustre_file_system_id": commonschema.ResourceIDReferenceRequiredForceNew(&amlfilesystems.AmlFilesystemId{}),
There was a problem hiding this comment.
Sure. Will address this.
| "auto_export_prefixes": { | ||
| Type: pluginsdk.TypeList, | ||
| Required: true, | ||
| MaxItems: 1, |
There was a problem hiding this comment.
Is MaxItems: 1 correct here? If users can only specify one prefix this should probably be auto_export_prefix and TypeString.
Unless this is meant to be MinItems: 1?
There was a problem hiding this comment.
Right, this should be MinItems. Will fix.
|
|
||
| existing, err := autoExportJobsClient.Get(ctx, id) | ||
| if err != nil && !response.WasNotFound(existing.HttpResponse) { | ||
| return fmt.Errorf("checking for existing Auto Export Job %s: %+v", id, err) |
There was a problem hiding this comment.
| return fmt.Errorf("checking for existing Auto Export Job %s: %+v", id, err) | |
| return fmt.Errorf("checking for existing %s: %+v", id, err) |
| if props.AdminStatus != nil && string(pointer.From(props.AdminStatus)) == string(autoexportjob.AutoExportJobAdminStatusEnable) { | ||
| state.AdminStatusEnabled = true | ||
| } else { | ||
| state.AdminStatusEnabled = false | ||
| } |
There was a problem hiding this comment.
We could simplify this to:
| if props.AdminStatus != nil && string(pointer.From(props.AdminStatus)) == string(autoexportjob.AutoExportJobAdminStatusEnable) { | |
| state.AdminStatusEnabled = true | |
| } else { | |
| state.AdminStatusEnabled = false | |
| } | |
| state.AdminStatusEnabled = pointer.From(props.AdminStatus) == autoexportjobs.AutoExportJobAdminStatusEnable |
| if response.WasNotFound(resp.HttpResponse) { | ||
| return pointer.To(false), nil | ||
| } |
There was a problem hiding this comment.
we generally just return any error encounter in the Exists func
| if response.WasNotFound(resp.HttpResponse) { | |
| return pointer.To(false), nil | |
| } |
| location = azurerm_resource_group.test.location | ||
|
|
||
| auto_export_prefixes = ["/"] | ||
| admin_status_enabled = true |
There was a problem hiding this comment.
Since this defaults to true, could you change it to false for the complete test to ensure we can update this property?
| aml_file_system_name = azurerm_managed_lustre_file_system.test.name | ||
| location = azurerm_resource_group.test.location | ||
|
|
||
| auto_export_prefixes = ["/"] |
There was a problem hiding this comment.
Could you update this value so it's different from the value specified in the basic test?
| location = azurerm_managed_lustre_file_system_auto_export_job.test.location | ||
|
|
||
| auto_export_prefixes = azurerm_managed_lustre_file_system_auto_export_job.test.auto_export_prefixes | ||
| admin_status_enabled = azurerm_managed_lustre_file_system_auto_export_job.test.admin_status_enabled |
There was a problem hiding this comment.
import config should match referenced config (r.basic)
| admin_status_enabled = azurerm_managed_lustre_file_system_auto_export_job.test.admin_status_enabled |
|
|
||
| * `auto_export_prefixes` - (Required) A list of prefixes that get auto exported to the cluster namespace. | ||
|
|
||
| * `admin_status_enabled` - (Optional) Whether the administrative status of the Auto Export Job is enabled. Possible values are `true` and `false`. Defaults to `true`. |
There was a problem hiding this comment.
For booleans we don't need to include the possible values
| * `admin_status_enabled` - (Optional) Whether the administrative status of the Auto Export Job is enabled. Possible values are `true` and `false`. Defaults to `true`. | |
| * `admin_status_enabled` - (Optional) Whether the administrative status of the Auto Export Job is enabled. Defaults to `true`. |
|
|
||
| ## Timeouts | ||
|
|
||
| The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions: |
There was a problem hiding this comment.
We've recently updated the timeout reference url, would you mind changing this to
| The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions: | |
| The `timeouts` block allows you to specify [timeouts](https://developer.hashicorp.com/terraform/language/resources/configure#define-operation-timeouts) for certain actions: |
sreallymatt
left a comment
There was a problem hiding this comment.
Hi @jiaweitao001, it looks like there are some test failures. A couple are due to a separate issue relating to the resource group resource, however there is one that will need to be addressed. Based on the error, it seems auto_export_prefixes may need to be ForceNew:
Error: updating Auto Export Job (Subscription: "*******"
Resource Group Name: "acctestRG-amlfs-251126213709780808"
Aml Filesystem Name: "acctest-amlfs-251126213709780808"
Auto Export Job Name: "acctest-amlfsaej-251126213709780808"): performing CreateOrUpdate: unexpected status 409 (409 Conflict) with error: PropertyChangeNotAllowed: Changing property 'autoExportJob.properties.autoExportPrefixes' is not allowed.
with azurerm_managed_lustre_file_system_auto_export_job.test,
on terraform_plugin_test.tf line 195, in resource "azurerm_managed_lustre_file_system_auto_export_job" "test":
195: resource "azurerm_managed_lustre_file_system_auto_export_job" "test" {
Right. The provided description neglected this. Will fix. |
6d8c40f to
0fc76ed
Compare
The Create function performs three sequential long-running operations (waitForImportJobs, CreateOrUpdateThenPoll, UpdateThenPoll) that can cumulatively exceed the previous 60-minute timeout, causing context deadline exceeded errors when disabling admin status. Increase both Create and Update timeouts from 60 to 120 minutes and update documentation accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Community Note
Description
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_resource- support for thething1property [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the provider.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.