-
Notifications
You must be signed in to change notification settings - Fork 13
fix avset mixed vm pip no pip upgrade #160
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the handling of mixed VM scenarios in the Azure Availability Set Basic Public IP Upgrade module. The fix addresses cases where some VMs in an availability set have public IPs and others don't, ensuring the upgrade process continues for VMs with public IPs instead of terminating early.
- Changes logic from early return to skipping individual VMs without public IPs and continuing the upgrade process
- Updates test environment to include a mixed scenario with one VM having a public IP and another without
- Increments module version and updates release notes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| 006-vms-mixed-pip.bicep | Adds new test scenario with mixed VM configurations - one with public IP, one without |
| AzureAvSetBasicPublicIPUpgrade.psm1 | Fixes logic to continue processing other VMs instead of stopping when encountering VMs without public IPs |
| AzureAvSetBasicPublicIPUpgrade.psd1 | Updates module version to 1.0.1 and release notes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Add-LogEntry "VM '$($VM.vmObject.Name)' does not have any public IP addresses attached. Skipping upgrade." -severity WARNING | ||
| return | ||
| Add-LogEntry "VM '$($VM.vmObject.Name)' does not have any public IP addresses attached. Skipping upgrading this VM." -severity INFO | ||
| $VMs = $VMs | Where-Object { $_.vmObject.Id -ne $VM.vmObject.Id } |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifying the collection being iterated over during a foreach loop can lead to unexpected behavior. The VM should be filtered out before the loop or the loop should be restructured to avoid modifying the collection during iteration.
| $VMs = $VMs | Where-Object { $_.vmObject.Id -ne $VM.vmObject.Id } |
| imageReference: { | ||
| offer: 'UbuntuServer' | ||
| publisher: 'Canonical' | ||
| sku: '18.04-LTS' |
Copilot
AI
Aug 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Ubuntu 18.04 LTS reached end of standard support in May 2023. Consider using a more recent Ubuntu LTS version like '20.04-LTS' or '22.04-LTS' for better security and support.
| sku: '18.04-LTS' | |
| sku: '22_04-lts-gen2' |
No description provided.