-
Notifications
You must be signed in to change notification settings - Fork 1
fix: update architecture diagrams and reorder DAs deployment #26
Conversation
/run pipeline |
/run pipeline |
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.
A couple of suggestions on the order change and a point about using numbers in DA names in general.
@@ -383,7 +309,7 @@ | |||
}, | |||
{ | |||
"name": "existing_kms_instance_crn", | |||
"value": "ref:../../members/2a - Security Service - Key Management/outputs/kms_instance_crn" | |||
"value": "ref:../../members/2 - Security Service - Key Management/outputs/kms_instance_crn" |
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 this should follow the ref scheme we used in other places - i.e. ref:../2 - Security Service - Key Management/outputs/kms_instance_crn
sample_deploy_config.json
Outdated
"3a - Observability - Logging Monitoring Activity Tracker", | ||
"3b - Databases for Elasticsearch", | ||
"4 - Event Notifications", | ||
"5a - Security Service - Secret Manager", | ||
"5b - Security Service - Security Compliance Center", | ||
"6 - Sample RAG app - Application Lifecycle Management", | ||
"7 - Sample RAG app configuration" | ||
"6 - WatsonX SaaS services", |
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.
Logically the Elastic DA should be next to Watson (probably right before that). As is, Elastic sits in the middle of the Security / Observability DAs.
From the same perspective, may be we should put the Security Service DAs together, even though some of them are dependent on Event Notififcations and will not be deployed in that exact order.
Overall, I am starting to think that we might consider dropping the numbering from the member DA names - otherwise we get massive changes every time we change the order or add something in the middle. May be we should hold a vote on that.
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.
^^ @toddgiguere
@in-1911 should we wait for the new secrets manger and event notify DA versions and include those in this PR, along with above changes? |
I would wait at least until terraform-ibm-modules/terraform-ibm-secrets-manager#178 is merged and a new release is published to the catalog. |
As discussed here is the new suggested naming and sequence:
|
/run pipeline |
/run pipeline |
🎉 This issue has been resolved in version 0.3.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
…> - updated architecture diagrams<br> - new naming scheme for stack steps, removed numbering in names<br> - changed order of some steps<br> - updated secrets manager DA to latest * fix: update architecture diagrams * fix: remove extra region param in kms * fix(deps): update to latest version of secret manager * refactor: rename and reorder stack members --------- Co-authored-by: akocbek@ie.ibm.com <akocbek@ie.ibm.com> Co-authored-by: Todd Giguere <todd.giguere@ibm.com>
…> - updated architecture diagrams<br> - new naming scheme for stack steps, removed numbering in names<br> - changed order of some steps<br> - updated secrets manager DA to latest * fix: update architecture diagrams * fix: remove extra region param in kms * fix(deps): update to latest version of secret manager * refactor: rename and reorder stack members --------- Co-authored-by: akocbek@ie.ibm.com <akocbek@ie.ibm.com> Co-authored-by: Todd Giguere <todd.giguere@ibm.com>
Description
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