refactor: best input practices for da

[Vipin654]

Description

Review DA inputs to align with best practices.

Release required?

• No release
• Patch release (x.x.X)
• Minor release (x.X.x)
• Major release (X.x.x)

Release notes content

Review DA inputs to align with best practices.
Following input variables were renamed for better consumer experience

• cbr_rules --> scc_instance_cbr_rules
• existing_en_crn --> existing_event_notifications_crn
• en_source_name --> event_notifications_source_name
• en_source_description --> event_notifications_source_description
• scc_en_from_email --> scc_event_notifications_from_email
• scc_en_reply_to_email --> scc_event_notifications_reply_to_email
• scc_en_email_list --> scc_event_notifications_email_list
• skip_cos_kms_auth_policy --> skip_cos_kms_iam_auth_policy
• skip_scc_cos_auth_policy --> skip_scc_cos_iam_auth_policy
• skip_scc_workload_protection_auth_policy --> skip_scc_workload_protection_iam_auth_policy

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.

[Vipin654]

/run pipeline

[ocofaigh]

scc_region should be marked as required (just like region is in secrets manager PR)

Please review the sorting / grouping. All related inputs needs to be together. Some things I noticed:

• SCC instance inputs should be at the top (after provider_visibility) of the optional inputs. They are all over the place now (scc_instance_name, scc_service_plan, existing_scc_instance_crn, scc_instance_tags)
• The event notifications inputs are not grouped together
• ibmcloud_kms_api_key needs to live with other KMS inputs
• existing_activity_tracker_crn and existing_monitoring_crn should exist with the COS inputs
• skip_scc_workload_protection_auth_policy should exist with other workload protection inputs.

[ocofaigh]

just 1 comment

[ocofaigh]

see latest comments

[Vipin654]

/run pipeline

[ocofaigh]

Perhaps change the default value of scc_instance_name to just scc (aligning with what we did in secrets manager PR)

[ocofaigh]

Oh and also change default value of cos_instance_name to scc-cos
and scc_cos_bucket_name to scc-cos-bucket
and scc_workload_protection_instance_name to scc-workload_protection

[ocofaigh]

I think you missed these changes?

• refactor: best input practices for da #250 (review)
• refactor: best input practices for da #250 (comment)

[ocofaigh]

I would priortise putting inputs that are related to new instance creation first (since that is the default). So suggest to place this further down

[ocofaigh]

This is an allowlisted region. I don't think we should expose it

[ocofaigh]

/run pipeline

[ocofaigh]

@Vipin654 You need to update the test with new variable name:

TestRunExistingResourcesInstances 2025-03-04T09:07:25Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; ╷
│ Error: Value for undeclared variable
│ 
│ A variable named "cos_region" was assigned on the command line, but the
│ root module does not declare a variable of that name. To use this value,
│ add a "variable" block to the configuration.
╵}

[Vipin654]

/run pipeline

[Vipin654]

/run pipeline

[Vipin654]

@ocofaigh test has been fixed. Please approve again

[terraform-ibm-modules-ops]

🎉 This PR is included in version 1.27.0 🎉

The release is available on:

• GitHub release
• v1.27.0

Your semantic-release bot 📦🚀