Skip to content

Conversation

@jor2
Copy link
Member

@jor2 jor2 commented Apr 30, 2025

Description

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.

@jor2
Copy link
Member Author

jor2 commented Apr 30, 2025

Screenshot 2025-04-30 at 12 49 24 Screenshot 2025-04-30 at 12 49 56

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.

As discussed on slack, maybe we go with this:

  • App Config DA:
    • support creating AppConfig instance
    • supports enabling config aggregator + the required trusted profile (disabled by default)
  • Workload Protection DA
    • supports creating WP instance
    • defines the App Config DA as a dependant DA, and uses input mappings in the catalog json to enable the config aggregator
    • supports enabling CSPM by taking in existing AppConfig CRN and creating WP trusted profile

@jor2 jor2 marked this pull request as ready for review May 2, 2025 15:50
@jor2 jor2 requested review from akocbek and shemau as code owners May 2, 2025 15:50
@jor2 jor2 self-assigned this May 7, 2025
@jor2
Copy link
Member Author

jor2 commented May 8, 2025

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.

Left some comments - I think we should move the WP trusted profile logic into the root module

@jor2
Copy link
Member Author

jor2 commented May 22, 2025

/run pipeline

@jor2
Copy link
Member Author

jor2 commented May 22, 2025

/run pipeline

main.tf Outdated
# but profile can only exist after instance has been created
# hence we cannot directly enable CSPM in the instance creation
# and need to use a separate resource to enable it
resource "restapi_object" "enable_cspm" {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe change the name of the resource since it will also support disabling cspm

Suggested change
resource "restapi_object" "enable_cspm" {
resource "restapi_object" "cspm" {

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.

in order to unblock this PR, can you temporarily add the resource to the IgnoreAdds in test options and create a GIT issue to debug?
I suggest we add it to the module and DA documentation as a known issue

@jor2
Copy link
Member Author

jor2 commented May 23, 2025

/run pipeline

tests/pr_test.go Outdated
DeleteWorkspaceOnFail: false,
WaitJobCompleteMinutes: 60,
IgnoreUpdates: testhelper.Exemptions{
List: []string{"module.scc_wp.restapi_object.enable_cspm"},
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be needed in all tests that enable CSPM. Also please link to a git issue tracking the problem. And add a note in both module and DA readmes

@jor2
Copy link
Member Author

jor2 commented May 23, 2025

/run pipeline

tests/pr_test.go Outdated
DeleteWorkspaceOnFail: false,
WaitJobCompleteMinutes: 60,
// workaround for https://github.com/terraform-ibm-modules/terraform-ibm-scc-workload-protection/issues/243
IgnoreUpdates: testhelper.Exemptions{
Copy link
Contributor

Choose a reason for hiding this comment

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

Its not an update, its a create, so need to use IgnoreAdds

@jor2
Copy link
Member Author

jor2 commented May 23, 2025

/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.

Module and DA readme note still missing

@jor2
Copy link
Member Author

jor2 commented May 23, 2025

/run pipeline

@ocofaigh
Copy link
Contributor

While testing the DA through the catalog tile (and so through schematics), I'm getting the error:
Error: Patch "https://private.resource-controller.cloud.ibm.com/v2/resource_instances/9699e693-7fb2-4fcc-94ed-4893065219f0": dial tcp: lookup private.resource-controller.cloud.ibm.com on 172.21.0.10:53: no such host

This is off as schematics runtime should have access to this private endpoint. I have reached out to the schematics team. For now, I'm going to default it back to public endpoint

@ocofaigh
Copy link
Contributor

/run pipeline

@ocofaigh ocofaigh merged commit 778b248 into main May 23, 2025
2 checks passed
@ocofaigh ocofaigh deleted the app-config branch May 23, 2025 21:49
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.6.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.

6 participants