New Resource: `azurerm_iot_time_series_insights_access_policy` #7202

mbfrahry · 2020-06-04T02:57:37Z

--- PASS: TestAccAzureRMIoTTimeSeriesInsightsAccessPolicy_basic (140.50s)
--- PASS: TestAccAzureRMIoTTimeSeriesInsightsAccessPolicy_update (178.49s)

ArcturusZhang

Hi @mbfrahry thanks for this PR! This PR overall looks very good, and I have left some comment inline, please have a look, thanks!

ArcturusZhang · 2020-06-04T03:14:19Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

+				ForceNew: true,
+				ValidateFunc: validation.StringMatch(
+					regexp.MustCompile(`^[-\w\._\(\)]+$`),
+					"IoT Time Series Insights Access Policy name must contain only word characters, periods, underscores, and parentheses.",


The regex also says hyphens are allowed, could we please also update the message accordingly? Or please confirm whether the hyphens are allowed?

ArcturusZhang · 2020-06-04T03:14:50Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

+				ForceNew: true,
+				ValidateFunc: validation.StringMatch(
+					regexp.MustCompile(`^[-\w\._\(\)]+$`),
+					"IoT Time Series Insights Environment name must contain only word characters, periods, underscores, and parentheses.",


Same as above

ArcturusZhang · 2020-06-04T03:16:11Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.StringIsNotEmpty,


Since this is an object id, could we add a validation of IsUUID?
And I suppose maybe principal_id is sufficient, since there are a lot of principal_ids in the identity blocks of other resources.

I'm going based off the docs in Azure and the API accepts any string. I'd like to keep it matching with what the API accepts over forcing our own rules.

ArcturusZhang · 2020-06-04T03:23:45Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

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


I think we should not escape 404 errors here. Whether the SDK gives non-nil errors is defined here in the response section of the swagger. For the listed status codes, SDK will not return non-nil errors for them (in this case, they are 200 and 204). And all other status codes, the SDK will return non-nil errors using the error schema defined here.

This means in this case, 404 will always be an error and indicates something is wrong. If this resource does not exist, and we still want to delete this, the service should return 204 for this.

One of the scenario 404 was returned in deletion should be we are deleting this resource, but the resource group does not exist. I believe in this case the service would return 404. But in this case I believe we should call out and throw errors.
What do you think?

ArcturusZhang · 2020-06-04T03:27:24Z

...services/iottimeseriesinsights/tests/resource_iot_time_series_insights_access_policy_test.go

+		if utils.ResponseWasNotFound(resp.Response) {
+			return fmt.Errorf("Bad: Time Series Insights Access Policy %q (resource group: %q) does not exist", id.Name, id.ResourceGroup)
+		}


Could we please move this if statement should move into the above if err != nil statement? Because when the service returns 404, the err will always be non-nil, this means this piece of code is actually unreachable.

ArcturusZhang · 2020-06-04T03:31:14Z

...services/iottimeseriesinsights/tests/resource_iot_time_series_insights_access_policy_test.go

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


I suppose we should guarantee that this function only returns nil when the response code is 404 and return non-nil errors for all the other cases.

ArcturusZhang · 2020-06-04T03:32:43Z

website/docs/r/iot_time_series_insights_access_policy.html.markdown

+Azure IoT Time Series Insights Access Policy can be imported using the `resource id`, e.g.
+
+```shell
+terraform import azurerm_iot_time_series_access_policy.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/example/providers/Microsoft.TimeSeriesInsights/environments/environment1/accessPolicies/example


In the parsing function, we are poping out the name segment using accesspolicies(small p). Therefore could we either modify this example or check if the parsing function is working properly using the small p?

katbyte

aside from two comments LgTM 👍

katbyte · 2020-06-08T16:30:33Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

+
+			"resource_group_name": azure.SchemaResourceGroupName(),
+
+			"environment_name": {


i think this might be more clear is time_series_environment_name?

katbyte · 2020-06-08T16:31:22Z

...m/internal/services/iottimeseriesinsights/resource_iot_time_series_insights_access_policy.go

+	_, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy)
+	if err != nil {


these lines cna be combines

Suggested change

_, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy)

if err != nil {

if _, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy); err != nil {

ghost · 2020-06-11T13:38:56Z

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

ghost · 2020-07-10T14:50:25Z

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!

mbfrahry added 3 commits May 28, 2020 14:49

Start of IoT Time Series Insights Access Policy

110c588

Finish tests

57e942c

Update docs

dd31ba6

ghost added size/XL documentation labels Jun 4, 2020

mbfrahry requested a review from a team June 4, 2020 02:57

ArcturusZhang suggested changes Jun 4, 2020

View reviewed changes

mbfrahry added 2 commits June 4, 2020 12:28

Address review

ecd26e9

Lint

4363ce7

mbfrahry added service/time-series-insights new-resource labels Jun 8, 2020

mbfrahry added this to the v2.14.0 milestone Jun 8, 2020

katbyte approved these changes Jun 8, 2020

View reviewed changes

mbfrahry added 2 commits June 8, 2020 20:30

Merge with master

7ac66e3

Address review

801d7ef

mbfrahry merged commit 273d3bf into master Jun 9, 2020

mbfrahry deleted the f-tsi-access-policy branch June 9, 2020 15:06

mbfrahry added a commit that referenced this pull request Jun 9, 2020

Update changelog for #7202

2feded1

hashicorp locked and limited conversation to collaborators Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_iot_time_series_insights_access_policy` #7202

New Resource: `azurerm_iot_time_series_insights_access_policy` #7202

mbfrahry commented Jun 4, 2020

ArcturusZhang left a comment

ArcturusZhang Jun 4, 2020

ArcturusZhang Jun 4, 2020

ArcturusZhang Jun 4, 2020

mbfrahry Jun 4, 2020

ArcturusZhang Jun 4, 2020

ArcturusZhang Jun 4, 2020

ArcturusZhang Jun 4, 2020

ArcturusZhang Jun 4, 2020

katbyte left a comment

katbyte Jun 8, 2020

katbyte Jun 8, 2020

ghost commented Jun 11, 2020

ghost commented Jul 10, 2020


		"resource_group_name": azure.SchemaResourceGroupName(),

		"environment_name": {

		_, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy)
		if err != nil {

	_, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy)
	if err != nil {
	if _, err := client.CreateOrUpdate(ctx, resourceGroup, environmentName, name, policy); err != nil {

New Resource: azurerm_iot_time_series_insights_access_policy #7202

New Resource: azurerm_iot_time_series_insights_access_policy #7202

Conversation

mbfrahry commented Jun 4, 2020

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

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 11, 2020

ghost commented Jul 10, 2020

New Resource: `azurerm_iot_time_series_insights_access_policy` #7202

New Resource: `azurerm_iot_time_series_insights_access_policy` #7202