Skip to content

Conversation

@rajatagarwal-ibm
Copy link
Member

@rajatagarwal-ibm rajatagarwal-ibm commented Aug 14, 2025

Description

fixes https://github.ibm.com/GoldenEye/issues/issues/15042

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (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:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should wait until App config has KMS and Event Notification support (Mukul is working on adding this now). After that, the dependency tree would look like this:

1a - Key management
1b - Object storage
1c - Cloud Monitoring
2 - Event Notifications
3a - Cloud Logs for logging
3b - Cloud Logs for activity tracking
3c - App Config
3d - Secrets Manager
4a - Security and Compliance Center Workload Protection
4b - Activity Tracker Event Routing

Renaming stack members is currently a breaking change, however since this whole update is a breaking change anyway, we can use this opportunity to rename. We don't have the ability to order stack members either, so using numbers is the only way currently (I'l follow up with projects to see if we can get this ability any time soon).

I'm also wondering whether it makes sense to actually rename the variation programmatic name to prevent users from being able to upgrade, but that will all depend on what upgrade experience is - lets jump on a call to go through that before we decide.

@rajatagarwal-ibm
Copy link
Member Author

Update the diagram
- App config arrow to EN and KMS
- 2 more buckets: AT Cloud logs bucket, AT Cloud logs Metrics Bucket

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rajat can you please do a full review of all the inputs you are using for the DAs. Some of the inputs are not even valid inputs for the new variations.
Also some of the inputs can be removed, or updated. Things like secrets_manager_instance_name where we hard coded to "base-security-services-sm" to prevent a breaking change. We can reset all these now as this is a major version release with no upgrade path anyway

copyright:
years: 2024
lastupdated: "2024-12-05"
years: 2025
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are supposed to keep 2024

Suggested change
years: 2025
years: 2024, 2025

"value": "ref:../../inputs/existing_resource_group_name"
},
{
"name": "use_existing_resource_group",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a valid input for the fully configurable variation?? Neither is resource_group_name - are you using the correct variation? The input is called existing_resource_group_name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, you have this same issue in KMS, COS and Secrets Manager

@ocofaigh
Copy link
Contributor

ocofaigh commented Sep 6, 2025

Lets review the output names too. For example en_crn -> event_notifications_crn

@rajatagarwal-ibm
Copy link
Member Author

Thanks @ocofaigh for the review, I will go thru all the inputs and outpus for all the variations and mapping too.

@rajatagarwal-ibm
Copy link
Member Author

@ocofaigh Reviewed all the mappings, input and output for all the members and updated the PR

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new lines you added in the diagram are not straight?

image image

You also have the AT buckets wrong...

  • AT has a route that sends directly to a COS bucket
  • AT also has a route that sends to Cloud Logs. And that Cloud Logs instance will have 2 buckets associated with it. And its a different instance to the Cloud Logs instance for logging, so please add a second Cloud Logs icon to the diagram and fix the buckets up

@rajatagarwal-ibm
Copy link
Member Author

@ocofaigh, I have updated the diagram and fixed crooked lines and added a bucket for AT. However, I don't fully understand the second cloud logs bucket requirement. Perhaps we can have a quick discussion on Monday AM.

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final few comments

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Contributor

It says (1a - Key management) Validation result: failed however the workspace passed:
image

Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the display_name for secrets_manager_service_plan and update the key value to this. You will need to update the ref.

ocofaigh
ocofaigh previously approved these changes Sep 26, 2025
@ocofaigh
Copy link
Contributor

/run pipeline

@ocofaigh ocofaigh merged commit 5e56d7b into main Sep 26, 2025
@ocofaigh ocofaigh deleted the full-configurable branch September 26, 2025 13:15
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

1 similar comment
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants