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_bot #13832

Closed
wants to merge 10 commits into from
Closed

New Resource: azurerm_bot #13832

wants to merge 10 commits into from

Conversation

howlowck
Copy link
Contributor

@howlowck howlowck commented Oct 21, 2021

Issue

fixes #12522

Changes

  • Added a new resource azurerm_bot

Tests:

==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/bot -run=TestAccBot_ -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccBot_basic
--- PASS: TestAccBot_basic (88.49s)
=== RUN   TestAccBot_update
--- PASS: TestAccBot_update (246.73s)
=== RUN   TestAccBot_complete
--- PASS: TestAccBot_complete (299.36s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/bot   634.597s

Addressing Potential Questions

  • This looks like it's a carbon copy of azurerm_bot_web_app with minor changes. azurern_bot_web_app has a hard-coded kind="sdk", why not just make the kind in the azurerm_bot_web_app resource configurable?

    The Web App Bot is a specific Terraform resource. This PR could be a lot smaller if I just made the kind configurable in azure_bot_web_app. However it would dilute the intent of the Terraform resource. Creating an azure_bot_web_app with kind="azurebot", will not longer make a Web App Bot, but an "Azure Bot". Additionally, there are conditionals that are specific to web app bot which could lead to compatibilities or regression issues if removed.

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 for this PR @howlowck. Overall this look good, there are just a few suggestions regarding the error messages. However I recently stumbled across another generic resource which should actually have been split into a base resource + a resource for each scope. This is done because of the way the Azure API behaves and for UX reasons. Before approving this I'd like to check internally first whether that would apply here too.

internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource_test.go Outdated Show resolved Hide resolved
internal/services/bot/bot_resource.go Outdated Show resolved Hide resolved
@stephybun
Copy link
Member

Hi @howlowck, this is indeed a case where this resource should be split into a base resource and a resource with tests for
each kind. Below is an example from the provider which shows what that looks like:

More information on why we take this approach can be found in this issue comment.

Since the example I provided is also a typed resource (it may not look as familiar), I'm happy to take a look at this and to provide the base resource. You are of course more than welcome to have a crack at this too.

Let me know what you think.

Thanks!

@katbyte katbyte modified the milestones: v2.85.0, v2.86.0 Nov 12, 2021
@katbyte katbyte modified the milestones: v2.86.0, v2.87.0 Nov 19, 2021
@jackofallops jackofallops modified the milestones: v2.87.0, v2.88.0 Nov 26, 2021
@katbyte katbyte removed this from the v2.88.0 milestone Nov 26, 2021
@howlowck
Copy link
Contributor Author

Hi @stephybun! Thank you for your thoughtful response, and sorry for the late reply (I was on vacation).

Responding to your suggestion:

Since the example I provided is also a typed resource (it may not look as familiar), I'm happy to take a look at this and to provide the base resource. You are of course more than welcome to have a crack at this too.

I'd love it if you can take a look and provide a base resource, and I'd be happy to then build additional resources if necessary.

Thanks again!

@stephybun
Copy link
Member

Closing in favour of #14462

@stephybun stephybun closed this Dec 3, 2021
@howlowck howlowck deleted the feature/add-bot-resource branch December 3, 2021 17:57
@github-actions
Copy link

github-actions bot commented Jan 3, 2022

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 Jan 3, 2022
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.

Support for new Azure Bot Service
4 participants