-
Notifications
You must be signed in to change notification settings - Fork 0
Add SCC DA (Fully configurable + Security-enforced variations) #249
Conversation
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
| // Verify ibmcloud_api_key variable is set | ||
| checkVariable := "TF_VAR_ibmcloud_api_key" | ||
| val, present := os.LookupEnv(checkVariable) | ||
| require.True(t, present, checkVariable+" environment variable not set") | ||
| require.NotEqual(t, "", val, checkVariable+" environment variable is empty") |
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.
Seems like this code is not being used, so can be removed.
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.
@maheshwarishikha are you talking about line 77? This checks if the TF_VAR_ibmcloud_api_key env var value is set to an empty string, and fails if that is the case
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 was talking about line 73-77. Why do we need to check this?
I observed that we use this code if we use region part in tests:
// Verify ibmcloud_api_key variable is set
checkVariable := "TF_VAR_ibmcloud_api_key"
val, present := os.LookupEnv(checkVariable)
require.True(t, present, checkVariable+" environment variable not set")
require.NotEqual(t, "", val, checkVariable+" environment variable is empty")
// Programmatically determine region to use based on availability
region, _ := testhelper.GetBestVpcRegion(val, "../common-dev-assets/common-go-assets/cloudinfo-region-vpc-gen2-prefs.yaml", "eu-de")
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.
Line 73 - 77 is checking that the TF_VAR_ibmcloud_api_key env var is set as that is needed for the terraform apply to work
It has nothing to do with region selector
|
/run pipeline |
|
/run pipeline |
| }, | ||
| "configuration": [ | ||
| { | ||
| "key": "ibmcloud_api_key" |
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.
should this be 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.
Since the terraform code itself has no default value, you don't need to explicitly mark it as required here
| "key": "scc_region", | ||
| "required": true, | ||
| "options": [ | ||
| { |
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.
missing au-syd.
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.
see #249 (comment)
solutions/fully-configurable/main.tf
Outdated
| kms_service_name = var.existing_kms_instance_crn != null ? module.existing_kms_crn_parser[0].service_name : var.existing_kms_key_crn != null ? module.existing_kms_key_crn_parser[0].service_name : null | ||
| kms_account_id = var.existing_kms_instance_crn != null ? module.existing_kms_crn_parser[0].account_id : var.existing_kms_key_crn != null ? module.existing_kms_key_crn_parser[0].account_id : null | ||
| kms_key_id = var.existing_kms_instance_crn != null ? module.kms[0].keys[format("%s.%s", local.scc_cos_key_ring_name, local.scc_cos_key_name)].key_id : var.existing_kms_key_crn != null ? module.existing_kms_key_crn_parser[0].resource : null | ||
| scc_cos_key_ring_name = try("${local.prefix}-${var.scc_cos_key_ring_name}", var.scc_cos_key_ring_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.
are we still trying to reduce number of try statements? terraform-ibm-modules/terraform-ibm-scc-workload-protection#181 (comment)
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.
@jor2 good spot - I had just copied the code from SCC DA. This is refactored now
|
Left some small comments, code looks good couldn't find anything else. |
|
/run pipeline |
|
🎉 This PR is included in version 2.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Migrating the DA from https://github.com/terraform-ibm-modules/terraform-ibm-scc-da into this repo since it no longer creates Workload Protection instance (that is coming in terraform-ibm-modules/terraform-ibm-scc-workload-protection#181).
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