feat: initial commit #1
Conversation
variables.tf Outdated
| } | ||
| } | ||
| | ||
| variable "key_protect_region" { |
There was a problem hiding this comment.
Do we really need? Doesn't the KMS instance also have to be in the same region as the data engine instance anyway (double check this)? So the region variable should suffice here?
There was a problem hiding this comment.
There was a problem hiding this comment.
ok so lets rename it to kms_region to be consistent with other variables
examples/complete/main.tf Outdated
| providers = { | ||
| restapi = restapi.kp | ||
| } | ||
| source = "git::https://github.com/terraform-ibm-modules/terraform-ibm-key-protect-all-inclusive.git?ref=v3.1.2" |
There was a problem hiding this comment.
can you use the latest version of terraform-ibm-key-protect-all-inclusive - that way you can remove the restapi provider completely from the example.
There was a problem hiding this comment.
This is still an old version
ocofaigh left a comment
ocofaigh left a comment There was a problem hiding this comment.
I also notice the module is missing the code to create the KMS auth policy (and the variable skip_iam_authorization_policy to skip creating it). Can you add please?
| @Aayush-Abhyarthi no point in rebasing the PR (which kicks off the pipeline tests and provisions things in our account which has an associated cost) until PR comments have been addressed |
variables.tf Outdated
| } | ||
| } | ||
| | ||
| variable "key_protect_region" { |
There was a problem hiding this comment.
ok so lets rename it to kms_region to be consistent with other variables
| } | ||
| } | ||
| | ||
| variable "skip_iam_authorization_policy" { |
There was a problem hiding this comment.
We have decided to go with 1 extra bool variable called kms_encryption_enabled so that we can default the skip_iam_authorization_policy variable to false. Please take a look at the changes in terraform-ibm-modules/terraform-ibm-icd-postgresql#186 and update this PR to be the same. Thanks
|
scrap that, auto scaling not supported for data engine |
ocofaigh left a comment
ocofaigh left a comment There was a problem hiding this comment.
Pushed some final cleanups to this branch so it is consistent with other modules. Approving for merge so we can get the first release out. There are some gaps we need to fill such as support for CBRs and service credentials, but I'll create follow on issues for those.
| 🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Migrate Data Engine module to TIM
Types of changes in this PR
No release required
Release required
x.x.X): Change that fixes an issue and is compatible with earlier versions)x.X.x): Change that adds functionality and is compatible with earlier versions)X.x.x): Change that is likely incompatible with previous versions)Release notes content
Replace this text with information that users need to know about the bug fixes, features, and breaking changes. This information helps the merger write the commit message that is published in the release notes for the module.
Checklist for reviewers
Merge actions for mergers
Merge by using "Squash and merge".
Use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author.
The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).