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_voice_services_communications_gateway #20607

Merged

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Feb 22, 2023

Support Azure Communications Gateway.

Test Results:
PASS: TestAccVoiceServicesCommunicationsGateway_basic (281.01s)
PASS: TestAccVoiceServicesCommunicationsGateway_requiresImport (309.40s)
PASS: TestAccVoiceServicesCommunicationsGateway_update (671.47s)
PASS: TestAccVoiceServicesCommunicationsGateway_complete (274.35s)

@sinbai sinbai force-pushed the voiceservics/new_resource_communications_gateway branch from 95e2df9 to df4cb60 Compare February 22, 2023 08:43
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 @sinbai, left some comments in-line but once those are resolved we can take another look and get this merged!


"tags": commonschema.Tags(),

"teams_voicemail_pilot_number": {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a number or a string?

Suggested change
"teams_voicemail_pilot_number": {
"microsoft_teams_voicemail_pilot_number": {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The microsoft_teams_voicemail_pilot_number is a string since the type of API is string.

if properties.ApiBridge != nil && *properties.ApiBridge != nil {
apiBridgeValue, err := json.Marshal(*properties.ApiBridge)
if err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return err
return fmt.Errorf("marshalling value for ApiBridge: %+v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 516 to 522
for _, v := range inputList {
input := v
output := communicationsgateways.ServiceRegionProperties{
Name: input.Name,
}

primaryRegionPropertiesValue := expandPrimaryRegionPropertiesModel(input.PrimaryRegionProperties)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for _, v := range inputList {
input := v
output := communicationsgateways.ServiceRegionProperties{
Name: input.Name,
}
primaryRegionPropertiesValue := expandPrimaryRegionPropertiesModel(input.PrimaryRegionProperties)
for _, v := range inputList {
output := communicationsgateways.ServiceRegionProperties{
Name: v.Name,
}
primaryRegionPropertiesValue := expandPrimaryRegionPropertiesModel(v.PrimaryRegionProperties)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 564 to 529
var outputList []ServiceRegionPropertiesModel
if inputList == nil {
return outputList
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var outputList []ServiceRegionPropertiesModel
if inputList == nil {
return outputList
}
outputList := make([]ServiceRegionPropertiesModel, 0)
if inputList == nil {
return outputList
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

func flattenPrimaryRegionPropertiesModel(input *communicationsgateways.PrimaryRegionProperties) []PrimaryRegionPropertiesModel {
var outputList []PrimaryRegionPropertiesModel
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var outputList []PrimaryRegionPropertiesModel
outputList := make([]PrimaryRegionPropertiesModel, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

func flattenCommunicationsPlatformModel(input []communicationsgateways.CommunicationsPlatform) []string {
var output []string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var output []string
output := make([]string, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sinbai
Copy link
Contributor Author

sinbai commented Feb 24, 2023

Hi @stephybun thanks for your time and feedback. Code has been updated. Could you please take another look?

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.

Hi @sinbai, there are still two issues in the schema which have not been resolved from the first review, could you please take a look at them? Thanks!

Comment on lines 568 to 572
"name": {
Type: pluginsdk.TypeString,
Required: true,
ValidateFunc: validation.StringIsNotEmpty,
},
Copy link
Member

Choose a reason for hiding this comment

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

This is still called name this should be called location

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, the code has been updated. Could you please take another look?

Comment on lines 580 to 608
"operator_addresses": {
Type: pluginsdk.TypeSet,
Required: true,
Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
},
},
"allowed_media_source_address_prefixes": {
Type: pluginsdk.TypeSet,
Optional: true,
Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
},
},

"allowed_signaling_source_address_prefixes": {
Type: pluginsdk.TypeSet,
Optional: true,
Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
},
},

"esrp_addresses": {
Type: pluginsdk.TypeSet,
Optional: true,
Elem: &pluginsdk.Schema{
Type: pluginsdk.TypeString,
},
Copy link
Member

Choose a reason for hiding this comment

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

These should be flattened and brought a level up, so under service_locations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, the code has been updated. Could you please take another look?

@sinbai sinbai force-pushed the voiceservics/new_resource_communications_gateway branch from a067553 to ec900b2 Compare March 8, 2023 06:26
@sinbai sinbai force-pushed the voiceservics/new_resource_communications_gateway branch from ec900b2 to c253fb5 Compare March 8, 2023 06:41
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🛻

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

sorry noticed something on my last pass.

codecs = "PCMA"
platforms = ["OperatorConnect", "TeamsPhoneMobile"]

service_locations {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be

Suggested change
service_locations {
service_location {

as its a block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katbyte thanks for the feedback. The code has been updated. Could you please take another look?

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @sinbai - LGTM 🗨️

@katbyte katbyte merged commit b4e3645 into hashicorp:main Mar 23, 2023
@github-actions github-actions bot added this to the v3.49.0 milestone Mar 23, 2023
katbyte added a commit that referenced this pull request Mar 23, 2023
@github-actions
Copy link

This functionality has been released in v3.49.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!

@sinbai sinbai deleted the voiceservics/new_resource_communications_gateway branch March 24, 2023 06:39
@github-actions
Copy link

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 Apr 24, 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.

3 participants