-
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
New Resource: azurerm_bot_channel_directline #5445
New Resource: azurerm_bot_channel_directline #5445
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 @crazedpeanut,
Thanks for the PR, i've given this a quick review and left some comments inline to address
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/tests/resource_arm_bot_channel_directline_test.go
Show resolved
Hide resolved
@katbyte should be ready for another look when you're ready. Hoping I understood your feedback correctly! |
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 the PR @crazedpeanut, overall this is off to a great start! i have left some mostly minor comments inline that need to be addressed before merge.
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/resource_arm_bot_channel_directline.go
Outdated
Show resolved
Hide resolved
Ready for review |
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.
Hey @crazedpeanut, I've taken a look through the rest of the PR and things look generally good but the tests are failing. I've also left some minor nitpicks around formatting
We also want to add the doc to the sidebar here.
The docs are formatted slightly off as well. We have a nifty tool to get that done https://github.com/katbyte/terrafmt but I've also left details for what to look out for.
And then lastly, I'm seeing test failures for directlineUpdate
and directlineComplete
that warrant taking another look.
--- FAIL: TestAccAzureRMBotChannelsRegistration/channel/directlineUpdate (130.31s)
testing.go:640: Step 2 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: azurerm_bot_channel_directline.test
bot_name: "acctestdf200211195641757850" => "acctestdf200211195641757850"
id: "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-200211195641757850/providers/Microsoft.BotService/botServices/acctestdf200211195641757850/channels/DirectLineChannel" => "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-200211195641757850/providers/Microsoft.BotService/botServices/acctestdf200211195641757850/channels/DirectLineChannel"
location: "global" => "global"
resource_group_name: "acctestRG-200211195641757850" => "acctestRG-200211195641757850"
site.#: "1" => "1"
site.0.enabled: "true" => "true"
site.0.enhanced_authentication_enabled: "true" => "true"
site.0.id: "" => "<computed>"
site.0.key: "G9aNDsh4ihA.C9PEVAG1qxzzv6sevABcShLgX1iCSiHGCnrTcNk0djc" => "<computed>"
site.0.key2: "G9aNDsh4ihA.3vkIz-lIM6RUfalAIf1HjI6jKSgnIlfRVPgDeoHrJAg" => "<computed>"
site.0.name: "test" => "test"
site.0.trusted_origins.#: "0" => "1"
site.0.trusted_origins.0: "" => "https://example.com"
site.0.v1_allowed: "true" => "true"
site.0.v3_allowed: "true" => "true"
--- FAIL: TestAccAzureRMBotChannelsRegistration/channel/directlineComplete (80.63s)
testing.go:640: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: azurerm_bot_channel_directline.test
bot_name: "acctestdf200211195521114466" => "acctestdf200211195521114466"
id: "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-200211195521114466/providers/Microsoft.BotService/botServices/acctestdf200211195521114466/channels/DirectLineChannel" => "/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-200211195521114466/providers/Microsoft.BotService/botServices/acctestdf200211195521114466/channels/DirectLineChannel"
location: "global" => "global"
resource_group_name: "acctestRG-200211195521114466" => "acctestRG-200211195521114466"
site.#: "1" => "1"
site.0.enabled: "true" => "true"
site.0.enhanced_authentication_enabled: "false" => "true"
site.0.id: "" => "<computed>"
site.0.key: "IWQnH2hrqpc.eeNEIEpjSGuVskN43krr03EklDcia-g8_oQC_St60g4" => "<computed>"
site.0.key2: "IWQnH2hrqpc.D_jJS_Qu4OGJ2mis2htOW2UyGgtLq7RkqRhLx427F40" => "<computed>"
site.0.name: "test" => "test"
site.0.trusted_origins.#: "0" => "1"
site.0.trusted_origins.0: "" => "https://example.com"
site.0.v1_allowed: "true" => "true"
site.0.v3_allowed: "true" => "true"
- `is_secure_site_enabled` - (Optional) Enables additional security measures for this site, see [Enhanced Directline Authentication Features](https://blog.botframework.com/2018/09/25/enhanced-direct-line-authentication-features). Disabled by default. | ||
|
||
- `trusted_origins` - (Optional) This field is required when `is_secure_site_enabled` is enabled. Determines which origins can establish a Directline conversation for this site. | ||
|
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.
So we tend to put Computed attributes underneath the ## Attributes Reference
section since. You can remove the (Computed)
portion and move these attributes there.
azurerm/internal/services/bot/tests/resource_arm_bot_channel_directline_test.go
Show resolved
Hide resolved
azurerm/internal/services/bot/tests/resource_arm_bot_channel_directline_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/bot/tests/resource_arm_bot_channel_directline_test.go
Outdated
Show resolved
Hide resolved
@mbfrahry, thanks for the review. I'll take a look |
Ready for review |
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 the revisions @crazedpeanut! LGTM now 👍
This has been released in version 2.1.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.1.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! |
Hi! I've added a new resource to the existing ones.
This is my first time working with Go so I heavily referred to the other to slack and msteams.
Appreciate any feedback!