canonical / pebble

Pebble is a lightweight Linux service manager with layered configuration and an HTTP API.
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
146 stars 54 forks source link

When combining layers, Pebble should validate the whole plan rather than a single combined layer #349

Closed benhoyt closed 7 months ago

benhoyt commented 8 months ago

When doing pebble add --combine, Pebble is calling plan.CombineLayers on the existing labelled layer plus the new one, and validating the result as if it's in the entire plan (rather than just a single new layer), so we get this error:

$ pebble plan
services:
    django:
        override: replace
        command: sleep 1000

$ cat merge.yaml 
services:
  django:
    override: merge
    environment:
      POSTGRES: pg

$ pebble add mergie merge.yaml 
Layer "mergie" added successfully from "merge.yaml"

$ cat merge2.yaml 
services:
  django:
    override: merge
    environment:
      MYSQL: mysql

$ pebble add mergie merge2.yaml --combine
error: plan must define "command" for service "django"

The above should work, because we should only validate on the overall combined plan. The expected result is:

$ PEBBLE=~/pebble go run ./cmd/pebble plan
services:
    django:
        override: replace
        command: sleep 1000
        environment:
            MYSQL: mysql
            POSTGRES: pg
benhoyt commented 8 months ago

Probably the best way to fix this is to extract the validation of the combined fields from plan.CombineLayers (.Services, .Checks, .LogTargets, and checkCycles) and then add a separate function plan.Validate(p *Plan) which does the validation on a combined plan. We'd only call plan.Validate in the two places validation is actually desired: plan.ReadDir and ServiceManager.updatePlanLayers.