-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: support existing scc instance #121
Conversation
/run pipeline |
/run pipeline |
I have added data lookup for existing scc instance so that name of instance could be added in output as in we did for crn and region |
main.tf
Outdated
@@ -61,8 +76,9 @@ resource "time_sleep" "wait_for_scc_cos_authorization_policy" { | |||
|
|||
# attach a COS bucket and an event notifications instance | |||
resource "ibm_scc_instance_settings" "scc_instance_settings" { | |||
count = var.existing_scc_instance_crn == null ? 1 : 0 |
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.
this means that a user will be unable to attach an event notifications CRN to an existing SCC instance. I think we will want to support that use case, so you should probably use some dynamic blocks here to allow that to work
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.
Tried with dynamic blocks as follows:
dynamic "event_notifications" {
for_each = var.en_instance_crn != null ? { crn = var.en_instance_crn } : {}
content {
instance_crn = var.en_instance_crn
}
}
But this does not seem to work, as both the event_notifications
and object_storage
blocks are required:
│ Error: Insufficient event_notifications blocks
│
│ on ../../main.tf line 83, in resource "ibm_scc_instance_settings" "scc_instance_settings":
│ 83: resource "ibm_scc_instance_settings" "scc_instance_settings" {
│
│ At least 1 "event_notifications" block is required.
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.
@ocofaigh - there is a problem with using existing Event Notification instance with SCC.
It turns out that when the ibm_scc_instance_settings resource is used, it creates a source in the EN instance with the same name "compliance", so if two SCC instances are provisioned with this module, the second one trying to use EN will fails with the error about "source with the same name already exists.
It probably has to be fixed at the provider level as there is no name parameter in SCC instance settings resource, but when the integration is created in SCC UI, the source name is set to SCC instance name.
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.
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.
some comments
/run pipeline |
/run pipeline |
/run pipeline |
1 similar comment
/run pipeline |
@ocofaigh I have added a new variable I have also made var.cos_instance_crn and var.cos_bucket optional |
/run pipeline |
@ocofaigh do you have any comments on this one? |
/run pipeline |
/run pipeline |
/run pipeline |
@ocofaigh Could you please review this PR, The logic to update |
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 the changes look good, however there is a test gap. I see you added existing_scc_instance_crn
to the complete example, but none of the tests are using it. I suggest to add a test which will first provision an SCC instance (add some tf files to tests/resources like we do in other repos) and have a test provision that first, and then test the complete example passing a value for existing_scc_instance_crn
.
For reference take a look at the existing resources test in the terraform-ibm-scc-da repo
/run pipeline |
I've added a new test to pass an existing SCC instance to the complete example, but I'm wondering if we should remove the complete test since we already have a basic test that creates an SCC instance with COS settings. I guess we won't introduce test gaps removing it or maybe move it to other_test.go |
/run pipeline |
@ocofaigh I have moved the completeExampleTest to other_test.go |
/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.
1 minor comment
/run pipeline |
🎉 This issue has been resolved in version 1.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
#138
terraform-ibm-modules/terraform-ibm-scc-da#140
terraform-ibm-modules/terraform-ibm-scc-da#138 (review)
Support existing scc instance
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