-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Fixes to IoTHub diff algorithm #2951
Fixes to IoTHub diff algorithm #2951
Conversation
@tombuildsstuff Is there anyone that can review the changes? One of the things this fixes is secret leakage to the console. Right now, connection strings with secret access keys are shown in plain text while printing the execution plan. |
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 @maxbog,
Thank you for the PR,
There is alot going on in here so ideally this would have been a bit better split up into a couple PRs so we could have merged the obvious ones such as marking the connection string as sensitive.
I am struggling to understand the need for the suppressIfTypeIsNot
calls, could you elaborate upon them? Aside from those the rest of the PR LGTM 🙂
@katbyte Thanks for the review, I extracted the obvious parts to a different PR: #3007 As for the For example when I defined the following simple endpoint: endpoint {
type = "AzureIotHub.EventHub"
connection_string = "${azurerm_eventhub_authorization_rule.device_messages_iothubroutes.primary_connection_string}"
name = "device-messages"
} The second apply, which should not show any changes, actually shows this:
Of course, this does not actually result in any changes being made, so the diff output is incorrect. The approach adopted here with the However, please do not merge this PR for now, as I think there may be an issue with |
98bd1df
to
91d4c08
Compare
91d4c08
to
a4b3f25
Compare
@katbyte I believe the PR can be merged now, I changed the diff supression logic a bit for |
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 splitting it out @maxbog, we'll get the other one merged shortly, however this one is currently failing on the tests with:
------- Stdout: -------
=== RUN TestAccAzureRMIotHub_customRoutes
=== PAUSE TestAccAzureRMIotHub_customRoutes
=== CONT TestAccAzureRMIotHub_customRoutes
--- FAIL: TestAccAzureRMIotHub_customRoutes (97.26s)
testing.go:538: Step 0 error: Error applying: 1 error occurred:
* azurerm_iothub.test: 1 error occurred:
* azurerm_iothub.test: Error creating/updating IotHub "acctestIoTHub-190306190727160725" (Resource Group "acctestRG-190306190727160725"): devices.IotHubResourceClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="Failed" Message="The async operation failed." InnerError={"unmarshalError":"json: cannot unmarshal number into Go struct field serviceError2.code of type string"} AdditionalInfo=[{"code":400116,"httpStatusCode":"BadRequest","message":"endpointName:export, exceptionMessage:The argument must not be empty string.\r\nParameter name: containerName. If you contact a support representative please include this correlation identifier: 4b1e7873-fa43-4ef9-9d1c-6e09d146e04f, timestamp: 2019-03-06 19:08:04Z, errorcode: IH400116."}]
Also check whether EventHub endpoint works
010901f
to
0ad81ea
Compare
@katbyte: Any comments? |
Apologies for the delay @maxbog, my attention was elsewhere. Tests now pass and so this LGTM 🚀 |
This has been released in version 1.24.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.24.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 👉 hashibot-feedback@hashicorp.com. Thanks! |
Specific fixes:
batch_frequency_in_seconds
,max_chunk_size_in_bytes
,encoding
,container_name
,file_name_format
are only applicable to endpoints of typeAzureIotHub.StorageContainer