VIRTWINKVM-1882: Read VM names from config#1041
Conversation
Signed-off-by: Bhanuja Bhatt <bhbhatt@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic VM naming using a $VMNAMES array across setup scripts and helper engines, replacing hardcoded computer names. Feedback highlights two potential issues: a possible NoMethodError in machine_name_match? if machine_name is nil, and a potential out-of-bounds array access in client.ps1 if $clientNumber exceeds the $VMNAMES array bounds.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def machine_name_match?(machine_name) | ||
| machine_name.casecmp?(@name) | ||
| end |
There was a problem hiding this comment.
If machine_name is nil, calling casecmp? on it will raise a NoMethodError because NilClass does not implement casecmp?. Previously, machine['name'].eql?(@name) would safely return false if machine['name'] was nil. To prevent potential crashes, we should ensure machine_name is not nil before calling casecmp?.
def machine_name_match?(machine_name)
machine_name && machine_name.casecmp?(@name)
end| $clientNumber = [int32]("0x" + $macSegments[3]) | ||
| $clientIp = $clientNumber + 1 | ||
| $clientName = "$CLIENTCOMPUTERNAME$clientNumber" | ||
| $clientName = $VMNAMES[$clientNumber] |
There was a problem hiding this comment.
If $clientNumber is out of bounds of the $VMNAMES array (for example, due to an unexpected MAC address or mismatched configuration), $clientName will be $null. Passing $null to Rename-Computer -NewName will result in an empty string argument error. Adding a check to ensure $clientName is valid before proceeding improves robustness and error reporting.
$clientName = $VMNAMES[$clientNumber]
if (-not $clientName) {
Write-Error "No VM name found in VMNAMES for client number $clientNumber"
}
| when Integer | ||
| value = v | ||
| when Array | ||
| items = v.map { |item| "'#{item.to_s.gsub("'", "''")}'" }.join(', ') |
There was a problem hiding this comment.
Is there any reason why in example " is used ("STUDIO", "CL1", "CL2") while there we wrap strings in '?
3.3.7 :001 > list = ["Name1", "Name2", "Name3"]
=> ["Name1", "Name2", "Name3"]
3.3.7 :002 > list.map { |item| "'#{item.to_s.gsub("'", "''")}'" }.join(', ')
=> "'Name1', 'Name2', 'Name3'"
There was a problem hiding this comment.
sorry by mistake will change this to single case
| def pool | ||
| @studio.list_pools.each do |pool| | ||
| return pool['name'] if pool['machines'].any? { |machine| machine['name'].eql?(@name) } | ||
| return pool['name'] if pool['machines'].any? { |machine| machine_name_match?(machine['name']) } |
There was a problem hiding this comment.
Just replace .eql? with casecmp?, no need for extra function
| @studio.list_pools.flat_map { |pool| pool['machines'] } | ||
| .detect { |machine| machine['name'].eql?(@name) } | ||
| machines = @studio.list_pools.flat_map { |pool| pool['machines'] } | ||
| machines.detect { |machine| machine_name_match?(machine['name']) } |
Uh oh!
There was an error while loading. Please reload this page.