Pre commit fix pr 199 #209

nupurg-ibm · 2025-03-13T10:54:42Z

Description

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.

bksagar · 2025-03-14T08:42:43Z

.secrets.baseline

    "lines": null
  },
-  "generated_at": "2024-04-08T15:16:34Z",
+  "generated_at": "2025-03-12T08:49:38Z",


Is this the latest one ? Or you need to use what we have on our HPCaaS repo ? @manirtm

bksagar · 2025-03-14T08:43:53Z

examples/create_vpc/modules/landing_zone_vpc/datasource.tf

@@ -1,3 +1,3 @@
-data "ibm_resource_group" "itself" {
-  name = var.resource_group
+data "ibm_resource_group" "resource_group" {


@manirtm May be here also can we use it as existing_resource_group as a resource name ?

bksagar · 2025-03-14T08:44:25Z

examples/create_vpc/modules/landing_zone_vpc/locals.tf

    # var.network_cidr
  ])
-  resource_group_id = var.resource_group != null ? data.ibm_resource_group.itself.id : ""
+  resource_group_id = var.existing_resource_group != null ? data.ibm_resource_group.resource_group.id : ""


@manirtm Once we make the changes above, probably here also you can change that to existing_resource_group

bksagar · 2025-03-14T08:46:14Z

locals.tf

-  #vpc_id            = var.vpc == null ? module.landing_zone.vpc_id[0] : data.ibm_is_vpc.itself[0].id
-  #vpc_crn           = var.vpc == null ? module.landing_zone.vpc_crn[0] : data.ibm_is_vpc.itself[0].crn
+  #vpc_id            = var.vpc_name == null ? module.landing_zone.vpc_id[0] : data.ibm_is_vpc.existing_vpc[0].id
+  #vpc_crn           = var.vpc_name == null ? module.landing_zone.vpc_crn[0] : data.ibm_is_vpc.existing_vpc[0].crn


@manirtm If we do not need this content, can we please remove the commented lines ?

bksagar · 2025-03-14T08:47:11Z

locals.tf

-    service_rg  = var.resource_group == null ? module.landing_zone.resource_group_id[0]["service-rg"] : data.ibm_resource_group.resource_group[0].id
-    workload_rg = var.resource_group == null ? module.landing_zone.resource_group_id[0]["workload-rg"] : data.ibm_resource_group.resource_group[0].id
+    # management_rg = var.existing_resource_group == null ? module.landing_zone.resource_group_id[0]["management-rg"] : one(values(one(module.landing_zone.resource_group_id)))
+    service_rg  = var.existing_resource_group == null ? module.landing_zone.resource_group_id[0]["service-rg"] : data.ibm_resource_group.resource_group[0].id


@manirtm even here when we change the resource name to existing_resource_group.. The data source name should be changed

bksagar · 2025-03-14T08:59:58Z

main.tf

  ha_shared_dir         = local.ha_shared_dir
  nfs_install_dir       = local.nfs_install_dir
-  Enable_Monitoring     = local.Enable_Monitoring
+  enable_monitoring     = local.enable_monitoring


@manirtm We are setting enable_monitoring as false on the locals above. What monitoring is this used for ? Is this the observability monitoring ?

bksagar · 2025-03-14T09:01:36Z

modules/deployer/datasource.tf

@@ -1,4 +1,4 @@
-data "ibm_resource_group" "itself" {
+data "ibm_resource_group" "resource_group" {


@manirtm I would prefer to use the same existing_resource_group

bksagar · 2025-03-14T09:05:33Z

modules/deployer/main.tf

-  source           = "./../key"
-  private_key_path = var.enable_bastion ? "${path.root}/../../modules/ansible-roles/compute_id_rsa" : "${path.root}/modules/ansible-roles/compute_id_rsa" #checkov:skip=CKV_SECRET_6
-}
-


@manirtm I see that the version for using the landing_zone_vsi module is 4.2.0.. But we are already on 4.5.0. Please make sure the versions are upgraded now itself. If there are any issues then we will have to debug and make changes again !

The latest is 4.6.0. But we are using 4.5.0 on our existing code. Please see this:
https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone-vsi

Please keep a note on line 62: skip_iam_authorization_policy = var.enable_bastion ? true : false

I dont want the changes now. When we implement the existing bastion concept we need the changes to implemented like below. Which is already there on our LSF DA

skip_iam_authorization_policy = var.bastion_instance_name != null ? false : local.skip_iam_authorization_policy @manirtm

bksagar · 2025-03-14T09:09:46Z

modules/deployer/variables.tf

-  )
-  description = "Total Number of instances to be launched for compute cluster."
-}
+# variable "management_instances" {


I see these are not needed on the deployer node. As these are created through another module. Can we remove this from here. I dont want any commented codes . @manirtm

bksagar · 2025-03-14T09:13:02Z

modules/deployer/locals.tf

-  scale_cloud_infra_repo_url  = "https://github.com/IBM/ibm-spectrum-scale-install-infra"
-  scale_cloud_infra_repo_name = "ibm-spectrum-scale-install-infra"
-  scale_cloud_infra_repo_tag  = "ibmcloud_v2.6.0"
+  # scale_cloud_deployer_path   = "/opt/IBM/ibm-spectrumscale-cloud-deploy"


@manirtm Please see this line no -2: Tags

terraform-ibm-hpc/modules/deployer/locals.tf

Line 4 in 001a14f

name = "hpc"

I see we are hardcoding it as HPC.. Either we need to update this or come up with a appropriate name.. As the deployer node is common for all the solutions. It cannot be hpc.. We need to talk through this.

And where ever we add tags, please ensure for scale they are as scale and for lsf it is lsf.

bksagar · 2025-03-14T09:32:31Z

modules/inventory/main.tf

@@ -1,14 +1,14 @@
-# resource "local_sensitive_file" "itself" {
+# resource "local_sensitive_file" "mount_path_file" {


@manirtm As discussed if this is not used, please remove this commented lines

bksagar · 2025-03-14T09:34:00Z

modules/landing_zone/version.tf

+    #   version = ">= 3.4.3, < 4.0.0"
+    # }
+    # time = {
+    #   source  = "hashicorp/time"


Please check if these versions are needed or not !!

bksagar · 2025-03-14T09:51:11Z

solutions/hpcaas/main.tf

@@ -1,17 +1,14 @@
 module "hpcaas" {
  source                    = "./../.."
-  scheduler                 = "HPCaaS"


@manirtm Why are we removing this scheduler attribute from here ?

bksagar · 2025-03-14T09:53:50Z

solutions/lsf/catalogValidationValues.json.template

  "prefix": $PREFIX,
-  "zones": "[\"ca-tor-1\"]",
-  "resource_group": "geretain-hpc-rg",
+  "zone": "ca-tor-1",


@manirtm We need the zone value to be a list of string like below:
["ca-tor-1"] This is required on our DA pipelines

solutions/lsf/main.tf

bksagar · 2025-03-14T09:56:26Z

solutions/lsf/variables.tf

-  description = "Password for IBM Spectrum LSF Application Center database GUI."
-}
+# variable "app_center_db_password" {
+#   type        = string


This variable is not needed and we are not supporting.. Need to review once and then remove ! @manirtm

bksagar · 2025-03-14T10:05:36Z

solutions/scale/catalogValidationValues.json.template

 {
  "ibmcloud_api_key": $VALIDATION_APIKEY,
  "prefix": $PREFIX,
-  "zones": "[\"ca-tor-1\"]",


@manirtm same as above we need it in this format only

bksagar

Discussed with @manirtm and reviewed the changes and looks good.

This can be merged once we do the deployment directly from LSF folder for end-end and one deployment from scale folder . If both of them are passed we are good to merge this PR

bksagar · 2025-03-17T08:55:01Z

Discussed with @manirtm and understood that the deployment for the LSF with this changes are working as expected. And there are some hicups with doing deployments as we have the implementation of the deployer node. So the scale deployments are failing. Considering the requirement of other PR merging,

This can be considered as merged !

Manikandan-Renu1 added 19 commits February 27, 2025 00:16

pr-199

3c39665

crn-change

4f0e8a2

sensistive-add

0a1d785

subnet-id-change

a3a66bd

subnet-id-fix

fb1bd36

subent-id-fix

243fc43

subnet-issue

412643e

subnet-issue

1654dba

subnet-issue

cc6ac5d

ssh-key-copy

a8b470f

base64-change

2104f79

resource_provisioner and prepare_tf_input module add

d07f70b

module-issue-fix

6a5b998

resource-condition-handle

596fd17

revoke-destroy-change

8c86192

comments-remove

cd5215e

pre-commit-fix

3c1b482

var-name-change-in-prepare-tf-file

3d04951

var-name-change-in-prepare-tf-file

001a14f

nupurg-ibm requested review from bksagar and go-davidng as code owners March 13, 2025 10:54

bksagar reviewed Mar 14, 2025

View reviewed changes

solutions/lsf/main.tf Show resolved Hide resolved

bksagar reviewed Mar 14, 2025

View reviewed changes

removed-unwanted-commented-lines-and-variables

b73d1cd

bksagar approved these changes Mar 14, 2025

View reviewed changes

bksagar merged commit 45df77a into develop Mar 17, 2025

bksagar deleted the pre-commit-fix-pr-199 branch March 17, 2025 08:56

		@@ -1,4 +1,4 @@
		data "ibm_resource_group" "itself" {
		data "ibm_resource_group" "resource_group" {

		@@ -1,14 +1,14 @@
		# resource "local_sensitive_file" "itself" {
		# resource "local_sensitive_file" "mount_path_file" {

Uh oh!

Pre commit fix pr 199 #209

Pre commit fix pr 199 #209

Uh oh!

Conversation

nupurg-ibm commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bksagar Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

bksagar commented Mar 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nupurg-ibm commented Mar 13, 2025 •

edited

Loading

bksagar Mar 14, 2025 •

edited

Loading