-
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_mobile_network_attached_data_network
; new datasource: azurerm_mobile_network_attached_data_network
#22168
Conversation
…source: `azurerm_mobile_network_attached_data_network` Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
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!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
internal/services/mobilenetwork/mobile_network_attached_data_network_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_data_source.go
Outdated
Show resolved
Hide resolved
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
|
||
func (r AttachedDataNetworkDataSource) Arguments() map[string]*pluginsdk.Schema { | ||
return map[string]*pluginsdk.Schema{ | ||
"mobile_network_data_network_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.
Shall we rename this to name
, or do we want to rename the corresponding field in the model from Name
to MobileNetworkDataNetworkName
to be consistent?
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.
This was marked resolved but I don't see any discussion or explanation, can we have some context?
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.
On reflection, given that this needs to be the same value as the corresponding Mobile Network Data Network's name value, we should in fact use the ID for that here and extract the name from it. This ensures the value is consistent, provides additional validation, and the value can be pulled from a reference allowing an explicit dependency between the resources.
"mobile_network_data_network_name": { | |
"mobile_network_data_network_id": { |
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.
The reason to use name instead of id is the id format of azurerm_mobile_network_attached_data_network
does not contain mobileNetworkName
, the response/model does not contain it either... Then we cannot set the id in read
func
attached_data_network_id:/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.MobileNetwork/packetCoreControlPlanes/{packetCoreControlPlaneName}/packetCoreDataPlanes/{packetCoreDataPlaneName}/attachedDataNetworks/{attachedDataNetworkName}
data_network_id: /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.MobileNetwork/mobileNetworks/{mobileNetworkName}/dataNetworks/{dataNetworkName}
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.
![Screenshot 2023-06-29 at 11 15 13](https://private-user-images.githubusercontent.com/51212351/249662727-13177663-83e7-4f67-801b-930b3bc3fa72.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQxOTAwNzgsIm5iZiI6MTcyNDE4OTc3OCwicGF0aCI6Ii81MTIxMjM1MS8yNDk2NjI3MjctMTMxNzc2NjMtODNlNy00ZjY3LTgwMWItOTMwYjNiYzNmYTcyLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjAlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIwVDIxMzYxOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWY2Y2RjMWFhNmUyZjhhNGRiNTk0OTU5NTlhZWIzMzA3OWY4NTQwZTM5MTZjNTFmOTYxNmY4MzQxMTUwNTAwZjQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.NHBnivX0PNRedYXuQWssxnWIsiMGEQ4XTh_jK2_YiSk)
Here is the dependency between these resources, black arrow means dependency, yellow arrow means id path. As there is a N:1
relation between azurerm_mobile_network_site
and azurerm_mobile_network_packet_core_control_plane
, itt seems there is no way get the id of azurerm_mobile_network_data_network
. WDYT?
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
internal/services/mobilenetwork/mobile_network_attached_data_network_resource.go
Outdated
Show resolved
Hide resolved
website/docs/d/mobile_network_attached_data_network.html.markdown
Outdated
Show resolved
Hide resolved
website/docs/r/mobile_network_attached_data_network.html.markdown
Outdated
Show resolved
Hide resolved
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
@ziyeqf Thanks! LGTM 👍 |
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 @ziyeqf, thanks for this new resource and data source.
Following on from the previous review, I've added some additional comments and a recommended slight design change that should improve the user experience and make things a little more reliable and structured in user configs. If you can take a look, we can continue review.
Thanks!
} | ||
|
||
func (r AttachedDataNetworkDataSource) ModelObject() interface{} { | ||
return &AttachedDataNetworkModel{} |
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.
The model for the data source should be separately defined to the resource model. Whilst they can be the same, they will often contain different properties so we should always start out with a discrete struct for the data source.
%s | ||
|
||
resource "azurerm_mobile_network_attached_data_network" "import" { | ||
mobile_network_data_network_name = azurerm_mobile_network_data_network.test.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.
these values should all be taken as references from the test
resource, for example:
mobile_network_data_network_name = azurerm_mobile_network_data_network.test.name | |
mobile_network_data_network_name = azurerm_mobile_network_attached_data_network.test.name |
|
||
depends_on = [azurerm_mobile_network_data_network.test] |
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.
The dependency is already established here by mobile_network_data_network_name
.
depends_on = [azurerm_mobile_network_data_network.test] |
|
||
depends_on = [azurerm_mobile_network_data_network.test] |
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.
The dependency is already established here by mobile_network_data_network_name
.
depends_on = [azurerm_mobile_network_data_network.test] |
internal/services/mobilenetwork/mobile_network_attached_data_network_resource_test.go
Outdated
Show resolved
Hide resolved
* `create` - (Defaults to 180 minutes) Used when creating the Mobile Network Attached Data Network. | ||
* `read` - (Defaults to 5 minutes) Used when retrieving the Mobile Network Attached Data Network. | ||
* `update` - (Defaults to 180 minutes) Used when updating the Mobile Network Attached Data Network. | ||
* `delete` - (Defaults to 180 minutes) Used when deleting the Mobile Network Attached Data Network. |
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.
This is a data source, only read timeouts apply here
* `create` - (Defaults to 180 minutes) Used when creating the Mobile Network Attached Data Network. | |
* `read` - (Defaults to 5 minutes) Used when retrieving the Mobile Network Attached Data Network. | |
* `update` - (Defaults to 180 minutes) Used when updating the Mobile Network Attached Data Network. | |
* `delete` - (Defaults to 180 minutes) Used when deleting the Mobile Network Attached Data Network. | |
* `read` - (Defaults to 5 minutes) Used when retrieving the Mobile Network Attached Data Network. |
|
||
## Import | ||
|
||
Mobile Network Attached Data Network can be imported using the `resource id`, e.g. |
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.
Imports don't apply to data sources
## Import | |
Mobile Network Attached Data Network can be imported using the `resource id`, e.g. |
key = "value" | ||
} | ||
|
||
depends_on = [azurerm_mobile_network_data_network.example] |
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.
again, this was marked as resolved with no explanation, can we have context here?
|
||
func (r AttachedDataNetworkDataSource) Arguments() map[string]*pluginsdk.Schema { | ||
return map[string]*pluginsdk.Schema{ | ||
"mobile_network_data_network_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.
On reflection, given that this needs to be the same value as the corresponding Mobile Network Data Network's name value, we should in fact use the ID for that here and extract the name from it. This ensures the value is consistent, provides additional validation, and the value can be pulled from a reference allowing an explicit dependency between the resources.
"mobile_network_data_network_name": { | |
"mobile_network_data_network_id": { |
"mobile_network_data_network_name": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.StringIsNotEmpty, | ||
}, |
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.
As above, this should be the ID of the related resource:
"mobile_network_data_network_name": { | |
Type: pluginsdk.TypeString, | |
Required: true, | |
ForceNew: true, | |
ValidateFunc: validation.StringIsNotEmpty, | |
}, | |
"mobile_network_data_network_id": { | |
Type: pluginsdk.TypeString, | |
Required: true, | |
ForceNew: true, | |
ValidateFunc: datanetwork.ValidateDataNetworkID, | |
}, |
From this we can extract the correct name
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
kindly ping for review |
Signed-off-by: ziyeqf <51212351+ziyeqf@users.noreply.github.com>
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.
LGTM ☎️
just to confirm are we ready to merge or waiting for another look? |
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. |
overall
this is the second to the last PR, we are almost there.
test