CLOUDP-380507-cluster: update schema and handler implementation#1622
CLOUDP-380507-cluster: update schema and handler implementation#1622
Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: CONTRIBUTING.md |
There was a problem hiding this comment.
Pull request overview
Updates the MongoDB Atlas Cluster CloudFormation resource to align with newer Atlas Admin API/SDK behavior, including new cluster properties and process args support.
Changes:
- Bumps cluster resource implementation to use
go.mongodb.org/atlas-sdk/v20250312013/adminand switches advanced config calls toGetProcessArgs/UpdateProcessArgs. - Extends the CFN schema/model and docs with new cluster properties (e.g., config server management mode, scaling strategy, log redaction) and new process args fields.
- Adjusts DiskSizeGB handling to map the CFN top-level
DiskSizeGBto per-hardware-spec disk sizing in the newer API.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cfn-resources/cluster/mongodb-atlas-cluster.json | Adds new schema properties (cluster + process args) and marks RetainBackups as write-only. |
| cfn-resources/cluster/docs/processargs.md | Documents the two new process args fields. |
| cfn-resources/cluster/docs/README.md | Documents newly added top-level cluster properties. |
| cfn-resources/cluster/cmd/resource/resource.go | Switches to the newer SDK client and updates process args API usage; supports RetainBackups on delete. |
| cfn-resources/cluster/cmd/resource/model.go | Regenerates model structs to include new fields. |
| cfn-resources/cluster/cmd/resource/mappings.go | Updates mapping logic for the new SDK types, adds new fields, and introduces DiskSizeGB translation helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fmt.Printf("specs: len %d %+v", len(replicationSpecs), rSpecs) | ||
| return rSpecs |
There was a problem hiding this comment.
There is a leftover debug print (fmt.Printf) in expandReplicationSpecs that will write cluster configuration details to stdout/logs on every create/update. Please remove this line or replace it with the project’s structured logger at an appropriate debug level (and ensure it’s gated/disabled by default).
| // NumShards is not supported in the v20250312013 API; each shard must be a separate ReplicationSpec entry. | ||
| if replicationSpecs[i].ZoneName != nil { | ||
| rSpec.ZoneName = admin20231115014.PtrString(cast.ToString(replicationSpecs[i].ZoneName)) | ||
| rSpec.ZoneName = admin.PtrString(cast.ToString(replicationSpecs[i].ZoneName)) | ||
| } |
There was a problem hiding this comment.
NumShards is still present in the CFN schema/model (AdvancedReplicationSpec.NumShards), but this mapping now ignores it (comment says the API no longer supports it). This will silently drop a user-specified value. Either remove/deprecate NumShards from the schema/docs (and regenerate), or explicitly return an InvalidRequest error when NumShards is set so users don’t think it’s applied.
| // expandAdvancedSettings maps the CFN ProcessArgs model to the v20250312013 process args request. | ||
| // Note: DefaultReadConcern and FailIndexKeyTooLong are not sent as they were removed from the API. | ||
| func expandAdvancedSettings(processArgs ProcessArgs) *admin.ClusterDescriptionProcessArgs20240805 { | ||
| var args admin.ClusterDescriptionProcessArgs20240805 | ||
|
|
There was a problem hiding this comment.
The implementation intentionally no longer sends or populates DefaultReadConcern and FailIndexKeyTooLong, but these properties remain in the CFN schema/docs. As a result, stack updates that set these fields cannot ever converge to the desired state. Please either (a) remove/deprecate these properties in the schema/docs, or (b) add validation that fails the request when they are set (with a clear message) so the behavior is explicit.
| if currentModel.ReplicationSpecs != nil { | ||
| adminRepSpecs := expandReplicationSpecs(currentModel.ReplicationSpecs) | ||
| // Apply top-level DiskSizeGB to all hardware specs in the request | ||
| if currentModel.DiskSizeGB != nil { | ||
| applyDiskSizeGBToSpecs(adminRepSpecs, currentModel.DiskSizeGB) | ||
| } | ||
| clusterRequest.ReplicationSpecs = &adminRepSpecs | ||
| } |
There was a problem hiding this comment.
DiskSizeGB is only applied to the Atlas request when ReplicationSpecs is provided. Since ReplicationSpecs is not a required CFN property, users can update DiskSizeGB without specifying replication specs and this change would be silently ignored. Consider handling the case where DiskSizeGB is set but ReplicationSpecs is nil (e.g., fetch current replication specs and apply the disk size to them, or return an InvalidRequest error instructing users to include ReplicationSpecs).
| // diskSizeGBFromSpecs reads the DiskSizeGB from the first electable spec of the first replication spec. | ||
| // In the v20250312013 API, DiskSizeGB is per hardware spec rather than a top-level cluster field. | ||
| func diskSizeGBFromSpecs(replicationSpecs []admin.ReplicationSpec20240805) *float64 { | ||
| for _, spec := range replicationSpecs { | ||
| for _, rc := range spec.GetRegionConfigs() { | ||
| if rc.ElectableSpecs != nil && rc.ElectableSpecs.DiskSizeGB != nil { | ||
| return rc.ElectableSpecs.DiskSizeGB | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // applyDiskSizeGBToSpecs applies the given diskSizeGB value to all hardware specs in all replication specs. | ||
| // This maps the CFN top-level DiskSizeGB property to the per-spec field required by the v20250312013 API. | ||
| func applyDiskSizeGBToSpecs(replicationSpecs []admin.ReplicationSpec20240805, diskSizeGB *float64) { | ||
| if diskSizeGB == nil { | ||
| return | ||
| } | ||
| for i := range replicationSpecs { | ||
| if replicationSpecs[i].RegionConfigs == nil { | ||
| continue | ||
| } | ||
| for j := range *replicationSpecs[i].RegionConfigs { | ||
| rc := &(*replicationSpecs[i].RegionConfigs)[j] | ||
| if rc.ElectableSpecs != nil { | ||
| rc.ElectableSpecs.DiskSizeGB = diskSizeGB | ||
| } | ||
| if rc.AnalyticsSpecs != nil { | ||
| rc.AnalyticsSpecs.DiskSizeGB = diskSizeGB | ||
| } | ||
| if rc.ReadOnlySpecs != nil { | ||
| rc.ReadOnlySpecs.DiskSizeGB = diskSizeGB | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new disk-size translation helpers (diskSizeGBFromSpecs / applyDiskSizeGBToSpecs) introduce non-trivial behavior that impacts create/update/read, but there are no unit tests covering them. Please add tests that verify DiskSizeGB is extracted from electable specs and propagated to electable/analytics/readOnly specs across region configs, and that nil/missing specs are handled correctly.
| "<a href="#tags" title="Tags">Tags</a>" : <i>[ <a href="tag.md">tag</a>, ... ]</i>, | ||
| "<a href="#configservermanagementmode" title="ConfigServerManagementMode">ConfigServerManagementMode</a>" : <i>String</i>, | ||
| "<a href="#replicasetscalingstrategy" title="ReplicaSetScalingStrategy">ReplicaSetScalingStrategy</a>" : <i>String</i>, | ||
| "<a href="#acceptdatarisksandforcereplicasetreconfig" title="AcceptDataRisksAndForceReplicaSetReconfig">AcceptDataRisksAndForceReplicaSetReconfig</a>" : <i>String</i>, | ||
| "<a href="#retainbackups" title="RetainBackups">RetainBackups</a>" : <i>Boolean</i>, | ||
| "<a href="#redactclientlogdata" title="RedactClientLogData">RedactClientLogData</a>" : <i>Boolean</i> |
There was a problem hiding this comment.
The PR description still contains the template placeholders (e.g., _Jira ticket:_ CLOUDP-#) and no summary/QA info. Please update the PR description to reference CLOUDP-380507 and briefly describe the behavior change and validation/QA performed so reviewers can assess impact.
Proposed changes
Jira ticket: CLOUDP-#
Please include a summary of the fix/feature/change, including any relevant motivation and context.
Link to any related issue(s):
Type of change:
expected)
Manual QA performed:
Required Checklist:
make fmtand formatted my codeworks in Atlas
Further comments