terraform-ibm-modules / terraform-ibm-icd-elasticsearch

Implements an instance of the IBM Cloud Databases for Elasticsearch service.
Apache License 2.0
0 stars 1 forks source link

feat: allow exist es #243

Closed kierramarie closed 2 months ago

kierramarie commented 3 months ago

Description

Support for using an existing Elasticsearch db has been added to the DA. Variables and Outputs have been updated to reflect change.

Release required?

Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

For mergers

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

in-1911 commented 3 months ago

Other modules implementing this pattern (reference an existing instance) use CRN as a parameter. This actually makes it simpler to bind an existing instance because there is no need to know the ES instance resource group or region.

shemau commented 3 months ago

There are multiple issues that need resolution:

Indirectly related to this PR, but important is a test gap introduced in 1.15.0. It appears that previously there were DA and DA upgrade tests. This was replaced with a single DA in schematics test.

This is important because module.elasticsearch changes to module.elasticsearch[0] under this PR. It is unclear if the upgrade path is tested, so it is unclear if a moved block is required or not.

The inputs were chosen because they are required for the data source for ibm_database. A CRN is not an option, but it may be possible to use a CRN to generate GUID and location/region via ibm_resource_instance; to get the name and resource group to pass to ibm_database. The GUID (from the CRN) should be unique and as such would not require scoping down the search by name/resource group on ibm_resource_instance data source.

Can we avoid elasticsearch in the variable/local/module names? This will help making the code more portable to other ICD modules. So maybe var.existing_instance_crn, local.use_existing_instance or local.use_existing_database, data.ibm_database.existing_instance and a new data.ibm_resource_instance.existing_instance.

Finally, are we confident that data.ibm_database_connection.existing_connection works? Do we know if that connection was created or should the connection information always be a new resource (rather than a data source)?

kierramarie commented 3 months ago

@shemau I have made the changes that were requested. I updated the variable names to say db_instance instead. I still wanted to distinguish between that and the existing_kms variables. Also, right now we do not have a way to verify that there is a connection. If it was made not using this DA it might not have one.

I also needed to add the provider bit to allow the resource_instance to work properly.

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

in-1911 commented 3 months ago

You need the moved block:

TestRunStandardUpgradeSolution 2024-08-02T15:57:43Z command.go:185:   # module.elasticsearch.module.elasticsearch.ibm_database.elasticsearch will be destroyed
TestRunStandardUpgradeSolution 2024-08-02T15:57:43Z command.go:185:   # (because module.elasticsearch.module.elasticsearch is not in configuration)
kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

shemau commented 3 months ago

I think the moved statement should go in a separate file called moved.tf. Also the moved resource is just module.elasticsearch at the top level (which then goes to fscloud, in turn to the root module), but don't quote me on that.

moved {
  from = module.elasticsearch
  to   = module.elasticsearch[0]
}
kierramarie commented 3 months ago

/run pipeline

kierramarie commented 3 months ago

/run pipeline

toddgiguere commented 2 months ago

/run pipeline

terraform-ibm-modules-ops commented 2 months ago

:tada: This issue has been resolved in version 1.17.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: