Skip to content

Conversation

@RiadhJouini
Copy link

Description

Fixed Gaps and added the service aggregator sub-module.

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.

  • Can you please add a README.md file in the new submodule folder. As a reference, see: https://github.com/terraform-ibm-modules/terraform-ibm-cloud-monitoring/blob/main/modules/metrics_routing/README.md
  • I think you should rename the submodule: modules/scc_wp_config_aggregator -> modules/config_aggregator (drop the scc_wp_ prefix)
  • I'm not sure I understand the point of the appconfig example you added. It seems to be the same as the complete example. Don't you wan't to add a new example that tests your new config aggregator submodule? Usually I would say just append to the complete example, but I think that will be a problem with our current tests that runs the example as its not run in an enterprise account. So I suggest you create a new example called config-aggregator that provisions an App Config instance, the required trusted profile resource and the config-aggregator.

@ocofaigh
Copy link
Contributor

ocofaigh commented Apr 9, 2025

@ctolon22 @RiadhJouini Should the submodule be more generic? For example you are hard coding things like resource_collection_enabled = true and resource_collection_regions = ["all"]? Perhaps it should just be a generic config_aggregator submodule? Or perhaps it should be just a new feature to the root level module? Thoughts?

@ctolon22
Copy link

ctolon22 commented Apr 9, 2025

@ctolon22 @RiadhJouini Should the submodule be more generic? For example you are hard coding things like resource_collection_enabled = true and resource_collection_regions = ["all"]? Perhaps it should just be a generic config_aggregator submodule? Or perhaps it should be just a new feature to the root level module? Thoughts?

We could do it more general but maybe in a second phase.
The reason we need to have it as submodule is because it requires some trusted profiles so enabling this feature it needs to be done in 2 steps

@RiadhJouini
Copy link
Author

RiadhJouini commented Apr 12, 2025

Done these two: Can you please add a README.md file in the new submodule folder. As a reference, see: https://github.com/terraform-ibm-modules/terraform-ibm-cloud-monitoring/blob/main/modules/metrics_routing/README.md
I think you should rename the submodule: modules/scc_wp_config_aggregator -> modules/config_aggregator (drop the scc_wp_ prefix)

@RiadhJouini
Copy link
Author

Let's keep in mind the dependencies between modules here. We need the app config generated before the trusted profiles as it will be referenced as a trusted relationship. Then we generate the templates and policies before the config aggregator, as the latter needs a the template ID for scoping/assigning templates the child accounts. So the overall order of the full chain deploy is the following: resource group - wp - app config - trusted profiles - trusted profiles template - config aggregator.

@RiadhJouini
Copy link
Author

In answer to this comment: "I'm not sure I understand the point of the appconfig example you added. It seems to be the same as the complete example. Don't you wan't to add a new example that tests your new config aggregator submodule? Usually I would say just append to the complete example, but I think that will be a problem with our current tests that runs the example as its not run in an enterprise account. So I suggest you create a new example called config-aggregator that provisions an App Config instance, the required trusted profile resource and the config-aggregator." --> You're right, the appconfig folder served as a testing folder that was first called to avoid making any changes in the complete folder. As it's the same (and not called anymore), i deleted it. Config aggregator cannot be tested outside an enterprise account (at the moment)... cc @ctolon22

@ocofaigh
Copy link
Contributor

As discussed today, there is no point in having a module which is a straight wrapper around a provider resource. We will just use provider directly in the workload protection end to end example.

@ocofaigh ocofaigh closed this Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants