-
Notifications
You must be signed in to change notification settings - Fork 91
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
ec_deployment: add support for data tiers #253
Conversation
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
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.
Left a few editing suggestions. LGTM for the documentation part.
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Co-authored-by: alaudazzi <46651782+alaudazzi@users.noreply.github.com>
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.
This is looking great @marclop 🎉 ! Just wondering if this PR should include an update to the ec_deployment
data source as well 🤔 shouldn't be too big of a change I think? Otherwise I guess that should be tackled as a follow up
t.Helper() | ||
requiresAPIConn(t) | ||
|
||
deploymentTpl := setDefaultTemplate(region, depTpl) | ||
esIC, kibanaIC, apmIC, essIC, err := setInstanceConfigurations(deploymentTpl) | ||
// esIC is no longer needed |
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.
// esIC is no longer needed | |
// esIC is no longer needed after the implementation of node roles |
resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.node_type_master", ""), | ||
resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.node_type_ml", ""), | ||
resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.id", "hot_content"), | ||
resource.TestCheckResourceAttrSet(ccsResName, "elasticsearch.0.topology.0.node_roles.#"), |
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 check that the correct node roles are set in this and all tests?
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 thought about it, but node_roles
are likely to evolve over time, if we're too strict in our assertions we'll end up having a lot of maintenance overhead, so I've opted to at least for now check this lightly rather than assert all the node roles for all topology elements.
@karencfv I'll add the |
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
…/terraform-provider-ec into f/add-data-tiers-support
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 adding this to the data source as well!
Tests actually passed, the post destroy tracking is what returned an error, so we're good to merge. |
Description
I've split the pull request in individual commits which should make it
easier to review.
Adds support for
node_roles
and automatic migration fromnode_type_*
to
node_roles
. I will note a few things to make it easier to breakdown all of these changes.
f972aa8:
Adds
node_roles
support by updating the SDK to the latest availableversion, this makes the deployment template not work out of the box and
instead our client has to choose whether to use the
node_type
objector the
node_roles
array.Stack versions >=
7.10.0
can and should usenode_roles
since thatallows users to leverage the
cold
tier and autoscaling in the futureas well.
With the introduction of the
"id"
field, the topology elements needn'tuse
instance_configuration_id
as the unique field, instead,id
isthat unique field value.
The
instance_configuration_id
has been set as computed only andcannot be set by the user anymore. For now, this change is present on
the Elasticsearch topology only, in the future, as other applications
might also auto scale, the API will change accordingly.
Last, it validates that a set topology element has a size > 0, otherwise
an error will be thrown.
99dfe21:
Another somewhat breaking change is the removal of the version computed
property in the Elasticsearch object. This field is completely redundant
and should not be used, instead the global version declares the version
for all of the deployment's applications. Since we're removing a field
from the schema and previous versions of the provider have saved it in
the state, we're adding a
StateUpgradeFunc
and bumping the currentschema version to
1
. The schema upgrade function will only be run whenthe saved state version is
0
and will only run once, then the stateschema version will be bumped to 1.
The rest of commits are updates to the tests, fixtures, documentation
and example files.
Related Issues
Closes #212
How Has This Been Tested?
Thoroughly unit and acceptance tested, additionally I've tested the same scenarios as the acceptance tests manually.
Types of Changes