-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_data_factory_integration_runtime_self_hosted #6535
New resource - azurerm_data_factory_integration_runtime_self_hosted #6535
Conversation
I really would like to have this resource. Is there anything I can do to help? I see that it has a |
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.
thanks for this pr @pearcec! overall looks good but i've left some comments inline that should be addressed
}, | ||
}, | ||
}, | ||
Set: resourceArmDataFactoryIntegrationRuntimeSelfHostedRbacAuthorizationHash, |
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'm not sure we need a custom hash function here? 🤔
name := rs.Primary.Attributes["name"] | ||
factoryName := rs.Primary.Attributes["data_factory_name"] | ||
resourceGroup := rs.Primary.Attributes["resource_group_name"] |
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.
could we use a resource specific parse ID function here on the resources's id?
name := rs.Primary.Attributes["name"] | ||
factoryName := rs.Primary.Attributes["data_factory_name"] | ||
resourceGroup, hasResourceGroup := rs.Primary.Attributes["resource_group_name"] |
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.
could we use a resource specific parseID function here?
} | ||
|
||
resource "azurerm_resource_group" "test" { | ||
name = "acctestRG-%d" |
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.
could we use
name = "acctestRG-%d" | |
name = "acctestRG-df-%d" |
} | ||
|
||
resource "azurerm_resource_group" "host" { | ||
name = "acctesthostRG-%d" |
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.
and
name = "acctesthostRG-%d" | |
name = "acctestRG-df-%d-host" |
id, err := azure.ParseAzureResourceID(d.Id()) | ||
if err != nil { | ||
return err | ||
} | ||
resourceGroup := id.ResourceGroup | ||
factoryName := id.Path["factories"] | ||
name := id.Path["integrationruntimes"] |
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.
could we write a parse ID function specific to this resource and use it here like in #5692
id, err := azure.ParseAzureResourceID(d.Id()) | ||
if err != nil { | ||
return err | ||
} | ||
resourceGroup := id.ResourceGroup | ||
factoryName := id.Path["factories"] | ||
name := id.Path["integrationruntimes"] |
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.
and a resource specific parse ID function here
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.
None of these are IDs they are names. So there isn't really any path to parse. Both name and data_factory_name are validated via the schema.
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.
Hi @pearcec
I think Kt was referring to the pattern of a custom parser for the whole resource ID (e.g. /subscriptions/12345678-1234-1234-1234-12345678abc/resourceGroups/exampleResourceGroup/providers/Microsoft.DataFactory/factories/exampleFactoryName/integrationruntimes/exampleIntegrationRuntime
which could be parsed into a custom type, and then accessed as id.FactoryName
and id.Name
- (example here)
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.
ok makes sense, I will take a look.
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.
Ok I added it. I rebased it as well.
SelfHostedIntegrationRuntimeTypeProperties: expandAzureRmDataFactoryIntegrationRuntimeSelfHostedTypeProperties(d), | ||
} | ||
|
||
basicIntegrationRuntime, _ := selfHostedIntegrationRuntime.AsBasicIntegrationRuntime() |
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.
we should be checking the result of the cast for nil
), | ||
}, | ||
|
||
"description": { |
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.
we should place optional properties after required
"auth_key_1": { | ||
Type: schema.TypeString, | ||
Computed: true, | ||
}, | ||
|
||
"auth_key_2": { |
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.
computed properties should go after optional
e744456
to
972720f
Compare
@katbyte Hi - I made some adjustments and committed the changes. There are also some inline comments to the ParseID request. |
Can we expect that this PR will be a part of v2.15.0 milestone? |
I updated the code to address @katbyte changes. |
This resource type supports but a standalone Self-hosted and linking via rbac_authorization. The key_authorization has no documentation other than the REST interface. The Powershell Cmdlet does not make use of it. So it has been left out. A full test is setup and necessary for the linked environment to work. An example and documetnation is included to make the linked setup easier to understand. People may also reference the test case. Fixing issue hashicorp#6503.
- Adjust schema for accuracy and ordering - Clarify response from expandAzureRmDataFactoryIntegrationRuntimeSelfHostedTypeProperties - Adjust test names - Rename files to match current refactor
972720f
to
733cea7
Compare
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.
Hi @pearcec - thanks for the updates. Overall this looks good now aside from using the custom parse ID (which we can then add to resource import to check the ID there)
...erm/internal/services/datafactory/tests/data_factory_integration_runtime_self_hosted_test.go
Outdated
Show resolved
Hide resolved
...erm/internal/services/datafactory/tests/data_factory_integration_runtime_self_hosted_test.go
Outdated
Show resolved
Hide resolved
...erm/internal/services/datafactory/tests/data_factory_integration_runtime_self_hosted_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/datafactory/data_factory_integration_runtime_self_hosted.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/datafactory/data_factory_integration_runtime_self_hosted.go
Outdated
Show resolved
Hide resolved
This has been released in version 2.15.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.15.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
This resource type supports but a standalone Self-hosted and linking via rbac_authorization. The
key_authorization has no documentation other than the REST interface. The Powershell Cmdlet does
not make use of it. So it has been left out.
A full test is setup and necessary for the linked environment to work.
An example and documetnation is included to make the linked setup easier to understand. People
may also reference the test case.
Fixing issue #6503.