Repository navigation
fix(planstate): leave the plan unchanged when an appended layer fails validation - #937
Conversation
… validation appendLayer inserted with slices.Insert on m.plan.Layers itself. When the slice has spare capacity the insert shifts elements in place, so a layer that then failed plan validation was already spliced into the live layer list and stayed there. Insert into a clone; the success path is unchanged.
|
@chapman-von-monty review this PR. |
|
Understood. Reviewing PR #937 against Pebble's standards and test suites. |
|
I reviewed the change and found no issues. Cloning the layer slice before The regression test is meaningful: it creates a non-terminal sub-directory insertion with spare-capacity conditions, verifies both layer labels and combined services after rejection, then verifies a later append does not resurrect or lose state. It follows the package gocheck conventions and uses the dot import. I ran:
All passed. This is good to merge from my review. |
|
@hpidcock I checked the latest PR state and head. PR #937 is now merged (head I reran against the latest head:
No further issues found; the merged change is good. |
A
pebble add --innerwhose layer parses but fails plan validation leaves the daemon's live layer list changed.appendLayerinserted withslices.Insert(m.plan.Layers, ...). When the slice has spare capacity,slices.Insertshifts the existing elements in place, so the new layer was spliced intom.plan.LayersbeforeupdatePlanLayersvalidated the result; when validation failed, that mutation stayed.CombineLayeralready copies before mutating.Insert into
slices.Clone(m.plan.Layers)instead. The success path is unchanged: same index, sameOrder, same returned plan.The regression test appends five layers with the sub-directory layer first, so the insert index is not the end and the slice has spare capacity, then appends a layer that fails validation. On master it fails with the layer list read as
[foo/one foo/two bbb ccc ddd].go test -race ./...,go build ./cmd/pebble,gofmt,staticcheckpass.