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

Conversation

@bgelens
Copy link
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

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": {

This comment has been minimized.

@lfshr

lfshr Jul 6, 2018

Contributor

I would probably be explicit here and call this automation_account_name

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

Agreed, this should be automation_account_name

This comment has been minimized.

@bgelens

bgelens Jul 27, 2018

Author Contributor

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

This comment has been minimized.

@lfshr

lfshr Jul 27, 2018

Contributor

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

This comment has been minimized.

@lfshr

lfshr Jul 27, 2018

Contributor

Or we could deprecate the account_name in azurerm_automation_credential.

This comment has been minimized.

@katbyte

katbyte Jul 27, 2018

Contributor

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": {

This comment has been minimized.

@lfshr

lfshr Jul 6, 2018

Contributor

Same as above

ForceNew: true,
},

"account_name": {

This comment has been minimized.

@lfshr

lfshr Jul 6, 2018

Contributor

Same as above

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

Agreed

@lfshr

This comment has been minimized.

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

@katbyte
Copy link
Contributor

katbyte left a comment

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

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

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

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

As above, could we put these into alphabetical order?

ForceNew: true,
},

"account_name": {

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

Agreed, this should be automation_account_name

Show resolved Hide resolved azurerm/resource_arm_automation_dsc_configuration.go

"content": {
Type: schema.TypeString,
Required: true,

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

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

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

This comment has been minimized.

@katbyte

katbyte Jul 10, 2018

Contributor

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

This comment has been minimized.

@bgelens

bgelens Aug 20, 2018

Author Contributor

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

@katbyte
Copy link
Contributor

katbyte left a comment

Accidentally hit approve instead of request changes.

@bgelens

This comment has been minimized.

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 bgelens force-pushed the bgelens:automationresources branch from 479ee2b to 4b23f8f Jul 29, 2018

@bgelens bgelens force-pushed the bgelens:automationresources branch from 1be38b7 to f1c1e3e Jul 30, 2018

@metacpp metacpp requested a review from JunyiYi Aug 3, 2018

@bgelens

This comment has been minimized.

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

This comment has been minimized.

@JunyiYi

JunyiYi Aug 8, 2018

Collaborator

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)

This comment has been minimized.

@JunyiYi

JunyiYi Aug 8, 2018

Collaborator

%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

This comment has been minimized.

@bgelens

bgelens Aug 19, 2018

Author Contributor

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)

This comment has been minimized.

@JunyiYi

JunyiYi Aug 8, 2018

Collaborator

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.

This comment has been minimized.

@bgelens

bgelens Aug 20, 2018

Author Contributor

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)

This comment has been minimized.

@JunyiYi

JunyiYi Aug 8, 2018

Collaborator

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

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

This comment has been minimized.

@JunyiYi

JunyiYi Aug 8, 2018

Collaborator

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.

This comment has been minimized.

@bgelens

bgelens Aug 20, 2018

Author Contributor

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
  }
}
@JunyiYi
Copy link
Collaborator

JunyiYi left a comment

Hi @bgelens , thanks for the response and most of the code LGTM. However we might still need to wait for the API fix before publishing this resource.

Show resolved Hide resolved azurerm/resource_arm_automation_dsc_configuration.go Outdated

parameters := automation.DscConfigurationCreateOrUpdateParameters{
DscConfigurationCreateOrUpdateProperties: &automation.DscConfigurationCreateOrUpdateProperties{
LogVerbose: &logVerbose,

This comment has been minimized.

@JunyiYi

JunyiYi Aug 23, 2018

Collaborator

minor Latest coding style: Always using utils.Bool/utls.String instead of address of & for primitive types.

@hashibot hashibot bot added the size/XXL label Aug 24, 2018

@bgelens

This comment has been minimized.

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

This comment has been minimized.

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.

@hashibot hashibot bot added the size/XXL label Aug 31, 2018

@bgelens

This comment has been minimized.

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

@JunyiYi
Copy link
Collaborator

JunyiYi left a comment

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,

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

minor utils.String(description)

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

minor utils.String(contentEmbedded)

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

minor utils.String(location)

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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.

This comment has been minimized.

@bgelens

bgelens Sep 9, 2018

Author Contributor

It will work in this case. Removed

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

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

minor Just reuse local variable err here is fine.

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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

This comment has been minimized.

@bgelens

bgelens Sep 9, 2018

Author Contributor

It will fail validation.

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

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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)

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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"

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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

This comment has been minimized.

@bgelens

bgelens Sep 9, 2018

Author Contributor

Same as previous reply. No need to do this

This comment has been minimized.

@katbyte

katbyte Sep 12, 2018

Contributor

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)

This comment has been minimized.

@JunyiYi

JunyiYi Sep 7, 2018

Collaborator

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

This comment has been minimized.

@bgelens

bgelens Sep 7, 2018

Author Contributor

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

@hashibot hashibot bot added the size/XXL label Sep 9, 2018

@bgelens bgelens force-pushed the bgelens:automationresources branch from 608dfaf to 11926b1 Sep 9, 2018

@hashibot hashibot bot added the size/XXL label Sep 9, 2018

@bgelens

This comment has been minimized.

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.

stale

@hashibot hashibot bot added the size/XXL label Sep 12, 2018

@bgelens

This comment has been minimized.

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

This comment has been minimized.

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!

@katbyte
Copy link
Contributor

katbyte left a comment

Hi @bgelens,

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

@katbyte katbyte force-pushed the bgelens:automationresources branch from 8d50edb to 71503c8 Oct 17, 2018

@katbyte

This comment has been minimized.

Copy link
Contributor

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 terraform-providers:master Oct 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

katbyte added a commit that referenced this pull request Oct 17, 2018

@bgelens

This comment has been minimized.

Copy link
Contributor Author

bgelens commented Oct 18, 2018

Great! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment