-
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
Application Insights Webtests #3331
Conversation
this should now pass. I'd appreciate some input as I think it's probably a bit light on tests Also there was some debate as to how this should be configured - I've gone with straight xml which is what Microsoft go with, and will add this to the documentation, but this could be abstracted away as well.. |
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.
Thank you for the PR @AndyMoore, this is looking pretty good!
Aside from the comments i've left inline a couple things:
- could we add a markdown file for documentation in the website folder?
- could we add some more tests? the
_basic
one sould only be the required properties, the bare minimum required to provision the resource. a_complete
one can also be added and then an_update
that has multiple steps goes from basic -> complete -> basic to check properties update.
thanks!
A test is also failing with:
This means you are trying to set that on read and it is not part of the schema. |
hi @katbyte thanks for the review. I've resolved the individual comments, and will add to the tests over the next few days When I run the basicWeb test it passes locally:
|
updated with tests as requested:
|
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 @AndyMoore
Thanks for pushing those updates - taking a look through this is looking pretty good - if we can fix up the remaining comments this should otherwise be good to merge 👍
Thanks!
tests still passing will get the documents done asap |
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.
By default terraform ignores such schema errors. For our tests we turn on "strict" mode by running the tests with the following environment variable TF_SCHEMA_PANIC_ON_ERROR=1
I've left a comment on the offending line that is trying to set something that doesn't exist in the schema
@katbyte removed |
Approved (forgot to click submit following): Thanks for the updates @AndyMoore. Aside from renaming |
thanks @katbyte |
This has been released in version 1.29.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 = "~> 1.29.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 #1687