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.

Sign up for GitHub

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

data_source_key_vault, resource_arm_key_vault, resource_arm_automation_account, resource_arm_notification_hub_namespace, resource_arm_relay_namespace: Flatten SKU #3119

Merged
merged 30 commits into from
Jun 27, 2019
Merged

data_source_key_vault, resource_arm_key_vault, resource_arm_automation_account, resource_arm_notification_hub_namespace, resource_arm_relay_namespace: Flatten SKU #3119

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ed02bc9
Flatten azurerm_automation_account sku
WodansSon Mar 22, 2019
4376934
Flatten azurerm_key_vault sku
WodansSon Mar 22, 2019
857900f
Flatten azurerm_notification_hub sku
WodansSon Mar 22, 2019
f0f0f7d
Flatten azurerm_relay_namespace sku
WodansSon Mar 23, 2019
8e73fe9
Fix lint error
WodansSon Mar 25, 2019
d630381
Update for backward compatibility
WodansSon Apr 3, 2019
a68fd36
Updates for backwards compatibility
WodansSon Apr 3, 2019
96cf7e3
Merge branch 'master' into e_flatten_sku
WodansSon Apr 22, 2019
106e746
Update website/docs/r/automation_account.html.markdown
WodansSon Jun 20, 2019
2764db9
Update website/docs/r/key_vault.html.markdown
WodansSon Jun 20, 2019
d15ee4c
Update website/docs/r/notification_hub_namespace.html.markdown
WodansSon Jun 20, 2019
907f35e
Update website/docs/r/relay_namespace.html.markdown
WodansSon Jun 20, 2019
2bd0c80
Addressed PR comments...
WodansSon Jun 20, 2019
699cd49
Merge branch 'master' into e_flatten_sku
WodansSon Jun 20, 2019
a2f3528
Fix lint and build errors
WodansSon Jun 20, 2019
c9a9fae
Merge branch 'e_flatten_sku' of https://github.com/terraform-provider…
WodansSon Jun 20, 2019
f907f33
Fix CI\CD issues
WodansSon Jun 21, 2019
9781279
Ran gofmt on files...
WodansSon Jun 21, 2019
c642cdc
Updates per PR
WodansSon Jun 26, 2019
465a3ee
Update for PR
WodansSon Jun 26, 2019
c7cb43b
Fix lint error
WodansSon Jun 26, 2019
a406b85
Updated CreateUpdate for all resources
WodansSon Jun 27, 2019
4bd21fa
Fixed relay.SkuName issue
WodansSon Jun 27, 2019
7ac5cef
Fixed Travis Errors
WodansSon Jun 27, 2019
0c8c18b
Update azurerm/resource_arm_relay_namespace.go
WodansSon Jun 27, 2019
962aec1
Update azurerm/resource_arm_notification_hub_namespace.go
WodansSon Jun 27, 2019
d5e7a70
Set both Sku and Sku_Name in datasource
WodansSon Jun 27, 2019
c6ebd0f
Update azurerm/resource_arm_notification_hub_namespace_test.go
WodansSon Jun 27, 2019
86bebe7
Update Key Vault Datasource and test case
WodansSon Jun 27, 2019
832d630
Merge branch 'master' into e_flatten_sku
WodansSon Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions azurerm/data_source_key_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ func dataSourceArmKeyVaultRead(d *schema.ResourceData, meta interface{}) error {
d.Set("enabled_for_template_deployment", props.EnabledForTemplateDeployment)
d.Set("vault_uri", props.VaultURI)

if err := d.Set("sku", flattenKeyVaultDataSourceSku(props.Sku)); err != nil {
if err := d.Set("sku_name", flattenKeyVaultDataSourceSku(props.Sku)); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a mistake? there is no property sku_name in the schema for this resource

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it is not a mistake... the property coming from Azure will still be props.Sku, so I need to read that and then convert it to the new property sku_name

return fmt.Errorf("Error setting `sku` for KeyVault %q: %+v", *resp.Name, err)
}

Expand All @@ -196,12 +196,8 @@ func dataSourceArmKeyVaultRead(d *schema.ResourceData, meta interface{}) error {
return nil
}

func flattenKeyVaultDataSourceSku(sku *keyvault.Sku) []interface{} {
result := map[string]interface{}{
"name": string(sku.Name),
}

return []interface{}{result}
func flattenKeyVaultDataSourceSku(sku *keyvault.Sku) string {
return string(sku.Name)
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,

also we don't need a function to do a simple return string(sku.Name), i would have removed this entire function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made it a function just in case there is more that needs to be done in the future, I will go ahead and remove the function.

}

func flattenKeyVaultDataSourceNetworkAcls(input *keyvault.NetworkRuleSet) []interface{} {
Expand Down
75 changes: 64 additions & 11 deletions azurerm/resource_arm_automation_account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,18 @@ func resourceArmAutomationAccount() *schema.Resource {

"resource_group_name": resourceGroupNameSchema(),

// Remove in 2.0
"sku": {
Type: schema.TypeList,
Required: true,
MaxItems: 1,
Type: schema.TypeList,
Optional: true,
Deprecated: "This property has been deprecated in favour of the 'sku_name' property and will be removed in version 2.0 of the provider",
ConflictsWith: []string{"sku_name"},
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Optional: true,
Default: string(automation.Basic),
Required: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing this to required instead of leaving it as optional? this will break peoples configurations?

DiffSuppressFunc: suppress.CaseDifference,
ValidateFunc: validation.StringInSlice([]string{
string(automation.Basic),
Expand All @@ -59,6 +61,17 @@ func resourceArmAutomationAccount() *schema.Resource {
},
},

"sku_name": {
Type: schema.TypeString,
Optional: true,
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For new properties we shouldn't be ignoring case without good reason such as a broken API, can we remove this?

Suggested change
DiffSuppressFunc: suppress.CaseDifference,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure...

ConflictsWith: []string{"sku"},
ValidateFunc: validation.StringInSlice([]string{
string(automation.Basic),
string(automation.Free),
}, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}, true),
}, false),

},

"tags": tagsSchema(),

"dsc_server_endpoint": {
Expand All @@ -81,6 +94,12 @@ func resourceArmAutomationAccountCreateUpdate(d *schema.ResourceData, meta inter
client := meta.(*ArmClient).automationAccountClient
ctx := meta.(*ArmClient).StopContext

sku := expandAutomationAccountSku(d)

if sku == nil {
return fmt.Errorf("either 'sku_name' or 'sku' must be defined in the configuration file")
}

log.Printf("[INFO] preparing arguments for Automation Account create/update.")

name := d.Get("name").(string)
Expand All @@ -101,7 +120,6 @@ func resourceArmAutomationAccountCreateUpdate(d *schema.ResourceData, meta inter

location := azureRMNormalizeLocation(d.Get("location").(string))
tags := d.Get("tags").(map[string]interface{})
sku := expandAutomationAccountSku(d)

parameters := automation.AccountCreateOrUpdateParameters{
AccountCreateOrUpdateProperties: &automation.AccountCreateOrUpdateProperties{
Expand Down Expand Up @@ -140,6 +158,7 @@ func resourceArmAutomationAccountRead(d *schema.ResourceData, meta interface{})
}
resGroup := id.ResourceGroup
name := id.Path["automationAccounts"]
skuName := d.Get("sku_name").(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What you can do is mark both as computed until 2.0 and just set both instead of doing tricky logic like this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would that cause issues when we totally remove sku?

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 as at that point we'll only be setting the required one.


resp, err := client.Get(ctx, resGroup, name)
if err != nil {
Expand Down Expand Up @@ -169,8 +188,21 @@ func resourceArmAutomationAccountRead(d *schema.ResourceData, meta interface{})
d.Set("location", azureRMNormalizeLocation(*location))
}

if err := d.Set("sku", flattenAutomationAccountSku(resp.Sku)); err != nil {
return fmt.Errorf("Error setting `sku`: %+v", err)
if skuName == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What you can do is mark both as computed until 2.0 and just set both instead of doing tricky logic like this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would that cause issues when we totally remove sku?

// Remove in 2.0
if sku := resp.Sku; sku != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This entire thing can then be:

		if sku := resp.Sku; sku != nil {
			if name := sku.Name; name != nil {
				d.Set("sku", string(name)) // todo remove in 2.0
				d.Set("sku_name", string(name))
			}
		}

i don't think there is a need for the flatten function anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did remove one of the flatten functions, however I don't think we should be setting both of the values.

if err := d.Set("sku", flattenAutomationAccountSku(resp.Sku)); err != nil {
return fmt.Errorf("Error setting `sku`: %+v", err)
}
}
d.Set("sku_name", "")
} else {
if sku := resp.Sku; sku != nil {
if err := d.Set("sku_name", flattenAutomationAccountSkuName(resp.Sku)); err != nil {
return fmt.Errorf("Error setting `sku_name`: %+v", err)
}
}
d.Set("sku", "")
}

d.Set("dsc_server_endpoint", keysResp.Endpoint)
Expand Down Expand Up @@ -210,6 +242,15 @@ func resourceArmAutomationAccountDelete(d *schema.ResourceData, meta interface{}
return nil
}

func flattenAutomationAccountSkuName(sku *automation.Sku) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets remove this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I did.

if sku == nil {
return ""
}

return string(sku.Name)
}

// Remove in 2.0
func flattenAutomationAccountSku(sku *automation.Sku) []interface{} {
if sku == nil {
return []interface{}{}
Expand All @@ -221,9 +262,21 @@ func flattenAutomationAccountSku(sku *automation.Sku) []interface{} {
}

func expandAutomationAccountSku(d *schema.ResourceData) *automation.Sku {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for backward compatibility when both sku and sku_name exist side by side...

inputs := d.Get("sku").([]interface{})
input := inputs[0].(map[string]interface{})
name := automation.SkuNameEnum(input["name"].(string))
v := d.Get("sku_name").(string)
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 move this logic into the main function as we'll remove this in 2.0 and it'll end up being

sku = automation.SkuNameEnum(d.Get("sku_name").(string))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Move the logic and removed the function


// Remove in 2.0
if v == "" {
inputs := d.Get("sku").([]interface{})

if len(inputs) == 0 {
return nil
}

input := inputs[0].(map[string]interface{})
v = input["name"].(string)
}

name := automation.SkuNameEnum(v)

sku := automation.Sku{
Name: name,
Expand Down
97 changes: 87 additions & 10 deletions azurerm/resource_arm_automation_account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package azurerm

import (
"fmt"
"regexp"
"testing"

"github.com/hashicorp/terraform/helper/resource"
Expand All @@ -21,6 +22,35 @@ func TestAccAzureRMAutomationAccount_basic(t *testing.T) {
Steps: []resource.TestStep{
{
Config: testAccAzureRMAutomationAccount_basic(ri, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMAutomationAccountExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "sku_name", "Basic"),
resource.TestCheckResourceAttrSet(resourceName, "dsc_server_endpoint"),
resource.TestCheckResourceAttrSet(resourceName, "dsc_primary_access_key"),
resource.TestCheckResourceAttrSet(resourceName, "dsc_secondary_access_key"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: false,
},
},
})
}

// Remove in 2.0
func TestAccAzureRMAutomationAccount_basicClassic(t *testing.T) {
resourceName := "azurerm_automation_account.test"
ri := tf.AccRandTimeInt()

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMAutomationAccountDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMAutomationAccount_basicClassic(ri, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMAutomationAccountExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "sku.0.name", "Basic"),
Expand All @@ -38,6 +68,23 @@ func TestAccAzureRMAutomationAccount_basic(t *testing.T) {
})
}

// Remove in 2.0
func TestAccAzureRMAutomationAccount_basicNotDefined(t *testing.T) {
ri := tf.AccRandTimeInt()

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMAutomationAccountDestroy,
Steps: []resource.TestStep{
{
Config: testAccAzureRMAutomationAccount_basicNotDefined(ri, testLocation()),
ExpectError: regexp.MustCompile("either 'sku_name' or 'sku' must be defined in the configuration file"),
},
},
})
}

func TestAccAzureRMAutomationAccount_requiresImport(t *testing.T) {
if !requireResourcesToBeImported {
t.Skip("Skipping since resources aren't required to be imported")
Expand Down Expand Up @@ -80,14 +127,14 @@ func TestAccAzureRMAutomationAccount_complete(t *testing.T) {
Config: testAccAzureRMAutomationAccount_complete(ri, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMAutomationAccountExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "sku.0.name", "Basic"),
resource.TestCheckResourceAttr(resourceName, "sku_name", "Basic"),
resource.TestCheckResourceAttr(resourceName, "tags.hello", "world"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerify: false,
},
},
})
Expand Down Expand Up @@ -160,14 +207,48 @@ resource "azurerm_resource_group" "test" {
location = "%s"
}

resource "azurerm_automation_account" "test" {
name = "acctest-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"

sku_name = "Basic"
}
`, rInt, location, rInt)
}

// Remove in 2.0
func testAccAzureRMAutomationAccount_basicClassic(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_automation_account" "test" {
name = "acctest-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"

sku {
name = "Basic"
}
}
}
`, rInt, location, rInt)
}

// Remove in 2.0
func testAccAzureRMAutomationAccount_basicNotDefined(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_automation_account" "test" {
name = "acctest-%d"
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"
}
`, rInt, location, rInt)
}
Expand All @@ -182,9 +263,7 @@ resource "azurerm_automation_account" "import" {
location = "${azurerm_automation_account.test.location}"
resource_group_name = "${azurerm_automation_account.test.resource_group_name}"

sku {
name = "Basic"
}
sku_name = "Basic"
}
`, template)
}
Expand All @@ -201,10 +280,8 @@ resource "azurerm_automation_account" "test" {
location = "${azurerm_resource_group.test.location}"
resource_group_name = "${azurerm_resource_group.test.name}"

sku {
name = "Basic"
}

sku_name = "Basic"

tags = {
"hello" = "world"
}
Expand Down
Loading