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

refactor: Update the logic for prefix variable. #269

Merged
merged 8 commits into from
Jan 15, 2025
Merged

Conversation

arya-girish-k
Copy link
Contributor

@arya-girish-k arya-girish-k commented Dec 23, 2024

Description

Refactored the code logic for prefix variable by adding default value for prefix and marked as required in catalog manifest.Also allowed prefix to be "" (empty string) for advanced users.
#11959

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Add the default value for prefix variable and mark as required in catalog manifest. Also allowed prefix to be "" (empty string ).

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.

@arya-girish-k
Copy link
Contributor Author

/run pipeline

Copy link
Member

@imprateeksh imprateeksh left a comment

Choose a reason for hiding this comment

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

@arya-girish-k - I’ve added some comments —please take a look.
Also, have you tried running tests with prefix values set to both null and ""?

If not, I recommend doing so to ensure the logic handles these cases correctly. You can attach the test results for reference.

solutions/account-infrastructure-base/variables.tf Outdated Show resolved Hide resolved
solutions/account-infrastructure-base/variables.tf Outdated Show resolved Hide resolved
solutions/account-infrastructure-base/variables.tf Outdated Show resolved Hide resolved
@arya-girish-k
Copy link
Contributor Author

Modified the logic and description as per the review comments.Retriggering the pipeline .

@arya-girish-k
Copy link
Contributor Author

/run pipeline

@arya-girish-k
Copy link
Contributor Author

@imprateeksh -Tried running TestRunDA with prefix values set to both null and "".Please find the test reference .
If not providing any value or null , the output:

TestRunDA 2025-01-03T13:22:43+05:30 command.go:185: Changes to Outputs:

activity_tracker_route_name       = "accinfbase-cos-route" 
audit_resource_group_name         = "accinfbase-audit-rg" �
cos_bucket_name                   = "accinfbase-cos-bucket" �
cos_instance_name                 = "accinfbase-cos-instance" 
cos_target_name                   = "accinfbase-cos-target" �
devops_resource_group_name        = "accinfbase-devops-tools-rg" 
edge_resource_group_name          = "accinfbase-edge-rg" �
management_resource_group_name    = "accinfbase-management-plane-rg" �
observability_resource_group_name = "accinfbase-obs-resource-group" 
security_resource_group_name      = "accinfbase-security-rg" 
trusted_profile_name              = "accinfbase-trusted-profile" �
workload_resource_group_name      = "accinfbase-workload-rg"

if prefix as "",the output will be:

`TestRunDA 2025-01-03T13:11:37+05:30 logger.go:67: Changes to Outputs:

activity_tracker_route_name       = "-h3sgnu-cos-route"
audit_resource_group_name         = "-h3sgnu-audit-rg"
cos_bucket_name                   = "h3sgnu-cos-bucket"
cos_instance_name                 = "-h3sgnu-cos-instance"
cos_target_name                   = "-h3sgnu-cos-target"
devops_resource_group_name        = "-h3sgnu-devops-tools-rg"
edge_resource_group_name          = "-h3sgnu-edge-rg"
management_resource_group_name    = "-h3sgnu-management-plane-rg"
observability_resource_group_name = "-h3sgnu-obs-resource-group"
security_resource_group_name      = "-h3sgnu-security-rg"
trusted_profile_name              = "-h3sgnu-trusted-profile"
workload_resource_group_name      = "-h3sgnu-workload-rg"`

When an empty string is provided, all the variables using prefix starts with a hyphen. However, for cos_bucket_name the root module is checking the pattern ^[a-z0-9][a-z0-9-]+[a-z0-9]$, so I used trimprefix for that particular variable to address the issue.ie,
cos_bucket_name = !var.provision_atracker_cos ? null : (var.cos_bucket_name == null ? try("${trimprefix(local.prefix, "-")}-cos-bucket", "cos-bucket") : lower(var.cos_bucket_name))

@imprateeksh
Copy link
Member

/run pipeline

@imprateeksh
Copy link
Member

@imprateeksh -Tried running TestRunDA with prefix values set to both null and "".Please find the test reference . If not providing any value or null , the output:

TestRunDA 2025-01-03T13:22:43+05:30 command.go:185: Changes to Outputs:

activity_tracker_route_name       = "accinfbase-cos-route" 
audit_resource_group_name         = "accinfbase-audit-rg" �
cos_bucket_name                   = "accinfbase-cos-bucket" �
cos_instance_name                 = "accinfbase-cos-instance" 
cos_target_name                   = "accinfbase-cos-target" �
devops_resource_group_name        = "accinfbase-devops-tools-rg" 
edge_resource_group_name          = "accinfbase-edge-rg" �
management_resource_group_name    = "accinfbase-management-plane-rg" �
observability_resource_group_name = "accinfbase-obs-resource-group" 
security_resource_group_name      = "accinfbase-security-rg" 
trusted_profile_name              = "accinfbase-trusted-profile" �
workload_resource_group_name      = "accinfbase-workload-rg"

if prefix as "",the output will be:

`TestRunDA 2025-01-03T13:11:37+05:30 logger.go:67: Changes to Outputs:

activity_tracker_route_name       = "-h3sgnu-cos-route"
audit_resource_group_name         = "-h3sgnu-audit-rg"
cos_bucket_name                   = "h3sgnu-cos-bucket"
cos_instance_name                 = "-h3sgnu-cos-instance"
cos_target_name                   = "-h3sgnu-cos-target"
devops_resource_group_name        = "-h3sgnu-devops-tools-rg"
edge_resource_group_name          = "-h3sgnu-edge-rg"
management_resource_group_name    = "-h3sgnu-management-plane-rg"
observability_resource_group_name = "-h3sgnu-obs-resource-group"
security_resource_group_name      = "-h3sgnu-security-rg"
trusted_profile_name              = "-h3sgnu-trusted-profile"
workload_resource_group_name      = "-h3sgnu-workload-rg"`

When an empty string is provided, all the variables using prefix starts with a hyphen. However, for cos_bucket_name the root module is checking the pattern ^[a-z0-9][a-z0-9-]+[a-z0-9]$, so I used trimprefix for that particular variable to address the issue.ie, cos_bucket_name = !var.provision_atracker_cos ? null : (var.cos_bucket_name == null ? try("${trimprefix(local.prefix, "-")}-cos-bucket", "cos-bucket") : lower(var.cos_bucket_name))

I think this needs to be checked again. When local.prefix is an empty string (i.e. local.prefix = "").
Here, trimprefix("", "-") will return an empty string (""). So, this will result in "-cos-bucket"

And this might fail the cos_bucket_name as per the pattern in the root module.

image

@arya-girish-k
Copy link
Contributor Author

/run pipeline

@arya-girish-k
Copy link
Contributor Author

arya-girish-k commented Jan 8, 2025

Modified the local.prefix variable to handle the null and empty values .Reverted the changes for the variable cos_bucket_name.

@arya-girish-k
Copy link
Contributor Author

arya-girish-k commented Jan 8, 2025

Please find the test references for TestRunDA .
https://github.ibm.com/GoldenEye/issues/issues/11959#issuecomment-101396669

@imprateeksh
Copy link
Member

/run pipeline

Copy link
Member

@imprateeksh imprateeksh left a comment

Choose a reason for hiding this comment

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

Looks good. Approved.

@maheshwarishikha maheshwarishikha merged commit cb69398 into main Jan 15, 2025
2 checks passed
@maheshwarishikha maheshwarishikha deleted the 11959-prefix branch January 15, 2025 13:55
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants