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

Automation resources #1512

Merged
merged 25 commits into from Oct 17, 2018
Merged

Automation resources #1512

merged 25 commits into from Oct 17, 2018

Conversation

bgelens
Copy link
Contributor

@bgelens bgelens commented Jul 6, 2018

New resources for Automation Services

  • azurerm_automation_module
  • azurerm_automation_dsc_configuration
  • azurerm_automation_dsc_nodeconfiguration

I am new to almost every aspect of this (including development in go) so if it's not that "good" I totally understand but I'm seeking to learn!

Technically, dsc_nodeconfiguration has a hard dependency on dsc_configuration. The portal creates an empty configuration using special magic but the API doesn't expose this coupling of actions.

@lfshr
Copy link
Contributor

lfshr commented Jul 6, 2018

I totally didn't realise this was a PR and not an issue. Sorry bud, my last comment probably didn't make any sense.

@bgelens
Copy link
Contributor Author

bgelens commented Jul 6, 2018

No worries! I figured as much.
I'm going on holiday without internet for the coming week so take your time for reviewing.

ForceNew: true,
},

"account_name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably be explicit here and call this automation_account_name

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this should be automation_account_name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following already established Automation related resources like credential

resource "azurerm_automation_credential" "example" {
  name                = "credential1"
  resource_group_name = "${azurerm_resource_group.example.name}"
  account_name        = "${azurerm_automation_account.example.name}"
  username           = "example_user"
  password            = "example_pwd"
  description         = "This is an example credential"
}

Do I still need to change mine (it will result in inconsistent parameter naming)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Makes me slightly sad but I'd probably say consistency over correctness 😢 @katbyte your call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we could deprecate the account_name in azurerm_automation_credential.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do, the other resources are incorrect and will need to be changed eventually.

As much as I would usually agree in consistency over correctness, this isn't a very confusing change so for a new resource I'm going to go with its better to use the correct name.

I have already depreciated account_name in azurerm_automation_schedule, so feel free to apply that to the rest of the automation resources, preferably in a separate PR 🙂

ForceNew: true,
},

"account_name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

ForceNew: true,
},

"account_name": {
Copy link
Contributor

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
Collaborator

Choose a reason for hiding this comment

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

Agreed

@lfshr
Copy link
Contributor

lfshr commented Jul 6, 2018

I won't be doing the actual review, I'm really on the same boat as yourself re. GO. Only the one comment from my untrained eye. 😄

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.

Hello @bgelens,

Thank you for contributing these resources! In addition to the comments I have left inline each resource needs to be documented and added to the website directory and tags should be added to each one as well.

It seems you are not reading back into state the current state of the resource? This will prevent terraform from correcting any changes made outside of a plan/apply. Is there a reason for this?

Otherwise this is off to a good start, if you have any questions don't hesitate to ask 🙂

automationCredentialClient automation.CredentialClient
automationScheduleClient automation.ScheduleClient
automationModuleClient automation.ModuleClient
automationDscNodeConfigurationClient automation.DscNodeConfigurationClient
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 these in alphabetical order?

@@ -128,6 +128,9 @@ func Provider() terraform.ResourceProvider {
"azurerm_automation_credential": resourceArmAutomationCredential(),
"azurerm_automation_runbook": resourceArmAutomationRunbook(),
"azurerm_automation_schedule": resourceArmAutomationSchedule(),
"azurerm_automation_module": resourceArmAutomationModule(),
"azurerm_automation_dsc_nodeconfiguration": resourceArmAutomationDscNodeConfiguration(),
"azurerm_automation_dsc_configuration": resourceArmAutomationDscConfiguration(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, could we put these into alphabetical order?

ForceNew: true,
},

"account_name": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, this should be automation_account_name


"content": {
Type: schema.TypeString,
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.

Unless an empty string is valid, a quick validation.NoZeroValues would be nice to have here.

azurerm/resource_arm_automation_module.go Show resolved Hide resolved
{
Config: testAccAzureRMAutomationModule_basic(ri, testLocation()),
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMAutomationModuleExists(resourceName),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, we should be checking the actual state of the resource after a apply not just if it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case there is not much to check. If the resource exists is all I can test afaict

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.

Accidentally hit approve instead of request changes.

@bgelens
Copy link
Contributor Author

bgelens commented Jul 27, 2018

I'm back from holiday.. sorry for the delayed response 😄 I will start working on the issues mentioned!

@bgelens
Copy link
Contributor Author

bgelens commented Aug 7, 2018

Sorry I'm not making great progress. Working on it when I can. I'll request a new review once I have gone through all remarks (hopefully end of week)

setUserAgent(&dscNodeConfigurationClient.Client)
dscNodeConfigurationClient.Authorizer = auth
dscNodeConfigurationClient.Sender = sender
dscNodeConfigurationClient.SkipResourceProviderRegistration = c.skipProviderRegistration
Copy link

@JunyiYi JunyiYi Aug 8, 2018

Choose a reason for hiding this comment

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

These lines could be simplified to c.configureClient(&dscNodeConfigurationClient.Client, auth), take an example here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/config.go#L535.
Similar suggestion applies to the following 4 clients. #Closed

}

if read.ID == nil {
return fmt.Errorf("Cannot read Automation Module '%s' (resource group %s) ID", name, resGroup)
Copy link

@JunyiYi JunyiYi Aug 8, 2018

Choose a reason for hiding this comment

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

%s [](start = 52, length = 2)

We use %q instead of %s for name and resource group name.
This suggestion applies to all other code in this changeset as well. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the other automation resources which where already part of the provider. Changed the resources in scope of PR but fyi, the others do still have %s

return nil
}

return fmt.Errorf("Error issuing AzureRM delete request for Automation Module '%s': %+v", name, err)
Copy link

@JunyiYi JunyiYi Aug 8, 2018

Choose a reason for hiding this comment

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

minor Maybe reverse the condition here for clearer logic:

if err != nil {
  if !utils.ResponseWasNotFound(resp) {
    return fmt.Errorf(...)
  }
}

This suggestion also applies to the other two resources as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the other automation resources. Should we drift from these or clean them all up in one go through another PR?

func expandModuleLink(d *schema.ResourceData) automation.ContentLink {
inputs := d.Get("module_link").([]interface{})
input := inputs[0].(map[string]interface{})
uri := input["uri"].(string)
Copy link

Choose a reason for hiding this comment

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

sidenote We have a shortcut here: uri := d.Get("module_link.0.uri").(string).

ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"module_link"},
Copy link

Choose a reason for hiding this comment

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

ImportStateVerifyIgnore: []string{"module_link"}, [](start = 4, length = 49)

Why not import the module_link here? This should be the similar comment provided in resource_arm_automation_module.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the content link is not returned by the rest api in a get operation. This is the body I get returned:

{
  "id": "/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.Automation/automationAccounts/xxxx/modules/xActiveDirectory",
  "name": "xActiveDirectory",
  "type": "Microsoft.Automation/AutomationAccounts/Modules",
  "location": "westeurope",
  "tags": {},
  "etag": null,
  "properties": {
    "isGlobal": false,
    "version": "2.19.0.0",
    "sizeInBytes": 101851,
    "activityCount": 13,
    "creationTime": "2018-08-20T06:32:33.79+00:00",
    "lastModifiedTime": "2018-08-20T06:33:45.133+00:00",
    "error": {
      "code": null,
      "message": ""
    },
    "provisioningState": "Succeeded",
    "isComposite": false
  }
}

@ghost ghost added the size/XXL label Aug 24, 2018
@bgelens
Copy link
Contributor Author

bgelens commented Aug 24, 2018

@JunyiYi I've fixed your minor comments. I'll need to figure out what to do to reference the newer SDK once it is out so I can include the content fetch. Any pointers here would be greatly appreciated :)

@lfshr Please Go ahead creating that compilation resource! With regard to the psgallery translation, you think to embed that logic in the module resource? That would be a great addition IMO

@lfshr
Copy link
Contributor

lfshr commented Aug 27, 2018

@bgelens see govendor for help on updating the vendor packages.

I'm not sure. I think I'll keep it the same as the swagger and maybe think about including it in the dsc_configuration as well.

@ghost ghost added the size/XXL label Aug 31, 2018
@bgelens
Copy link
Contributor Author

bgelens commented Aug 31, 2018

@JunyiYi I've updated to v20.0.0 for automation and added the content fetch for the dsc configuration resource (updated test accordingly).

Copy link

@JunyiYi JunyiYi left a comment

Choose a reason for hiding this comment

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

Hi @bgelens , thanks for the update. I have looked through the updated code carefully and most of the logic LGTM. I still have one question, why not set back content_embedded and module_link in DSC Node Configuration and Automation Module?

parameters := automation.DscConfigurationCreateOrUpdateParameters{
DscConfigurationCreateOrUpdateProperties: &automation.DscConfigurationCreateOrUpdateProperties{
LogVerbose: utils.Bool(logVerbose),
Description: &description,
Copy link

Choose a reason for hiding this comment

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

minor utils.String(description)

Description: &description,
Source: &automation.ContentSource{
Type: automation.EmbeddedContent,
Value: &contentEmbedded,
Copy link

Choose a reason for hiding this comment

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

minor utils.String(contentEmbedded)

Value: &contentEmbedded,
},
},
Location: &location,
Copy link

Choose a reason for hiding this comment

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

minor utils.String(location)

},
},
Location: &location,
Name: &name,
Copy link

Choose a reason for hiding this comment

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

Is it necessary to pass name here? What will happen if we just leave it empty? Since name is already passed to the API in the following CreateOrUpdate call.

Copy link
Contributor Author

@bgelens bgelens Sep 9, 2018

Choose a reason for hiding this comment

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

It will work in this case. Removed

d.Set("state", resp.State)
}

contentresp, contenterr := client.GetContent(ctx, resGroup, accName, name)
Copy link

Choose a reason for hiding this comment

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

minor Just reuse local variable err here is fine.

Configuration: &automation.DscConfigurationAssociationProperty{
Name: &configurationName,
},
Name: &name,
Copy link

Choose a reason for hiding this comment

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

Can you confirm whether we need to pass name here or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will fail validation.

parameters := automation.DscNodeConfigurationCreateOrUpdateParameters{
Source: &automation.ContentSource{
Type: automation.EmbeddedContent,
Value: &content,
Copy link

Choose a reason for hiding this comment

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

minor Please prefer utils.*** functions to the address-of operator (&) here.


d.Set("name", resp.Name)
d.Set("resource_group_name", resGroup)
d.Set("automation_account_name", accName)
Copy link

Choose a reason for hiding this comment

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

Since DSC configuration API contains the content read function, is there any parity APIs for this resource?

}

resource "azurerm_automation_dsc_configuration" "test" {
name = "test"
Copy link

Choose a reason for hiding this comment

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

Similar to the comment above, all test resources names should start with acctest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as previous reply. No need to do this

Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned above, we would prefer this to be name acctest.

d.Set("name", resp.Name)
d.Set("resource_group_name", resGroup)
d.Set("automation_account_name", accName)

Copy link

Choose a reason for hiding this comment

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

module_link is not set back. So terraform will have no way to track it any more in its .tfstate file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not part of the object state in Azure Automation account so it cannot be read back.

{
  "id": "/subscriptions/xxxx/resourceGroups/bla/providers/Microsoft.Automation/automationAccounts/mytfaaaccount/modules/xActiveDirectory",
  "name": "xActiveDirectory",
  "type": "Microsoft.Automation/AutomationAccounts/Modules",
  "location": "westeurope",
  "tags": {},
  "etag": null,
  "properties": {
    "isGlobal": false,
    "version": "2.19.0.0",
    "sizeInBytes": 101851,
    "activityCount": 13,
    "creationTime": "2018-08-20T08:32:33.79+02:00",
    "lastModifiedTime": "2018-08-24T07:56:43.897+02:00",
    "error": {
      "code": null,
      "message": ""
    },
    "provisioningState": "Succeeded",
    "isComposite": false
  }
}

@ghost ghost added the size/XXL label Sep 9, 2018
@bgelens
Copy link
Contributor Author

bgelens commented Sep 9, 2018

@JunyiYi why not set back content_embedded and module_link in DSC Node Configuration and Automation Module? Unfortunately, getting the content_embedded for DSC Node configuration is not exposed via the API, nor is the module link for Automation module.

@ghost ghost added the size/XXL label Sep 12, 2018
@bgelens
Copy link
Contributor Author

bgelens commented Sep 12, 2018

I changed the resource name to acctest for both test cases. No dashes are allowed so I hope this is ok like this.

@bgelens
Copy link
Contributor Author

bgelens commented Sep 28, 2018

Wondering if there is anything else to fix or if it is ok like this? Thanks!

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 @bgelens,

Sorry for the delay in reviewing this, I've taken another quick look and it all LGTM now 🙂

@katbyte
Copy link
Collaborator

katbyte commented Oct 17, 2018

Tests pass:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMAutoma -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMAutomationAccount_basic
--- PASS: TestAccAzureRMAutomationAccount_basic (75.32s)
=== RUN   TestAccAzureRMAutomationAccount_complete
--- PASS: TestAccAzureRMAutomationAccount_complete (70.27s)
=== RUN   TestAccAzureRMAutomationCredential_basic
--- PASS: TestAccAzureRMAutomationCredential_basic (71.22s)
=== RUN   TestAccAzureRMAutomationCredential_complete
--- PASS: TestAccAzureRMAutomationCredential_complete (70.70s)
=== RUN   TestAccAzureRMAutomationDscConfiguration_basic
--- PASS: TestAccAzureRMAutomationDscConfiguration_basic (74.23s)
=== RUN   TestAccAzureRMAutomationDscNodeConfiguration_basic
--- PASS: TestAccAzureRMAutomationDscNodeConfiguration_basic (77.86s)
=== RUN   TestAccAzureRMAutomationModule_basic
--- PASS: TestAccAzureRMAutomationModule_basic (73.08s)
=== RUN   TestAccAzureRMAutomationRunbook_PSWorkflow
--- PASS: TestAccAzureRMAutomationRunbook_PSWorkflow (73.43s)
=== RUN   TestAccAzureRMAutomationRunbook_PSWorkflowWithHash
--- PASS: TestAccAzureRMAutomationRunbook_PSWorkflowWithHash (73.03s)
=== RUN   TestAccAzureRMAutomationRunbook_PSWithContent
--- PASS: TestAccAzureRMAutomationRunbook_PSWithContent (76.68s)
=== RUN   TestAccAzureRMAutomationSchedule_oneTime_basic
--- PASS: TestAccAzureRMAutomationSchedule_oneTime_basic (74.96s)
=== RUN   TestAccAzureRMAutomationSchedule_oneTime_complete
--- PASS: TestAccAzureRMAutomationSchedule_oneTime_complete (70.93s)
=== RUN   TestAccAzureRMAutomationSchedule_oneTime_update
--- PASS: TestAccAzureRMAutomationSchedule_oneTime_update (81.99s)
=== RUN   TestAccAzureRMAutomationSchedule_hourly
--- PASS: TestAccAzureRMAutomationSchedule_hourly (70.93s)
=== RUN   TestAccAzureRMAutomationSchedule_daily
--- PASS: TestAccAzureRMAutomationSchedule_daily (71.28s)
=== RUN   TestAccAzureRMAutomationSchedule_weekly
--- PASS: TestAccAzureRMAutomationSchedule_weekly (70.92s)
=== RUN   TestAccAzureRMAutomationSchedule_monthly
--- PASS: TestAccAzureRMAutomationSchedule_monthly (70.87s)
=== RUN   TestAccAzureRMAutomationSchedule_weekly_advanced
--- PASS: TestAccAzureRMAutomationSchedule_weekly_advanced (70.76s)
=== RUN   TestAccAzureRMAutomationSchedule_monthly_advanced_by_day
--- PASS: TestAccAzureRMAutomationSchedule_monthly_advanced_by_day (69.89s)
=== RUN   TestAccAzureRMAutomationSchedule_monthly_advanced_by_week_day
--- PASS: TestAccAzureRMAutomationSchedule_monthly_advanced_by_week_day (69.83s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1458.224s

@katbyte katbyte added this to the 1.17.0 milestone Oct 17, 2018
@katbyte katbyte merged commit 1c47f25 into hashicorp:master Oct 17, 2018
katbyte added a commit that referenced this pull request Oct 17, 2018
@bgelens
Copy link
Contributor Author

bgelens commented Oct 18, 2018

Great! Thanks :)

@ghost
Copy link

ghost commented Mar 6, 2019

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!

@hashicorp hashicorp locked and limited conversation to collaborators Mar 6, 2019
@ghost ghost removed the waiting-response label Mar 6, 2019
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.

None yet

5 participants