-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Converts data sources #24
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
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.
LGTM however we should double check if we want to generate the use_replication_spec_per_shard
attribute
internal/convert/convert.go
Outdated
for _, resource := range parser.Body().Blocks() { | ||
labels := resource.Labels() | ||
resourceName := labels[0] | ||
if resource.Type() != resourceType || resourceName != cluster { | ||
continue | ||
} | ||
resourceb := resource.Body() | ||
if errDyn := checkDynamicBlock(resourceb); errDyn != nil { | ||
return nil, errDyn | ||
} | ||
labels[0] = advCluster | ||
resource.SetLabels(labels) | ||
|
||
if resourceb.FirstMatchingBlock(nRepSpecs, nil) != nil { | ||
err = fillCluster(resourceb) | ||
} else { | ||
err = fillFreeTierCluster(resourceb) | ||
} | ||
converted, err := convertResource(resource) |
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.
nit: the resource
variable name confused me here, this would be a block which can be a resource or data source right?
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, changed here: b1d2a2b
data "mongodbatlas_advanced_cluster" "singular" { | ||
# data source content is kep, only data source type is changed - singular | ||
project_id = mongodbatlas_advanced_cluster.example.project_id | ||
name = mongodbatlas_advanced_cluster.example.name | ||
depends_on = [mongodbatlas_privatelink_endpoint_service.example_endpoint] | ||
|
||
# Generated by atlas-cli-plugin-terraform. | ||
# Please confirm that all references to this resource are updated. | ||
} |
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.
should we include use_replication_spec_per_shard = true
? Might be the most consistent output given we generated advanced_cluster resources with single replication spec per shard (no use of num_shards
).
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.
yes we should because eventually in 2.0 use_replication_spec_per_shard = true
becomes the standard, right?
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.
correct, added this into the v2 release doc as well
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.
great point, added here: 9297c5f
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.
LGTM overall - only one aspect related to use_replication_spec_per_shard = true
project_id = mongodbatlas_advanced_cluster.example.project_id | ||
name = mongodbatlas_advanced_cluster.example.name | ||
depends_on = [mongodbatlas_privatelink_endpoint_service.example_endpoint] | ||
use_replication_spec_per_shard = true |
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.
Nice to use the new format 👍
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.
LGTM!
Description
Converts data sources
Link to any related issue(s): CLOUDP-301789
Type of change:
Required Checklist:
Further comments