feat: support existing scc instance

[Soaib024]

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?

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

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

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

[ocofaigh]

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

[Soaib024]

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.

[in-1911]

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

[Soaib024]

@ocofaigh @in-1911 I have removed the logic to set scc_instance_settings when using an existing scc instance for now, we could add it later once we have fix in provider

[ocofaigh]

some comments

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

@ocofaigh I have added a new variable update_scc_settings. I think this is still not a perfect solution because, for example, if I have an existing SCC instance with EN and COS configured, and I want to change the EN only, I will need to pass the COS CRN and bucket as well. If not, then the COS attachment will be removed if the COS CRN and bucket are null and vice-versa. Apart from this edge case, I think the solution works fine.

I have also made var.cos_instance_crn and var.cos_bucket optional

[Soaib024]

/run pipeline

[Soaib024]

@ocofaigh I have added a new variable update_scc_settings. I think this is still not a perfect solution because, for example, if I have an existing SCC instance with EN and COS configured, and I want to change the EN only, I will need to pass the COS CRN and bucket as well. If not, then the COS attachment will be removed if the COS CRN and bucket are null and vice-versa. Apart from this edge case, I think the solution works fine.

I have also made var.cos_instance_crn and var.cos_bucket option

@ocofaigh do you have any comments on this one?

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

/run pipeline

[Soaib024]

@ocofaigh Could you please review this PR,

The logic to update scc_instance_settings has been removed when existing scc instance is used, as a follow-up fix is needed in the provider to:

• Expose the name parameter for EN settings
• Also may be to allow updating COS and EN settings individually if possible

[ocofaigh]

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

[Soaib024]

/run pipeline

[Soaib024]

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

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

[Soaib024]

/run pipeline

[Soaib024]

@ocofaigh I have moved the completeExampleTest to other_test.go

[Soaib024]

/run pipeline

[ocofaigh]

1 minor comment

[Soaib024]

/run pipeline

[terraform-ibm-modules-ops]

🎉 This issue has been resolved in version 1.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀