Page MenuHomePhabricator

Introduce strict/versioned config parsing
ClosedPublic

Authored by dduvall on Apr 4 2018, 10:35 PM.

Details

Maniphest Tasks
T191460: Blubber should error on unknown/obsolete config fields
Reviewers
thcipriani
demon
hashar
mmodell
mobrovac
Group Reviewers
Release-Engineering-Team
Commits
rGBLBR26b998456a56: Introduce strict/versioned config parsing
Patch without arc
git checkout -b D1021 && curl -L https://phabricator.wikimedia.org/D1021?download=true | git apply
Summary

Introduced a version config field that must be specified and match
config.CurrentVersion.

Changed config.ReadConfig to use yaml.UnmarshalStrict to ensure that
errors are surfaced when unknown/bad fields are present in the given
YAML config. A smaller config.VersionConfig is now unmarshaled first
to prevalidate the new version field before the entire config is
parsed.

Fixes T191460

Test Plan

Run go test ./.... Run blubber against some configuration containing
invalid fields and ensure that it surfaces a YAML error.

Diff Detail

Repository
rGBLBR Blubber
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dduvall created this revision.Apr 4 2018, 10:35 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptApr 4 2018, 10:35 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
dduvall requested review of this revision.Apr 4 2018, 10:35 PM
thcipriani accepted this revision.Apr 4 2018, 10:44 PM

Looks good. You should update the blubber.example.yaml to include version: v1 as well :)

config/reader_test.go
33

sensible-chuckle.gif

This revision is now accepted and ready to land.Apr 4 2018, 10:44 PM
dduvall updated this revision to Diff 2684.Apr 5 2018, 8:10 PM
dduvall edited the summary of this revision. (Show Details)

Reimplemented version field using a separate struct so that validation of the field may happen independent of the entire config and before strict unmarshaling occurs.

thcipriani accepted this revision.Apr 5 2018, 8:23 PM

Looks good, works for me. Double unmarshall makes the most sense for the version field as we talked about in IRC.

This revision was automatically updated to reflect the committed changes.