feat: support existing scc instance

[Soaib024]

Description

https://github.com/terraform-ibm-modules/terraform-ibm-scc/issues/138

https://github.com/terraform-ibm-modules/terraform-ibm-scc-da/issues/140

https://github.com/terraform-ibm-modules/terraform-ibm-scc-da/pull/138#pullrequestreview-2153192310 Support existing scc instance

Release required?

• [ ] No release
• [ ] Patch release (x.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

[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

[ocofaigh]

@Soaib024 what if we broke it down into 2 variables such as configure_cos_existing_instance and configure_en_existing_instance ? That way you could dynamically update the settings based on these values?

[Soaib024]

@Soaib024 what if we broke it down into 2 variables such as configure_cos_existing_instance and configure_en_existing_instance ? That way you could dynamically update the settings based on these values?

@ocofaigh

We will still have a same problem because both event_notifications and object_storage block are required here

resource "ibm_scc_instance_settings" "scc_instance_settings" {
  count       = var.existing_scc_instance_crn == null || var.update_scc_settings ? 1 : 0
  depends_on  = [time_sleep.wait_for_scc_cos_authorization_policy]
  instance_id = local.scc_instance_guid

  event_notifications {
    instance_crn = var.en_instance_crn
  }
  object_storage {
    instance_crn = var.cos_instance_crn
    bucket       = var.cos_bucket
  }
}

So, when we try to set one of these we would be forced to update other as well.

[Soaib024]

Some possible solutions:

• No option to set the en and cos settings when an existing instance is used, so the user will have to manage these settings from where the instance was created.
• Either set both en and cos or none.
• Maybe raising a provider issue to make the en and cos block optional.

[ocofaigh]

yea but you can do this:

event_notifications {
    instance_crn = null
  }

[Soaib024]

yea but you can do this:

event_notifications {
    instance_crn = null
  }

This will remove EN attachment if any, and this is the edge case that I mentioned here

[ocofaigh]

I was suggesting something like this:

event_notifications {
    instance_crn = var.existing_scc_instance_crn != null && var.configure_en_existing_instance ? var.en_instance_crn : null
  }

[Soaib024]

I have made changes that look similar to your suggestion, but it still doesn't seem possible to update one of the attachments. Whenever the ibm_scc_instance_settings resource block runs, it will update both attachments. If the parameter for an attachment is null, the attachment will be removed; if it is not null, the attachment will be updated with the latest value. However, I have made the variable description more descriptive to make this clear.

[Soaib024]

/run pipeline

[ocofaigh]

Need to think about this.. leave it with me overnight ..

[ocofaigh]

@Soaib024 I think lets park this effort for now and maybe add to next deep dive agenda to see if this is something we want to support. For now, lets focus on adding existing SCC support to the DA itself - aka what you were doing in https://github.com/terraform-ibm-modules/terraform-ibm-scc-da/pull/138

[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

[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

[Soaib024]

/run pipeline

[terraform-ibm-modules-ops]

:tada: This issue has been resolved in version 1.8.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: