refactor: cross object referencing for input variables #525
Conversation
|
/run pipeline |
|
/run pipeline |
shemau
left a comment
shemau
left a comment
There was a problem hiding this comment.
This is insufficient to upgrade to 1.9. When changing versions, it should be in all examples, modules and solutions.
|
@shemau i have updated the terraform version in examples.. please take a look again.. I need suggestion on which approach to take for variable validation to be consistent across all ICD's |
|
/run pipeline |
|
A few points here, review mysql as well. These commits change the main module, so they are going to require a release with a release note stating that required terraform version is being updated. Otherwise it is impossible to track the release notes for the next release to make sure that this information is shared. The ICD modules are 80% similar, whilst they could be 95% similar. The closer to 95% we can get, the easier it is to cherry pick commits from one instance to the other seven. There are some minor differences, configuration on some but not others, default/minimum spec on rabbitmq messaging is higher. But generally most of the new features could be cherry picked, or copy and pasted from one repo to the other. So, almost all changes that look to fix issues or add features that reduces the similarity should be avoided. |
| (var.use_ibm_owned_encryption_key || var.backup_encryption_key_crn != null) || (var.use_default_backup_encryption_key || var.use_same_kms_key_for_backups), | ||
| (var.use_ibm_owned_encryption_key || var.backup_encryption_key_crn == null) || (!var.use_same_kms_key_for_backups) | ||
| ]) | ||
| error_message = "If IBM owned encryption is used then 'kms_key_crn' and 'backup_encryption_key_crn' should be null. If not, 'kms_key_crn' should be provided and if 'backup_encryption_key_crn' is not provided 'use_same_kms_key_for_backups' or 'use_default_backup_encryption_key' should be true.If 'backup_encryption_key_crn' is provided then 'use_same_kms_key_for_backups' should be set to false" |
There was a problem hiding this comment.
This is definitely not as readable as the mysql solution.
From a consumability perspective this is confusing, there are four different tests, some of them do not even reference use_default_backup_encryption_key. Having four validations, with four specific error messages is more helpful to the consumer. Also having all validation on one of the variables in the validation is more helpful from an on going maintenance perspective.
I am not entirely getting the implications of the alltrue block. I do not think there is any difference to having 4 conditions, they must inherently all be true, because any one that was false would trigger a more specific error message.
|
I am closing this PR. This will not be merged. It would be better if all ICD's are taken care by a single person. |
Description
Validation using cross-reference between input variables.
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Validation using cross-reference between input variables.
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:
Checklist for reviewers
For mergers