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 & data source: azurerm_managed_application_definition #6211

Merged
merged 18 commits into from
Mar 28, 2020

Conversation

neil-yechenwei
Copy link
Contributor

This PR is the implement of the issue #5294

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

@neil-yechenwei
Copy link
Contributor Author

@magodo , Thanks for your comments. I've updated code. Please have a look.

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

I have left some additional mostly minor comments that once addressed this should be good to merge 👍

@neil-yechenwei
Copy link
Contributor Author

@magodo , Thanks for your comments. I've updated code.

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.

Thanks for the PR @neil-yechenwei, overall this looks good to me with just a couple comments inline. My main concern is conditionally setting things on read, typically we always read everything in.

}

if (parameters.CreateUIDefinition != nil && parameters.MainTemplate == nil) || (parameters.CreateUIDefinition == nil && parameters.MainTemplate != nil) {
return fmt.Errorf("`create_ui_definition` and `main_template` should be set or not set together")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think saying if one is set the other must be too covers all conditions:

Suggested change
return fmt.Errorf("`create_ui_definition` and `main_template` should be set or not set together")
return fmt.Errorf("if either `create_ui_definition` or `main_template` is set the other one must be too")

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 add a note about this in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment on lines 232 to 240
if v, ok := d.GetOk("create_ui_definition"); ok {
d.Set("create_ui_definition", v.(string))
}
if v, ok := d.GetOk("main_template"); ok {
d.Set("main_template", v.(string))
}
if v, ok := d.GetOk("package_file_uri"); ok {
d.Set("package_file_uri", v.(string))
}
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 conditionally setting these? typically we always read in all propertie

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've debugged code. I found create_ui_definition , main_template and package_file_uri returned nil from service side. So I have 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.

lets add a comment in here stating that

}

resource "azurerm_resource_group" "test" {
name = "acctestRG-managedapplication-%d"
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets go with mapp here, just a short reminder/identifier so we can spot services potentially not cleaning up after themselves

Suggested change
name = "acctestRG-managedapplication-%d"
name = "acctestRG-mapp-%d"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@@ -33,6 +33,7 @@ Load Balancer
Log Analytics
Logic App
Machine Learning
Managed Application
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be

Suggested change
Managed Application
Managed Applications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* `lock_level` - (Required) Specifies the managed application lock level. Valid values include `CanNotDelete`, `None`, `ReadOnly`. Changing this forces a new resource to be created.

* `create_ui_definition` - (Optional) Specifies the createUiDefinition json for the backing template with Microsoft.Solutions/applications resource.
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
* `create_ui_definition` - (Optional) Specifies the createUiDefinition json for the backing template with Microsoft.Solutions/applications resource.
* `create_ui_definition` - (Optional) Specifies the `createUiDefinition` json for the backing template with `Microsoft.Solutions/applications` resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.

* `authorization` - (Required) One or more `authorization` block defined below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be nice to explain what the authorization blocks correspond to. I question if authorization is the right name for the property

Copy link
Contributor Author

@neil-yechenwei neil-yechenwei Mar 27, 2020

Choose a reason for hiding this comment

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

It is used to grant permission to the managed resource group. After checked, seems azure portal and powershell and cli all named this property as "authorization". So I think maybe we should align with them, then user is easily to understand.

azure portal:
image

powershell:
image

azure cli:
image


* `description` - (Optional) Specifies the managed application definition description.

* `package_enabled` - (Optional) Is enabled for this package? Defaults to `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 think this needs rewording

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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.

Hey @neil-yechenwei, thanks for the PR. I've left only a couple comments inline, and i hope you don't mind but i am going to push the required changes soi can mege this. As well as some naming convention changes we are starting to roll out (resource/datasource at the end of the file name, dropping Arm from function names)


"location": azure.SchemaLocation(),

"lock_level": {
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 arrange these required properties alphabetically

Comment on lines 232 to 240
if v, ok := d.GetOk("create_ui_definition"); ok {
d.Set("create_ui_definition", v.(string))
}
if v, ok := d.GetOk("main_template"); ok {
d.Set("main_template", v.(string))
}
if v, ok := d.GetOk("package_file_uri"); ok {
d.Set("package_file_uri", v.(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 add a comment in here stating that

@katbyte katbyte added this to the v2.4.0 milestone Mar 27, 2020
@katbyte katbyte merged commit ae23b1a into hashicorp:master Mar 28, 2020
katbyte added a commit that referenced this pull request Mar 28, 2020
@ghost
Copy link

ghost commented Apr 2, 2020

This has been released in version 2.4.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.4.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 27, 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 Apr 27, 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