Reviewing a Merge Request on a 600-Line Network Script
A team of three network engineers maintains sync_vlans.py, a 600-line single-file script that pushes VLAN definitions to 180 switches. The key function is def push(device, cmds, dry_run=False, log=[]). Operators report that the run summary sometimes lists VLAN 40 as applied to a switch whose running config never changed. Device credentials sit in config.py, which is committed. CI runs flake8 only; there are no tests. A pull request adding a new site is open, and the author wants to merge on Friday afternoon before a two-week holiday. You are the reviewer.
1. Explain the mechanism behind 'VLAN 40 reported as applied but not present on the device', and describe the smallest fix that removes it.
Read the worked answer
The default argument log=[] is evaluated once, when the def statement executes - not per call. Every invocation therefore shares one list object for the life of the process. In a loop over 180 devices, device 12's summary contains device 1's entries, so a device whose push was skipped or errored still displays VLAN 40 as applied. The device is correct; the report is lying, which is worse, because it suppresses the alarm that the failure should have raised.
The minimal fix:
def push(device, cmds, dry_run=False, log=None):
log = [] if log is None else log
Two follow-ups belong in the same review, because fixing the signature alone leaves the bug class alive: the log must be written per device with the device identity in each record, and the pass/fail decision must come from the parsed device response, not from the absence of an exception. A dry_run that shares state with a live run is the same mistake wearing a different hat - check that it is threaded through, not defaulted.
2. Which tests and CI gates would you require before this merge, given the team has none today? Prioritise.
Read the worked answer
Require the minimum that would have caught the observed bug, then grow:
- A unit test on
pushwith a fake transport, calling it twice and asserting the second call's log does not contain the first call's entries. Fast, no device needed, catches mutable-default and cross-run state. - A test that a failed device response produces a failure record, not a silent success - the inverse of the reported symptom.
- A secret scanner in CI (
config.pyin the repo is a live credential leak, independent of the PR) plus credential loading from environment variables. Rotate first; the git history keeps the old secret forever. - A dry-run/diff gate: for a representative sample of devices,
check_mode-style comparison against running config, asserting the play is idempotent on a second pass.
Judgement on sequencing: the secret rotation is not a merge gate on this PR, it is an incident and outranks it. Tests 1 and 2 are hours of work and should block. Anything broader - full device simulation, coverage thresholds - is a trap before the holiday; shipping an untested but well-scoped change is worse than shipping a small tested one.
3. The senior engineer on the PR insists the whole script must be rewritten into classes with a plugin architecture before any further changes. Do you accept?
Read the worked answer
No - not as a precondition for this PR. The rewrite is a plausible destination but a terrible gate: it bundles a small, reviewable change with a large, unreviewable one, lands both the day before a two-week absence, and leaves nobody available to diagnose the fallout. Classic big-bang risk.
What I would accept instead is a strangler path: extract the one function under change into its own module with an explicit interface (push(device, cmds, dry_run, log) -> Result), test it in isolation, and let the main script call it. Each later PR moves another piece behind that seam. The architecture improves incrementally while every step remains revertable.
If the senior engineer can name a concrete requirement the current structure prevents - not 'cleanliness' but something like 'we need to add a second vendor next quarter' - then write that down as the driver and schedule the refactor as its own change with its own tests. Design arguments without a forcing requirement tend to be taste, and taste should not block a Friday merge on a shared team.