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 : azurerm_mongo_cluster #27636

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Oct 12, 2024

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

New resource azurerm_mongo_cluster.

Swagger:https://github.com/Azure/azure-rest-api-specs/blob/15b16d1b5c3cccdecdd1cfe936f6a8005680c557/specification/mongocluster/resource-manager/Microsoft.DocumentDB/stable/2024-07-01/mongoCluster.json#L279

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Test results:

PASS: TestAccMongoCluster_basic (1257.27s)
PASS: TestAccMongoCluster_requiresImport (1592.76s)
PASS: TestAccMongoCluster_update (2704.27s)
PASS: TestAccMongoCluster_previewFeature (2373.41s)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #24161

Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

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

Hi @sinbai,

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 👍

ValidateFunc: validation.All(
validation.StringLenBetween(3, 40),
validation.StringMatch(
regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`),
Copy link
Contributor

Choose a reason for hiding this comment

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

seems the name cannot start with a hyphen but can end with a hyphen?
maybe you can refer to this code to combine these 2 validations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be regexp.MustCompile(^[a-z\d][-a-z\d]{1,38}[a-z\d]$) because the name should have at least 3 characters.

Properties: &mongoclusters.MongoClusterProperties{},
}

if _, ok := metadata.ResourceData.GetOk("administrator_login"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we use AdministratorLogin != "" instead of metadata.ResourceData.GetOk like other existing typed resources did? please also update other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

if _, ok := metadata.ResourceData.GetOk("administrator_login"); ok {
parameter.Properties.Administrator = &mongoclusters.AdministratorProperties{
UserName: pointer.To(state.AdministratorLogin),
Password: pointer.To(state.AdministratorLoginPassword),
Copy link
Contributor

Choose a reason for hiding this comment

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

since administrator_login_password is optional, do you need to judge whether 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.

I added RequiredWith: []string{"administrator_username", "administrator_password"} for administrator_username and administrator_password properties. That's means once administrator_username is specified, the administrator_password is required. So, I assume that administrator_password doesn't need to be judged whether it exists here, what do you think?

})
}

func TestAccMongoCluster_update(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we also test a scenario that the elements of preview_features are changed?

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 assume that ForceNew property preview_features is not needed to test changes, right?


switch state.CreateMode {
case string(mongoclusters.CreateModeDefault):
if _, ok := metadata.ResourceDiff.GetOk("administrator_login"); !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we use state,AdministratorLogin != "" instead of metadata.ResourceDiff.GetOk ? Please also confirm other places below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


* `administrator_login` - (Optional) The administrator username of the Azure Cosmos DB for MongoDB vCore. Changing this forces a new resource to be created.

* `create_mode` - (Optional) The creation mode for the Azure Cosmos DB for MongoDB vCore. Possibles values are `Default` and `GeoReplica`. Defaults to 'Default'. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `create_mode` - (Optional) The creation mode for the Azure Cosmos DB for MongoDB vCore. Possibles values are `Default` and `GeoReplica`. Defaults to 'Default'. Changing this forces a new resource to be created.
* `create_mode` - (Optional) The creation mode for the Azure Cosmos DB for MongoDB vCore. Possibles values are `Default` and `GeoReplica`. Defaults to `Default`. Changing this forces a new resource to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


* `create_mode` - (Optional) The creation mode for the Azure Cosmos DB for MongoDB vCore. Possibles values are `Default` and `GeoReplica`. Defaults to 'Default'. Changing this forces a new resource to be created.

-> **Note** The creation mode "GeoReplica" is currently in preview. It is only available when specified via `preview_features`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-> **Note** The creation mode "GeoReplica" is currently in preview. It is only available when specified via `preview_features`.
-> **Note** The creation mode `GeoReplica` is currently in preview. It is only available when `preview_features` is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


* `source_location` - (Optional) The location of the source Azure Cosmos DB for MongoDB vCore. Changing this forces a new resource to be created.

* `source_resource_id` - (Optional) The ID of the replication source Azure Cosmos DB for MongoDB vCore. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be source_server_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


* `high_availability_mode` - (Optional) The high availability mode for the Azure Cosmos DB for MongoDB vCore. Possibles values are `Disabled` and `ZoneRedundantPreferred`.

* `public_network_access_enabled` - (Optional) Whether public network access is allowed for the Azure Cosmos DB for MongoDB vCore. Defaults to 'true'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `public_network_access_enabled` - (Optional) Whether public network access is allowed for the Azure Cosmos DB for MongoDB vCore. Defaults to 'true'.
* `public_network_access_enabled` - (Optional) Whether public network access is allowed for the Azure Cosmos DB for MongoDB vCore. Defaults to `true`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}, false),
},

"preview_features": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Elements in list can be duplicated. What will happen if duplicated elements are sent to Azure?
Will Azure reorder the element sequence in response if more than 1 elements are sent to Azure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If duplicated elements are sent to Azure, the API returns a 400 Bad Request with an error message for duplicate array value.

Since there is currently only one possible value for preview_features(it could not be deleted once specified). I am not sure whether the API would reorder the elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add the validation for the duplicated values in the customizeDiff function so that such error can be found in plan stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sinbai
Copy link
Contributor Author

sinbai commented Oct 16, 2024

Hi @sinbai,

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 👍

Hi @ms-zhenhua thanks for your time and your feedback. I have fixed/replied to the comments. Could you please take another look?

parameter.Properties.CreateMode = pointer.To(mongoclusters.CreateMode(state.CreateMode))
}

if v, ok := metadata.ResourceData.GetOk("preview_features"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we use PreviewFeatures instead of metadata.ResourceData.GetOk("preview_features")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parameter.Properties.PreviewFeatures = expandPreviewFeatures(v.([]interface{}))
}

if _, ok := metadata.ResourceData.GetOk("shard_count"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

since shard_count is non-zero, you may use ShardCount != 0 instead of metadata.ResourceData.GetOk("shard_count")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parameter.Properties.PublicNetworkAccess = pointer.To(mongoclusters.PublicNetworkAccessDisabled)
}

if _, ok := metadata.ResourceData.GetOk("storage_size_in_gb"); ok {
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
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
}

if _, ok := metadata.ResourceData.GetOk("tags"); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use Tags instead of metadata.ResourceData.GetOk("tags")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sinbai
Copy link
Contributor Author

sinbai commented Oct 18, 2024

Hi @ms-zhenhua , thanks for your feedback again. I have fixed the comments. Could you please take another look?

}
}

if len(state.Tags) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(state.Tags) > 0 {
if state.Tags != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

parameter.Properties.CreateMode = pointer.To(mongoclusters.CreateMode(state.CreateMode))
}

if len(state.PreviewFeatures) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we put len(state.PreviewFeatures) > 0 into the expand function as other resources did?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@ms-zhenhua ms-zhenhua left a comment

Choose a reason for hiding this comment

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

Hi @sinbai, thank you for your updates. LGTM~

@sinbai sinbai marked this pull request as ready for review October 21, 2024 07:58
@sinbai sinbai requested review from katbyte and a team as code owners October 21, 2024 07:58
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Hey @sinbai, I left some comments and questions in-line that would be good to have fixed up. Furthermore looking at the Azure documentation and how this resource is referred to in there, I'm wondering if this should actually be part of the cosmosdb service package instead of being separated into it's own mongocluster service package.

Optional: true,
ForceNew: true,
ValidateFunc: validation.StringIsNotEmpty,
RequiredWith: []string{"administrator_username", "administrator_password"},
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to specify the property name that the RequiredWith is applied to

Suggested change
RequiredWith: []string{"administrator_username", "administrator_password"},
RequiredWith: []string{"administrator_password"},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Type: schema.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: validation.StringIsNotEmpty,
Copy link
Member

Choose a reason for hiding this comment

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

This should have a stricter validation

Suggested change
ValidateFunc: validation.StringIsNotEmpty,
ValidateFunc: mongoclusters.ValidateMongoClusterID,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Optional: true,
Sensitive: true,
ValidateFunc: validation.StringIsNotEmpty,
RequiredWith: []string{"administrator_username", "administrator_password"},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RequiredWith: []string{"administrator_username", "administrator_password"},
RequiredWith: []string{"administrator_username"},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

parameter := mongoclusters.MongoCluster{
Location: azure.NormalizeLocation(state.Location),
Copy link
Member

Choose a reason for hiding this comment

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

This function is deprecated, please use location.Normalize

Suggested change
Location: azure.NormalizeLocation(state.Location),
Location: location.Normalize(state.Location),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

metadata.Logger.Info("Decoding state...")
var state MongoClusterResourceModel
if err := metadata.Decode(&state); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

This error should be wrapped like we do in the create

Suggested change
return err
return fmt.Errorf("decoding: %+v", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment on lines 6 to 11
Manages an Azure Cosmos DB for MongoDB vCore.
---

# azurerm_mongo_cluster

Manages an Azure Cosmos DB for MongoDB vCore.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Manages an Azure Cosmos DB for MongoDB vCore.
---
# azurerm_mongo_cluster
Manages an Azure Cosmos DB for MongoDB vCore.
Manages a MongoDB Cluster using vCore Architecture.
---
# azurerm_mongo_cluster
Manages a MongoDB Cluster using vCore Architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -0,0 +1,129 @@
---
subcategory: "MongoCluster"
Copy link
Member

Choose a reason for hiding this comment

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

This should typically have spacing

Suggested change
subcategory: "MongoCluster"
subcategory: "Mongo Cluster"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


The following arguments are supported:

* `name` - (Required) The name which should be used for the Azure Cosmos DB for MongoDB vCore. Changing this forces a new resource to be created.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please update all references in these docs from Azure Cosmos DB for MongoDB vCore to

Suggested change
* `name` - (Required) The name which should be used for the Azure Cosmos DB for MongoDB vCore. Changing this forces a new resource to be created.
* `name` - (Required) The name which should be used for the MongoDB Cluster. Changing this forces a new resource to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


## Import

Monitor Azure Active Directory Diagnostic Settings can be imported using the `resource id`, e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Monitor Azure Active Directory Diagnostic Settings can be imported using the `resource id`, e.g.
MongoDB Clusters can be imported using the `resource id`, e.g.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -73,6 +73,7 @@ Maps
Messaging
Mixed Reality
Mobile Network
MongoCluster
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MongoCluster
Mongo Cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sinbai sinbai force-pushed the mongocluster/new_mongocluster_resource branch from 868b10f to 969187a Compare October 29, 2024 05:10
@sinbai
Copy link
Contributor Author

sinbai commented Oct 29, 2024

Hey @sinbai, I left some comments and questions in-line that would be good to have fixed up. Furthermore looking at the Azure documentation and how this resource is referred to in there, I'm wondering if this should actually be part of the cosmosdb service package instead of being separated into it's own mongocluster service package.

Hi @stephybun thanks for your time and feedback. I have fixed/replied the comments. Could you please take another look?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Hey @sinbai, I left responses to some of the comments. Unless there is some additional evidence that this is an entirely new product, I still think this should be added to the cosmosdb service package instead of creating a new mongocluster service package.

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