fix(terraform): normalize numeric inputs and prevent phantom memory flags - #450
Conversation
| var.ora_release != "" ? "--ora-release ${var.ora_release}" : "", | ||
| var.ora_edition != "" ? "--ora-edition ${var.ora_edition}" : "", | ||
| var.ora_listener_port != "" ? "--ora-listener-port ${var.ora_listener_port}" : "", | ||
| try(var.ora_listener_port > 0, false) ? "--ora-listener-port ${var.ora_listener_port}" : "", |
There was a problem hiding this comment.
should we add tonumber() here as well?
| var.skip_database_config ? "--skip-database-config" : "", | ||
| var.ora_pga_target_mb != "" ? "--ora-pga-target-mb ${var.ora_pga_target_mb}" : "", | ||
| var.ora_sga_target_mb != "" ? "--ora-sga-target-mb ${var.ora_sga_target_mb}" : "", | ||
| try(var.ora_pga_target_mb > 0, false) ? "--ora-pga-target-mb ${var.ora_pga_target_mb}" : "", |
There was a problem hiding this comment.
I actually added it to all the numerical checks.
4db8719 to
da082cb
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexBasinov, mfielding The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test oracle-toolkit-install-26ai-data-guard-on-gcp |
1 similar comment
|
/test oracle-toolkit-install-26ai-data-guard-on-gcp |
…lags Fix string comparison antipattern (var.X != "") on numeric Terraform variables. When ora_pga_target_mb or ora_sga_target_mb was unset (default 0), HCL string coercion evaluated "0" != "" to true, always passing --ora-pga-target-mb 0 and --ora-sga-target-mb 0 to install-oracle.sh. Furthermore, passing null to numeric variables caused string interpolation runtime errors. This change: 1. Updates local.common_flags to use try(tonumber(var.X) > 0, false) for all numeric inputs. 2. Adds null-safe validations for ora_pga_target_mb, ora_sga_target_mb, ora_listener_port, and tls_listener_port. 3. Allows ora_redo_log_count to accept numbers, strings, or null while preserving backward compatibility with existing tfvars templates. 4. Uses tostring() for firewall port mappings to ensure strict schema adherence.
da082cb to
226c506
Compare
|
New changes are detected. LGTM label has been removed. |
|
@mfielding: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Summary
Fixes string comparison antipattern (
var.X != "") on numeric Terraform variables interraform/main.tfandterraform/variables.tf.Problem Statement
0 != ""Coercion):In
terraform/variables.tf,ora_pga_target_mbandora_sga_target_mbdefault to0to signal that the toolkit should automatically compute memory (45% of host RAM for SGA, HugePages allocation, and baseline PGA). However,terraform/main.tfchecks:""performs implicit string coercion:"0" != ""evaluates totrue. This causes--ora-pga-target-mb 0and--ora-sga-target-mb 0to always be passed on standard deployments when unset by the caller.null:When callers pass
nullinto module arguments,null != ""evaluates totrue, and string template interpolation ("${var.ora_pga_target_mb}") aborts with:Invalid interpolation: null value cannot be formatted as a string.ora_redo_log_countwas previously typed asstringwith a regex check (^[0-9]+$) to work around the string idiom. Passingnullfails validation becausenull == ""isfalseand regex failscan().ora_listener_portandtls_listener_portvalidation rules (var.port >= 1) crash if a caller passesnull.Solution
try(var.X > 0, false):In
local.common_flags, replacevar.num != ""withtry(var.num > 0, false)forora_pga_target_mb,ora_sga_target_mb,ora_listener_port,tls_listener_port, andora_redo_log_count.Update validation conditions across all numeric inputs in
variables.tfto explicitly permitnull.ora_redo_log_count:Change
ora_redo_log_counttype toanywith validation allowing numbers, strings (e.g."2"in existing tfvars templates), empty string, ornull.Add
tostring()to firewall rule port mapping to ensure strict schema compliance.Test Results & Matrix
A verification test suite was executed across all 7 caller scenarios (defaults, explicit tuning, explicit zero, nulls, custom ports, string numbers, and TLS).
The full test matrix output is published in this GitHub Gist:
https://gist.github.com/mfielding/e7c3a4e9db5bd7594ff6d00309fe3a22