Skip to content
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

[Do Not Merge] add : fully configurable version #300

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Aayush-Abhyarthi
Copy link
Contributor

Description

https://github.ibm.com/GoldenEye/issues/issues/12575

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.

@Aayush-Abhyarthi Aayush-Abhyarthi changed the title add : fully configurable version [WIP] add : fully configurable version Mar 5, 2025
@Aayush-Abhyarthi Aayush-Abhyarthi changed the title [WIP] add : fully configurable version [Do Not Merge] add : fully configurable version Mar 5, 2025
Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use spaces in directory names. The renamed directory should be solutions/security-enforced. The label name should be Security-enforced and the programmtic variation name should be security-enforced. I think you need to check your changes against the recent updates that were made to the standard variation. Several variables were renamed, and it seems you do not have the new names.
I think the direction we are going is the the whole code in the standard (security-enforced) directory will go away, and that new Security-enforced variation will actually just call the Fully configurable variation in the ibm_catalog.json but will hard code a few variables, and force require others to lock it down. Otherwise we end up with alot of code duplication for now reason. Such a change will be a full breaking major version release. see below comment

@@ -6,7 +6,7 @@ offerings:
catalog_id: 7df1e4ca-d54c-4fd0-82ce-3d13247308cd
offering_id: 6d6ebc76-7bbd-42f5-8bc7-78f4fabd5944
variations:
- name: standard
- name: Security-enforced
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Security-enforced
- name: security-enforced

@@ -1,7 +1,7 @@
# More info about this file at https://github.com/terraform-ibm-modules/common-pipeline-assets/blob/main/.github/workflows/terraform-test-pipeline.md#cra-config-yaml
version: "v1"
CRA_TARGETS:
- CRA_TARGET: "solutions/standard" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
- CRA_TARGET: "solutions/Security-enforced" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- CRA_TARGET: "solutions/Security-enforced" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.
- CRA_TARGET: "solutions/security-enforced" # Target directory for CRA scan. If not provided, the CRA Scan will not be run.

@@ -21,7 +21,7 @@
],
"short_description": "Creates and configures a Secrets Manager instance.",
"long_description": "This solution is used to provision and configure an IBM Cloud Secrets Manager instance.",
"offering_docs_url": "https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/main/solutions/standard/README.md",
"offering_docs_url": "https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/main/solutions/Security-enforced/README.md",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"offering_docs_url": "https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/main/solutions/Security-enforced/README.md",
"offering_docs_url": "https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/main/solutions/security-enforced/README.md",

"label": "Standard",
"name": "standard",
"label": "Security-enforced",
"name": "Security-enforced",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"name": "Security-enforced",
"name": "security-enforced",

"install_type": "fullstack",
"working_directory": "solutions/standard",
"working_directory": "solutions/Security-enforced",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also change the directory name

Suggested change
"working_directory": "solutions/Security-enforced",
"working_directory": "solutions/security-enforced",

variable "prefix" {
type = string
description = "The prefix to apply to all resources created by this solution."
default = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the default value


variable "prefix" {
type = string
description = "The prefix to apply to all resources created by this solution."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the description to match what is in https://github.ibm.com/GoldenEye/issues/issues/12841

variable "secrets_manager_instance_name" {
type = string
description = "The name to give the Secrets Manager instance provisioned by this solution. If a prefix input variable is specified, it is added to the value in the `<prefix>-value` format."
default = "base-security-services-sm"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default = "base-security-services-sm"
default = "secrets-manager"

default = false
}

variable "secret_manager_tags" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Access tag support will be coming in #294 so suggest to explicitly say its a resource tag:

Suggested change
variable "secret_manager_tags" {
variable "secret_manager_resource_tags" {

}))
})))
}))
description = "(Optional, list) List of CBR rules to create. [Learn more](https://github.com/terraform-ibm-modules/terraform-ibm-secrets-manager/blob/main/solutions/standard/DA-cbr_rules.md)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointing to old directory

@ocofaigh
Copy link
Member

ocofaigh commented Mar 6, 2025

@Aayush-Abhyarthi We talked more about what to do with the current standard (new name Security-enforced) variation. We should take the same approach as the fscloud submodule and have the code be a wrapper around the Fully configurable variation. Since it will a major version change, you can actually go ahead and change the programatic name of the variation in the ibm_catalog,json to security-enforced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants