-
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 r/azurerm.function_app_slot
#6435
Conversation
848ec44
to
06c4b4d
Compare
292c0c1
to
293f029
Compare
It's almost complete, I've a few acc tests still failing. Update: acctests are fixed! Improvements and extension of the acceptance tests are welcome, as I'm not exactly a functional user of Function App deployments slots. For now I've copied the tests mostly from App Service deployment slots, dropped a few and replaced a few for Function App tests.
Update
|
f5047d0
to
4665b59
Compare
Can this be merged in and added to the next release!? @francescopersico |
This is place 4 in the most wanted features of The Azure TerraForm provider and it appears this PR is ready to be merged. I am sure the size of the PR is a factor but can it be merged? @tombuildsstuff |
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 @aristosvo
Thanks for the PR for the new resource. I've commented and requested changes below. I realise that a lot of the schema comments are because you've used the azurerm_function_app
as a basis for this resource a brought a lot of the schema over, but as it's a new resource we'd try to bring the good practices learned elsewhere in the provider along too. (The service has also changed a lot since being added to the provider)
There's a blocking change that will require your branch rebasing against master as there's a reliance on a function in the function_app
resource which has changed (noted below).
Other than this, it's looking good. If you can address these, I'll loop back around asap and re-review
Thanks!
Ste
azurerm/internal/services/web/resource_arm_function_app_slot.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/web/resource_arm_function_app_slot.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/web/tests/resource_arm_function_app_slot_test.go
Show resolved
Hide resolved
azurerm/internal/services/web/tests/resource_arm_function_app_slot_test.go
Show resolved
Hide resolved
968f387
to
c3b5c28
Compare
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 @aristosvo
Thanks for the updates. I discovered a bug in master that would prevent us testing this PR for merge. This has now been fixed, so I've had another quick run through this and there's a couple more things I've spotted, noted below. You should probably rebase against master to pick up the fix I mentioned too if that's OK?
Thanks again,
Ste
azurerm/internal/services/web/resource_arm_function_app_slot.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/web/resource_arm_function_app_slot.go
Outdated
Show resolved
Hide resolved
I'll run the acctests once again and add I hope to finish that tonight, otherwise tomorrow night. Edit: Done
|
Thank you all! We can't wait to start using this! |
This has been released in version 2.9.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.9.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! |
Fixes #1307.
I'll move in a bit more, but started with the basics.