-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_dev_center_project_environment_type
#26941
New Resource: azurerm_dev_center_project_environment_type
#26941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @neil-yechenwei ,
Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍
internal/services/devcenter/dev_center_project_environment_type.go
Outdated
Show resolved
Hide resolved
internal/services/devcenter/dev_center_project_environment_type.go
Outdated
Show resolved
Hide resolved
website/docs/r/dev_center_project_environment_type.html.markdown
Outdated
Show resolved
Hide resolved
@ms-zhenhua , thanks for the comments. I updated PR and left suggestion. Please take another look. |
@ms-zhenhua , thanks for the comments. I updated PR. Please take another look. Below is the latest test result I just now triggered. All related test cases passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @neil-yechenwei ,
thank you for your updates. LGTM~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neil-yechenwei. Please take a look at the comments left in-line. Once those are resolved this should be good to go.
properties := resp.Model | ||
if properties == nil { | ||
return fmt.Errorf("retrieving %s: `model` was nil", id) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the very unlikely event that Model
is returned to us as null, this should be nil checked in the Update
as well. For consistency can you please update this to:
properties := resp.Model | |
if properties == nil { | |
return fmt.Errorf("retrieving %s: `model` was nil", id) | |
} | |
if resp.Model == nil { | |
return fmt.Errorf("retrieving %s: `model` was nil", id) | |
} | |
if resp.Model.Properties == nil { | |
return fmt.Errorf("retrieving %s: `properties` was nil", id) | |
} | |
payload := resp.Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
identity_ids = [azurerm_user_assigned_identity.test.id] | ||
} | ||
|
||
creator_role_assignment_roles = [split("/", data.azurerm_role_definition.test.id)[length(split("/", data.azurerm_role_definition.test.id)) - 1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this expression can be replaced by using the role_definition_id
since that is the GUID/UUID of the role definition
creator_role_assignment_roles = [split("/", data.azurerm_role_definition.test.id)[length(split("/", data.azurerm_role_definition.test.id)) - 1]] | |
creator_role_assignment_roles = [data.azurerm_role_definition.test.role_definition_id] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
user_role_assignment { | ||
user_id = azurerm_user_assigned_identity.test.principal_id | ||
roles = [split("/", data.azurerm_role_definition.test.id)[length(split("/", data.azurerm_role_definition.test.id)) - 1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above.
creator_role_assignment_roles = [split("/", data.azurerm_role_definition.test.id)[length(split("/", data.azurerm_role_definition.test.id)) - 1], split("/", data.azurerm_role_definition.test2.id)[length(split("/", data.azurerm_role_definition.test2.id)) - 1]] | ||
|
||
user_role_assignment { | ||
user_id = azurerm_user_assigned_identity.test.principal_id | ||
roles = [split("/", data.azurerm_role_definition.test.id)[length(split("/", data.azurerm_role_definition.test.id)) - 1], split("/", data.azurerm_role_definition.test2.id)[length(split("/", data.azurerm_role_definition.test2.id)) - 1]] | ||
} | ||
|
||
user_role_assignment { | ||
user_id = azurerm_user_assigned_identity.test2.principal_id | ||
roles = [split("/", data.azurerm_role_definition.test2.id)[length(split("/", data.azurerm_role_definition.test2.id)) - 1]] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above.
|
||
* `dev_center_project_id` - (Required) The ID of the associated Dev Center Project. Changing this forces a new resource to be created. | ||
|
||
* `deployment_target_id` - (Required) The ID of the subscription that the environment type will be mapped to. The environment's resources will be deployed into this subscription. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `deployment_target_id` - (Required) The ID of the subscription that the environment type will be mapped to. The environment's resources will be deployed into this subscription. | |
* `deployment_target_id` - (Required) The ID of the subscription that the Environment Type will be mapped to. The environment's resources will be deployed into this subscription. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
@stephybun , thanks for the comments. I updated PR. Please take another look. Below is the test result I just now triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neil-yechenwei LGTM 👍
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
This PR is to support new resource Dev Center Project Environment Type.
API Spec: https://github.com/Azure/azure-rest-api-specs/blob/c4a450f92c5a0b0cb70828d3d1588a2e690b80db/specification/devcenter/resource-manager/Microsoft.DevCenter/stable/2023-04-01/devcenter.json#L2128
Overview: https://learn.microsoft.com/en-us/azure/deployment-environments/how-to-configure-project-environment-types
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_dev_center_project_environment_type
This is a (please select all that apply):
Related Issue(s)
Fixes #26706
Note
If this PR changes meaningfully during the course of review please update the title and description as required.