-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
azurerm_windows_web_app
,azurerm_windows_web_app_slot
,azurerm_windows_function_app
,azurerm_windows_function_app_slot
- add push_setting
feature.
#20522
Conversation
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 @xiaxyi
Thanks for this PR. I've left some comments below on the implementation / design of this property and how it's used. Can you take a look?
Thanks!
MaxItems: 1, | ||
Elem: &pluginsdk.Resource{ | ||
Schema: map[string]*pluginsdk.Schema{ | ||
"is_push_enabled": { |
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 presence of this block should be used define this value?
}, | ||
"tags_to_whitelist": { |
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.
}, | |
"tags_to_whitelist": { | |
}, | |
"tags_to_whitelist": { |
tags_requiring_auth = ["dtags1"] | ||
} | ||
app_settings = { | ||
"MS_NotificationHubName" = "${azurerm_notification_hub.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.
I feel this should be a schema property, specifically the ID of the notification hub, and not a value in the app_settings
map so we can validate it, and retrieve the name value from the resource? This will allow terraform to correctly plan the dependencies, and will also better inform users that this is required, rather than expecting users to know this needs adding here?
@xiaxyi - i'm going to close this out as its been sitting here for a number of months, please do reopen it once you are ready to continue working on it. thanks |
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. |
Add push feature for windows apps.
acc tests: