New Resource: `azurerm_iot_time_series_insights_standard_environment` #7012

mbfrahry · 2020-05-19T22:08:13Z

--- PASS: TestAccAzureRMTimeSeriesInsightsEnvironment_basic (139.73s)
--- PASS: TestAccAzureRMTimeSeriesInsightsEnvironment_complete (301.78s)

Fixes #3844

ArcturusZhang

Hi @mbfrahry thanks for this PR!

Mostly this looks really good 👍 , aside from some comments I left inline.

ArcturusZhang · 2020-05-21T06:14:12Z

azurerm/internal/services/timeseriesinsights/parse/time_series_insights_environment.go

+func TimeSeriesInsightsEnvironmentID(input string) (*TimeSeriesInsightsEnvironmentId, error) {
+	id, err := azure.ParseAzureResourceID(input)
+	if err != nil {
+		return nil, fmt.Errorf("[ERROR] Unable to parse Time Series Insights Environment ID %q: %+v", input, err)


Could we please modify this error message a bit to make it to look less like a log rather than an error message?

Suggested change

return nil, fmt.Errorf("[ERROR] Unable to parse Time Series Insights Environment ID %q: %+v", input, err)

return nil, fmt.Errorf("parsing Time Series Insights Environment ID %q: %+v", input, err)

Good catch! Done!

ArcturusZhang · 2020-05-21T06:16:38Z

azurerm/internal/services/timeseriesinsights/resource_time_series_insights_environment.go

+					regexp.MustCompile(`^[-\w\._\(\)]+$`),
+					"Time Series Insights Environment name must be 1 - 90 characters long, contain only word characters and underscores.",


This regex does not really match the error message below. For instance this regex does not have a constraint of length, and the regex also allows braces. Which one is expected? We should at least modify one of these.

ArcturusZhang · 2020-05-21T06:20:20Z

azurerm/internal/services/timeseriesinsights/resource_time_series_insights_environment.go

+		Importer: azSchema.ValidateResourceIDPriorToImport(func(id string) error {
+			_, err := parse.TimeSeriesInsightsEnvironmentID(id)
+			return err
+		}),


Based on the content below, this resource is a polymorphic one. Since different types of this resource share the same ID, could we add a bit more validation here to ensure the user is importing the ID to the right resource?

ArcturusZhang · 2020-05-21T06:22:10Z

azurerm/internal/services/timeseriesinsights/resource_time_series_insights_environment.go

+	if resource.ID == nil {
+		return fmt.Errorf("cannot read Time Series Insights Environment %q (Resource Group %q) ID", name, resourceGroup)
+	}


Could we add a check to ensure the ID is not nil and not empty?

Suggested change

if resource.ID == nil {

return fmt.Errorf("cannot read Time Series Insights Environment %q (Resource Group %q) ID", name, resourceGroup)

}

if resource.ID == nil || *resource.ID == "" {

return fmt.Errorf("cannot read Time Series Insights Environment %q (Resource Group %q) ID", name, resourceGroup)

}

ArcturusZhang · 2020-05-21T06:24:39Z

azurerm/internal/services/timeseriesinsights/resource_time_series_insights_environment.go

+		if !utils.ResponseWasNotFound(response) {
+			return fmt.Errorf("deleting Time Series Insights Environment %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err)


I suppose if the service returns 404 when you are deleting the resource, it is still something wrong happening. Typically if you are attempting to delete a non-existing resource, the service should return 204

ArcturusZhang · 2020-05-21T06:31:48Z

...internal/services/timeseriesinsights/tests/resource_time_series_insights_environment_test.go

+		if err != nil {
+			return nil
+		}
+
+		if resp.StatusCode != http.StatusNotFound {
+			return fmt.Errorf("Time Series Insights Environment still exists: %q", id.Name)
+		}


This reads really weird. If the resource does not exist, the error would be non-nil, and this check would return nil immediately. But if something is wrong (for instance 429 returned), the error would be also non-nil. In this case, this case would regard this as "the resource does not exist" and return nil.

On the other hand, the if resp.StatusCode != http.StatusNotFound would never succeed. Get function would only return nil error when it gets the resource successfully (in other word 200), in this case the status code would never be 404.
This behaviour is also defined in the swagger here

And to be honest, a lot of the existing tests have this kind of check destroy functions, which I suppose it is not so correct. What do you think?

ArcturusZhang · 2020-05-21T06:34:11Z

...internal/services/timeseriesinsights/tests/resource_time_series_insights_environment_test.go

+			{
+				Config: testAccAzureRMTimeSeriesInsightsEnvironment_basic(data),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMTimeSeriesInsightsEnvironmentExists(data.ResourceName),
+				),
+			},
+			data.ImportStep(),
+			{
+				Config: testAccAzureRMTimeSeriesInsightsEnvironment_complete(data),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMTimeSeriesInsightsEnvironmentExists(data.ResourceName),
+				),
+			},
+			data.ImportStep(),
+			{
+				Config: testAccAzureRMTimeSeriesInsightsEnvironment_basic(data),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMTimeSeriesInsightsEnvironmentExists(data.ResourceName),
+				),
+			},
+			data.ImportStep(),


Does this update test really help? Because in the schema, the tags are defined as ForceNew, and the config for basic does not have a tag, complete config has a tag. Therefore this update would always be a replacement rather than a in-place update.

ArcturusZhang · 2020-05-21T06:36:37Z

website/docs/r/time_series_insights_environment.html.markdown

+
+* `partition_key` - (Optional) The name of the event property which will be used to partition data.
+
+* `tags` - (Optional) A mapping of tags to assign to the resource.


Is the tags ForceNew as defined in the schema? If it is, we should change this description accordingly.

My fault here! Tags now updates without recreating the resource

katbyte

LGTM 👍

ghost · 2020-05-22T00:31:15Z

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

ghost · 2020-06-21T14:50:30Z

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!

mbfrahry added 4 commits March 10, 2020 22:06

Up through testing but investigating a potential bug

87e51aa

Finish off resource

3adf62c

merge with master

faddb5b

Update error messages

300af76

ghost added size/XXL dependencies documentation labels May 19, 2020

mbfrahry requested a review from a team May 19, 2020 22:08

mbfrahry added the service/time-series-insights label May 19, 2020

mbfrahry added this to the v2.11.0 milestone May 19, 2020

mbfrahry added 2 commits May 20, 2020 08:36

linting

a1a59a5

terrafmt

0bd095d

ArcturusZhang suggested changes May 21, 2020

View reviewed changes

Finalizing review

d15bea9

katbyte approved these changes May 21, 2020

View reviewed changes

mbfrahry added 3 commits May 21, 2020 14:26

Add IoT prefix

7aba707

Update prefix

020f924

terrafmt

a4667e7

mbfrahry merged commit 4267a85 into master May 21, 2020

mbfrahry deleted the f-tsi-environments branch May 21, 2020 22:34

mbfrahry changed the title ~~New Resource: azurerm_time_series_insights_environment~~ New Resource: azurerm_iot_time_series_insights_standard_environment May 21, 2020

mbfrahry added a commit that referenced this pull request May 21, 2020

Update changelog for #7012

76fbea8

ghost locked and limited conversation to collaborators Jun 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_iot_time_series_insights_standard_environment` #7012

New Resource: `azurerm_iot_time_series_insights_standard_environment` #7012

mbfrahry commented May 19, 2020 •

edited

Loading

ArcturusZhang left a comment

ArcturusZhang May 21, 2020

mbfrahry May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

ArcturusZhang May 21, 2020

mbfrahry May 21, 2020

katbyte left a comment

ghost commented May 22, 2020

ghost commented Jun 21, 2020

	return nil, fmt.Errorf("[ERROR] Unable to parse Time Series Insights Environment ID %q: %+v", input, err)
	return nil, fmt.Errorf("parsing Time Series Insights Environment ID %q: %+v", input, err)

		regexp.MustCompile(`^[-\w\._\(\)]+$`),
		"Time Series Insights Environment name must be 1 - 90 characters long, contain only word characters and underscores.",

		if !utils.ResponseWasNotFound(response) {
		return fmt.Errorf("deleting Time Series Insights Environment %q (Resource Group %q): %+v", id.Name, id.ResourceGroup, err)


		* `partition_key` - (Optional) The name of the event property which will be used to partition data.

		* `tags` - (Optional) A mapping of tags to assign to the resource.

New Resource: azurerm_iot_time_series_insights_standard_environment #7012

New Resource: azurerm_iot_time_series_insights_standard_environment #7012

Conversation

mbfrahry commented May 19, 2020 • edited Loading

ArcturusZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

ghost commented May 22, 2020

ghost commented Jun 21, 2020

New Resource: `azurerm_iot_time_series_insights_standard_environment` #7012

New Resource: `azurerm_iot_time_series_insights_standard_environment` #7012

mbfrahry commented May 19, 2020 •

edited

Loading