-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Convert tags, labels, timeouts, advanced_configuration, bi_connector_config and pinned_fcv #21
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
return ret | ||
} | ||
|
||
// RemoveLeadingNewline removes the first newline if it exists to make the output prettier. |
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.
e.g for timeouts, HC funcs don't delete the newline after the block is opened
instance_size = "M10" | ||
disk_size_gb = 100 | ||
} | ||
}, { |
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.
change format for arrays because TF formatting is not clear when arrays bracket are with the first/last object, the intermediate }, {
is not indented correctly.
This format takes a few more lines but it's more clear, especially for long cluster definitions.
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!!
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.
Very nice work!
} | ||
fillBlockOpt(resourceb, nTimeouts) | ||
fillBlockOpt(resourceb, nAdvConf) | ||
fillBlockOpt(resourceb, nBiConnector) |
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.
love this 1 line additions
] | ||
tags = { | ||
environment = "dev" | ||
(var.tag_key) = var.tag_value |
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!
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 for addressing all comments!
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
pinned_fcv { | ||
# comments in pinned_fcv are kept | ||
expiration_date = var.fcv_date | ||
version = var.fcv_date | ||
} |
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: version
is a computed-only attribute in both cluster and advanced_cluster so no need to handle it
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.
ah, thanks, we're just moving the block entirely, but will remove the version attribute in a follow-up PR so the example is more real
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.
@lantoli nit: it's preferable to comment the code and create a CLOUDP to help everyone giving confidence we don't forget this
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.
provider_name = "AWS" | ||
provider_instance_size_name = "M10" | ||
replication_specs { | ||
num_shards = 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.
question not related to PR, how will we handle num_shards > 1? Will we have logic in the conversion to define multiple replication specs?
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.
to be honest, i haven't thought about it yet, but I guess it'll be in the func creating the replication_specs, that will create multiple ones
Description
Convert tags, labels, timeouts, advanced_configuration, bi_connector_config and pinned_fcv.
It also improves arrays formatting.
Link to any related issue(s): CLOUDP-299725
Type of change:
Required Checklist:
Further comments