-
Notifications
You must be signed in to change notification settings - Fork 3
DA rally feedback #336
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
DA rally feedback #336
Conversation
|
/run pipeline |
ocofaigh
left a comment
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.
see comments
| } | ||
| } | ||
|
|
||
| variable "kms_endpoint_type" { |
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 think we can keep the variable but hide it in the ibm_catalog.json
ibm_catalog.json
Outdated
| { | ||
| "key": "event_notifications_email_list" | ||
| "key": "event_notifications_email_list", | ||
| "type": "array" |
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.
we need to add the custom array type for strings:
{
"key": "event_notifications_email_list",
"type": "array",
"custom_config": {
"grouping": "deployment",
"original_grouping": "deployment",
"config_constraints": {
"type": "string"
}
}
},
ibm_catalog.json
Outdated
| { | ||
| "key": "event_notifications_email_list" | ||
| "key": "event_notifications_email_list", | ||
| "type": "array" |
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.
we need to add the custom array type for strings:
{
"key": "event_notifications_email_list",
"type": "array",
"custom_config": {
"grouping": "deployment",
"original_grouping": "deployment",
"config_constraints": {
"type": "string"
}
}
},
ocofaigh
left a comment
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.
see latest comments
| } | ||
|
|
||
| variable "skip_event_notifications_iam_authorization_policy" { | ||
| variable "skip_secrets_manager_event_notifications_iam_authorization_policy" { |
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.
getting very long, maybe shorten iam_authorization -> iam_auth in the name? same for the other related inputs
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.
in both DA variations of course
| } | ||
|
|
||
| variable "skip_sm_ce_iam_authorization_policy" { | ||
| variable "skip_secrets_manager_certificate_engine_iam_auth_policy" { |
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.
certificate engine is not correct here. This boolean controls the IAM credentials engine
| } | ||
|
|
||
| variable "skip_sm_ce_iam_authorization_policy" { | ||
| variable "skip_secrets_manager_certificate_engine_iam_auth_policy" { |
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.
certificate engine is not correct here. This boolean controls the IAM credentials engine
ocofaigh
left a comment
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.
just spotted 1 more thing
|
/run pipeline |
1 similar comment
|
/run pipeline |
|
/run pipeline |
1 similar comment
|
/run pipeline |
|
🎉 This PR is included in version 2.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Description
Hit list for small issues raised during DA rally, main issue: #333
Outstanding issue is how do we want to handle email validation?
skip_sm_ce_iam_authorization_policy → skip_secrets_manager_certificate_engine_iam_authorization_policy
skip_sm_ce_iam_authorization_policy - Description clarifying why to skip
allowed_network - link to corresponding sm documentation added
skip_sm_kms_iam_authorization_policy → skip_secrets_manager_kms_iam_authorization_policy (do we expand KMS too?)
ibmcloud_kms_api_key - clarify when it is(n't) needed
kms_endpoint_type → removes
event_notifications_email_list → does not use the right input widget (should be using the string array widget)
skip_event_notifications_iam_authorization_policy → skip_secrets_manager_event_notifications_iam_authorization_policy
Release required?
x.x.X)x.X.x)X.x.x)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:
Checklist for reviewers
For mergers