New Resource: azurerm_data_lake_store #1219

paktek123 · 2018-05-09T11:59:43Z

This PR creates a new resource which can be used for create an Azure Data Lake Store.

…aform-provider-azurerm into add-adls-accounts

paktek123 · 2018-05-15T15:33:08Z

Hi @tombuildsstuff any chance of this being reviewed, would be good to see this in 1.6

WodansSon · 2018-05-16T02:26:27Z

azurerm/data_source_data_lake_store.go

+	resp, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			d.SetId("")


Can you add a warning message here that it wasn't found? Something like...
log.Printf("[WARN] DataLakeStoreAccount '%s' was not found (resource group '%s')", name, resourcGroup)

done, in next commit

WodansSon · 2018-05-16T02:34:40Z

azurerm/resource_arm_data_lake_store.go

+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			d.SetId("")
+			fmt.Print("Oh no nothing was found")


Can you format the not found message to something like...
log.Printf("[WARN] DataLakeStore '%s' was not found (resource group '%s')", name, resourcGroup)

done, in next commit

WodansSon · 2018-05-16T02:37:58Z

azurerm/resource_arm_data_lake_store.go

+		if response.WasNotFound(future.Response()) {
+			return nil
+		}
+		return fmt.Errorf("Error issuing delete request for Azure Data Lake Store %q (Resource Group %q): %+v", name, resourceGroup, err)


Can you update this to be just return err?

done, in next commit

WodansSon · 2018-05-16T02:38:40Z

azurerm/resource_arm_data_lake_store.go

+		if response.WasNotFound(future.Response()) {
+			return nil
+		}
+		return fmt.Errorf("Error waiting for Azure Data Lake Store %q (Resource Group %q) to be deleted: %+v", name, resourceGroup, err)


Same here, just return err

done, in next commit

WodansSon · 2018-05-16T02:39:28Z

azurerm/resource_arm_data_lake_store.go

+		return fmt.Errorf("Error waiting for Azure Data Lake Store %q (Resource Group %q) to be deleted: %+v", name, resourceGroup, err)
+	}
+
+	return err


Shouldn't this be return nil?

yes this should be nil, done

WodansSon

Hi paktek123, thanks for the PR we really appreciate it. I've taken a look and it general looks good, but I left a few comments.

paktek123 · 2018-05-16T14:47:14Z

Thank you for reviewing @jeffreyCline, please kindly review again.

katbyte

Hi @paktek123,

Thank you for opening this PR! data lake support has frequently been asked for. I've taken a look and left some comments inline.

Generally this looks good and I look forward to getting this merged for you once these minor issues are resolved 🙂

katbyte · 2018-05-16T22:05:28Z

azurerm/data_source_data_lake_store.go

+		d.Set("location", azureRMNormalizeLocation(*location))
+	}
+
+	if tier := resp.DataLakeStoreAccountProperties; tier != nil {


could we assign this to properties not tier for clarity?

katbyte · 2018-05-16T22:07:14Z

azurerm/data_source_data_lake_store_test.go

+
+func TestAccDataSourceAzureRMDataLakeStore_payasyougo(t *testing.T) {
+	dataSourceName := "data.azurerm_data_lake_store.test"
+	rInt := acctest.RandIntRange(1, 999999)


Could we use acctest.RandInt() like the majority of the other tests?

there is a limitation on the name for an ADLS account which has to be between 3 to 24 characters. I wanted to restrict the length of the name to within that limit. RandInt() generates a larger string if I am not mistaken. I will use random string and revert the resource group name to randInt()

katbyte · 2018-05-16T22:07:25Z

azurerm/data_source_data_lake_store_test.go

+
+func TestAccDataSourceAzureRMDataLakeStore_monthlycommitment(t *testing.T) {
+	dataSourceName := "data.azurerm_data_lake_store.test"
+	rInt := acctest.RandIntRange(1, 999999)


same as above

there is a limitation on the name for an ADLS account which has to be between 3 to 24 characters. I wanted to restrict the length of the name to within that limit. RandInt() generates a larger string if I am not mistaken.I will use random string and revert the resource group name to randInt()

katbyte · 2018-05-16T22:09:17Z

website/docs/r/data_lake_store.html.markdown

+
+The following arguments are supported:
+
+* `name` - (Required) Specifies the name of the Data Lake Store. Changing this forces a


Could we put this all on the same line?

fixed in next commit

katbyte · 2018-05-16T22:10:25Z

azurerm/resource_arm_data_lake_store.go

+		},
+
+		Schema: map[string]*schema.Schema{
+			"name": {


Could we get some validation on this property? the docs say 3-24 so at the very lease we should check that. Ideally we should use regex to also validate the characters.

done in next commit

katbyte · 2018-05-16T22:32:05Z

website/docs/r/data_lake_store.html.markdown

+* `name` - (Required) Specifies the name of the Data Lake Store. Changing this forces a
+    new resource to be created. Has to be between 3 to 24 characters.
+
+* `resource_group_name` - (Required) The name of the resource group in which to


same as above

katbyte · 2018-05-16T22:32:56Z

website/docs/r/data_lake_store.html.markdown

+    location = "northeurope"
+}
+
+resource "azurerm_data_lake_store" "payasyougo" {


Can we change this to consumption rather the payasyougo to better match the tier name in azure?

yup renamed

katbyte · 2018-05-16T22:33:24Z

website/docs/r/data_lake_store.html.markdown

+
+```hcl
+# Pay As You Go
+resource "azurerm_resource_group" "test" {


Can we get the terraform in the example formatted to look nice please?

yup moved to 2 spaces

katbyte · 2018-05-16T22:34:47Z

azurerm/resource_arm_data_lake_store_test.go

+					testCheckAzureRMDataLakeStoreExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "tier", "Commitment_1TB"),
+				),
+			},


Could we get import checks on all these tests?

{ ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

added import checks for all

and removed import file

katbyte · 2018-05-16T22:35:56Z

azurerm/resource_arm_data_lake_store.go

+
+	future, err := client.Create(ctx, resGroup, name, dateLakeStore)
+	if err != nil {
+		return err


Could we wrap these errors? like how you are in the update

future, err := client.Update(ctx, resourceGroup, name, props) if err != nil { return fmt.Errorf("Error issuing update request for Data Lake Store %q (Resource Group %q): %+v", name, resourceGroup, err) }

done for Create

paktek123 · 2018-05-17T14:15:04Z

Thank you for reviewing @katbyte. Please kindly review again.

katbyte

@paktek123,

Thank you for your updates! Its almost there, i've left a few comments inline only one of which is a showstopper: if the tier property on the datasource should be computed or not.

katbyte · 2018-05-19T19:21:19Z

azurerm/data_source_data_lake_store.go

+
+			"tier": {
+				Type:     schema.TypeString,
+				Optional: true,


I believe this should be computed with no default?

yup has to be computed

katbyte · 2018-05-19T19:23:03Z

azurerm/resource_arm_data_lake_store.go

+
+	err = future.WaitForCompletion(ctx, client.Client)
+	if err != nil {
+		return err


Could we wrap this error like above?

wrapped in next commit

katbyte · 2018-05-19T19:23:12Z

azurerm/resource_arm_data_lake_store.go

+
+	read, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		return err


Could we wrap this error like above?

wrapped in next commit

katbyte · 2018-05-19T19:27:34Z

azurerm/resource_arm_data_lake_store.go

+	return nil
+}
+
+func validateArmDataLakeName(v interface{}, k string) (ws []string, es []error) {


Very minor: as it doesn't look like this is used anywhere else you could just use the built in validation helpers:

ValidateFunc: validation.StringMatch( regexp.MustCompile(`\A([a-z0-9]{3,24})\z`), "Name can only consist of lowercase letters and numbers, and must be between 3 and 24 characters long", ),

But I'll merge as is

makes sense, no need for a func that doesnt get used elsewhere

katbyte · 2018-05-19T19:33:05Z

azurerm/resource_arm_data_lake_store.go

+		if response.WasNotFound(future.Response()) {
+			return nil
+		}
+		return err


Could we wrap this error too?

wrapped in next commit

katbyte · 2018-05-19T19:33:10Z

azurerm/resource_arm_data_lake_store.go

+		if response.WasNotFound(future.Response()) {
+			return nil
+		}
+		return err


Could we wrap this error too?

wrapped in next commit

paktek123 · 2018-05-19T21:01:07Z

Thank you for reviewing @katbyte . Please kindly review again.

Fix typo in test

katbyte

Hi @paktek123,

Speedy fixes! Thank you, this LGTM now and tests pass:

[18:37:26] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMDataLakeStore
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMDataLakeStore -timeout 180m
=== RUN   TestAccAzureRMDataLakeStore_basic
--- PASS: TestAccAzureRMDataLakeStore_basic (136.35s)
=== RUN   TestAccAzureRMDataLakeStore_tier
--- PASS: TestAccAzureRMDataLakeStore_tier (134.34s)
=== RUN   TestAccAzureRMDataLakeStore_withTags
--- PASS: TestAccAzureRMDataLakeStore_withTags (147.89s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	418.611s
[18:44:36] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$

All requested changes are outdated

ghost · 2020-03-31T14:58:32Z

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!

Neeran Gul and others added 16 commits May 4, 2018 11:22

adding adls client and account resources

a7a4050

fixing errors

9b4590c

adding provider, sorted order and fixed tests

0f434d8

adding website docs and fixing data source

6d3c508

no clue how this happned

d50f52a

correct letters

56e9a01

only 3 to 24 chars allowed

dd3022f

dont mess with this

5ef2472

add monthly tier

242dfcc

add v16.2.1 for datalake

7e430a4

removing extra stuff

4c487a1

update the correct vendor

16c79f2

Merge branch 'master' into add-adls-accounts

0046546

resolve conflict;

0eb9490

Merge branch 'add-adls-accounts' of https://github.com/paktek123/terr…

e0722f1

…aform-provider-azurerm into add-adls-accounts

was missing a bracket

fe30e5d

WodansSon reviewed May 16, 2018

View reviewed changes

WodansSon previously requested changes May 16, 2018

View reviewed changes

Neeran Gul added 2 commits May 16, 2018 15:22

better error messages as suggested

b663c43

add log in data_source

1641e01

katbyte requested changes May 16, 2018

View reviewed changes

Neeran Gul added 2 commits May 17, 2018 14:51

added fixes as reccommended

3d4c9f8

formtting terraform

915a968

katbyte added new-resource service/data-lake labels May 19, 2018

katbyte added this to the 1.6.0 milestone May 19, 2018

katbyte requested changes May 19, 2018

View reviewed changes

Neeran Gul added 2 commits May 19, 2018 21:43

wrapping exceptions and removing unused func

d014b3a

didnt run fmt

9bc6096

katbyte added 4 commits May 19, 2018 17:20

Update data_lake_store.html.markdown

8d74125

Update resource_arm_data_lake_store_test.go

59dee53

Update data_source_data_lake_store_test.go

36e3882

Update resource_arm_data_lake_store_test.go

47bb571

Fix typo in test

katbyte approved these changes May 20, 2018

View reviewed changes

katbyte merged commit a836939 into hashicorp:master May 20, 2018

katbyte added a commit that referenced this pull request May 20, 2018

Update CHANGELOG.md to include #1219

449ccc0

ghost locked and limited conversation to collaborators Mar 31, 2020


		The following arguments are supported:

		* `name` - (Required) Specifies the name of the Data Lake Store. Changing this forces a

New Resource: azurerm_data_lake_store #1219

New Resource: azurerm_data_lake_store #1219

Conversation

paktek123 commented May 9, 2018

paktek123 commented May 15, 2018

WodansSon May 16, 2018 • edited Loading

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

WodansSon left a comment

Choose a reason for hiding this comment

paktek123 commented May 16, 2018 • edited Loading

katbyte left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paktek123 May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paktek123 May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paktek123 May 17, 2018 • edited Loading

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

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

paktek123 commented May 17, 2018

katbyte left a comment • edited Loading

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

Choose a reason for hiding this comment

paktek123 commented May 19, 2018

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

WodansSon May 16, 2018 •

edited

Loading

paktek123 commented May 16, 2018 •

edited

Loading

katbyte left a comment •

edited

Loading

paktek123 May 17, 2018 •

edited

Loading

paktek123 May 17, 2018 •

edited

Loading

paktek123 May 17, 2018 •

edited

Loading

katbyte left a comment •

edited

Loading