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

Add option in IoTHub to Enable Fallback Route #2764

Merged
merged 28 commits into from
Feb 22, 2019

Conversation

BlueBasher
Copy link
Contributor

@BlueBasher BlueBasher commented Jan 25, 2019

Added the option to specify if the IoTHub fallback route should be enabled or not.
It defaults to enabled since the Azure Portal, CLI or ARM all default to true. This is a change in the behaviour of Terraform since Terraform used to default to disabled.
See also issue #2719

PS. apologies for the multiple commits, first time Go developer and couldn't get the build to run locally.

@ghost ghost added the size/XS label Jan 25, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @BlueBasher,

Thank you for the PR, don't worry about the commits I'll squash on merge 🙂 the main things this PR needs is a test and updating the documentation with the new property.

I also question if we shouldn't be exposing the other properties here with those values as defaults? WDYU

azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
@ghost ghost added size/M documentation and removed size/XS labels Jan 27, 2019
@ghost ghost added size/L and removed size/M labels Jan 27, 2019
@BlueBasher
Copy link
Contributor Author

Hi @katbyte,
FYI: I've finished the changes you requested in the review.

@ghost ghost removed the waiting-response label Jan 28, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @BlueBasher,

Thanks for the updates, i've gone over it once more, most of the comments are pretty minor with the main blocker being the missing nil check.

azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
website/docs/r/iothub.html.markdown Outdated Show resolved Hide resolved
website/docs/r/iothub.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_iothub_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_iothub_test.go Show resolved Hide resolved
azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff modified the milestones: 2.0.0, 1.23.0 Feb 7, 2019
@maxbog
Copy link
Contributor

maxbog commented Feb 18, 2019

@katbyte, @BlueBasher Hi, I am very interested in this PR, will this be included in 1.23.0? Do you need any help with moving it forward?

@katbyte
Copy link
Collaborator

katbyte commented Feb 19, 2019

Hi @maxbog, @BlueBasher,

I am hoping to have it included in 1.23, right now the only thing holding it up is the tests don't pass:

multiple:

------- Stdout: -------
=== RUN   TestAccAzureRMIotHubConsumerGroup_operationsMonitoringEvents
=== PAUSE TestAccAzureRMIotHubConsumerGroup_operationsMonitoringEvents
=== CONT  TestAccAzureRMIotHubConsumerGroup_operationsMonitoringEvents
--- FAIL: TestAccAzureRMIotHubConsumerGroup_operationsMonitoringEvents (334.78s)
    testing.go:538: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: azurerm_iothub.test
          fallback_route.#: "1" => "0"

@katbyte
Copy link
Collaborator

katbyte commented Feb 19, 2019

As well as:

--- FAIL: TestAccAzureRMIotHub_fallbackRoute (62.55s)
    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-190218234111779608" (Resource Group "acctestRG-190218234111779608"): 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":400108,"httpStatusCode":"BadRequest","message":"EndpointName:fallback is not from a valid type. If you contact a support representative please include this correlation identifier: ede0b60b-e576-456b-8896-c12b7b75a936, timestamp: 2019-02-19 07:41:21Z, errorcode: IH400108."}]

Looks like the API may have changed 🤔

@katbyte
Copy link
Collaborator

katbyte commented Feb 19, 2019

@BlueBasher,

I hope you don't mind but i pushed an update to fix the non-empty plan failure. Opted to ignore the default fallback route, another option would be to mark those properties as computed but i'm not sure it makes sense. WDYT?

@BlueBasher
Copy link
Contributor Author

BlueBasher commented Feb 19, 2019

@katbyte
Definitely don't mind, most important thing is we get the functionality merged :)
Your change looks fine to me, think ignoring the default is the best option.

Do we need anymore change now? Or does this also fix the AccTest?

@ghost ghost added size/XXL dependencies and removed size/L labels Feb 19, 2019
@katbyte
Copy link
Collaborator

katbyte commented Feb 19, 2019

@BlueBasher,

I spent some more time trying to figure out why it was 400'ing: updated the API and removed name (it doesn't seem to actually do anything if you set it?) and did a create then update that finally got rid of the 400.

However now it doesn't seem to be actually setting enabled to true so i am going to push what i have for now and pick this up later in the week.

azurerm/resource_arm_iothub.go Outdated Show resolved Hide resolved
@@ -313,6 +362,11 @@ func resourceArmIotHubCreateUpdate(d *schema.ResourceData, meta interface{}) err
Tags: expandTags(tags),
}

if !d.IsNewResource() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the point of this diff.
When I removed it and set the Properties.Routing.FallbackRoute property to fallbackRoute in the line above, fallback routes started working

@maxbog
Copy link
Contributor

maxbog commented Feb 21, 2019

@katbyte, @BlueBasher I made some minor modifications to make the code work, please look at my comments


fallback_route {
source = "DeviceMessages"
endpoint_names = ["fallback"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the test to pass, this is needed

Suggested change
endpoint_names = ["fallback"]
endpoint_names = ["events"]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that was it! thanks

@katbyte
Copy link
Collaborator

katbyte commented Feb 22, 2019

Thank you very much for pointing us in the right direction @maxbog! tests pass and this LGTM now. Will merge it shortly @BlueBasher

@katbyte katbyte self-requested a review February 22, 2019 00:02
@katbyte katbyte merged commit 9bd7bd8 into hashicorp:master Feb 22, 2019
katbyte added a commit that referenced this pull request Feb 22, 2019
@BlueBasher
Copy link
Contributor Author

@katbyte @maxbog Thanks for all the help to get this merged!

@ghost
Copy link

ghost commented Mar 8, 2019

This has been released in version 1.23.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.23.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 24, 2019

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!

@ghost ghost locked and limited conversation to collaborators Mar 24, 2019
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.

4 participants