refactor: update dhcp on dns server change#447
Conversation
|
Check where you would like a Mattermost message to be sent to when CI completes and this PR is merged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[maas-code-reviewer review]
LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.
The change to trigger a DHCP workflow when dns_servers are updated is a great addition and the new tests look good. However, there is a critical bug in update_dhcp introduced by expanding the watched fields without updating the signal handler's unpacking logic, which will result in runtime crashes.
| update_dhcp, | ||
| Subnet, | ||
| ["vlan_id"], | ||
| ["vlan_id", "dns_servers"], |
There was a problem hiding this comment.
By adding "dns_servers" to the watched fields, old_values passed to update_dhcp will now contain two elements: (old_vlan_id, old_dns_servers). However, update_dhcp (around line 110, just above this hunk) unpacks it as [old_vlan_id] = old_values, which expects a single element. This will raise a ValueError: too many values to unpack (expected 1) and crash whenever a Subnet is saved.
You must update update_dhcp to handle the additional field. For example:
old_vlan_id, old_dns_servers = old_valuesPerformance tip: Since old_vlan_id == instance.vlan_id when only dns_servers change, you could also optimise update_dhcp to skip the VLAN.objects.get(id=old_vlan_id) query if old_vlan_id == instance.vlan_id.
2 participants
No description provided.