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

New Resource: azurerm_data_lake_store #1219

Merged
merged 26 commits into from
May 20, 2018

Conversation

paktek123
Copy link
Contributor

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

@paktek123
Copy link
Contributor Author

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

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

@WodansSon WodansSon May 16, 2018

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, in next commit

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, in next commit

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update this to be just return err?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, in next commit

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, just return err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, in next commit

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

return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be return nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this should be nil, done

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

paktek123 commented May 16, 2018

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

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 @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 🙂

d.Set("location", azureRMNormalizeLocation(*location))
}

if tier := resp.DataLakeStoreAccountProperties; tier != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we assign this to properties not tier for clarity?

Copy link
Contributor Author

@paktek123 paktek123 May 17, 2018

Choose a reason for hiding this comment

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

done


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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@paktek123 paktek123 May 17, 2018

Choose a reason for hiding this comment

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

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()


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

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

@paktek123 paktek123 May 17, 2018

Choose a reason for hiding this comment

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

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()


The following arguments are supported:

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

Choose a reason for hiding this comment

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

Could we put this all on the same line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next commit

},

Schema: map[string]*schema.Schema{
"name": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in next commit

* `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
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

location = "northeurope"
}

resource "azurerm_data_lake_store" "payasyougo" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup renamed


```hcl
# Pay As You Go
resource "azurerm_resource_group" "test" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup moved to 2 spaces

testCheckAzureRMDataLakeStoreExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "tier", "Commitment_1TB"),
),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get import checks on all these tests?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added import checks for all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and removed import file


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

Choose a reason for hiding this comment

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

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)
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done for Create

@paktek123
Copy link
Contributor Author

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

@katbyte katbyte added this to the 1.6.0 milestone May 19, 2018
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.

@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.


"tier": {
Type: schema.TypeString,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should be computed with no default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup has to be computed


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

Choose a reason for hiding this comment

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

Could we wrap this error like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in next commit


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

Choose a reason for hiding this comment

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

Could we wrap this error like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in next commit

return nil
}

func validateArmDataLakeName(v interface{}, k string) (ws []string, es []error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if response.WasNotFound(future.Response()) {
return nil
}
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we wrap this error too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in next commit

if response.WasNotFound(future.Response()) {
return nil
}
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we wrap this error too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped in next commit

@paktek123
Copy link
Contributor Author

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

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 @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$

@katbyte katbyte dismissed WodansSon’s stale review May 20, 2018 01:46

All requested changes are outdated

@katbyte katbyte merged commit a836939 into hashicorp:master May 20, 2018
katbyte added a commit that referenced this pull request May 20, 2018
@ghost
Copy link

ghost commented Mar 31, 2020

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 31, 2020
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.

3 participants