From e10025d688b8e9899a8c7f884f76461e2a4de629 Mon Sep 17 00:00:00 2001 From: Solomon Hykes Date: Sun, 24 Jan 2021 12:54:55 -0800 Subject: [PATCH] Refactor op/script/component loading and spec validation Signed-off-by: Solomon Hykes --- dagger/client.go | 2 +- dagger/compiler.go | 7 +- dagger/component.go | 39 +++++--- dagger/component_test.go | 38 +++++++- dagger/env.go | 9 +- dagger/gen.go | 5 -- dagger/mount.go | 12 ++- dagger/op.go | 29 ++++-- dagger/op_test.go | 4 +- dagger/script.go | 42 +++++++-- dagger/script_test.go | 78 ++++++++++++++-- dagger/spec.cue | 5 -- dagger/spec.go | 10 +++ dagger/types.go | 19 ++++ dagger/value.go | 90 +------------------ dagger/value_test.go | 20 +---- examples/tests/compute/noop/main.cue | 8 -- .../tests/compute/undefined_prop/main.cue | 25 +++--- examples/tests/test.sh | 4 +- 19 files changed, 269 insertions(+), 177 deletions(-) diff --git a/dagger/client.go b/dagger/client.go index 5c13542f..df1e0db2 100644 --- a/dagger/client.go +++ b/dagger/client.go @@ -114,7 +114,7 @@ func (cfg *ClientConfig) Finalize(ctx context.Context) (map[string]string, error return nil, errors.Wrap(err, "invalid client config") } // Finalize boot script - boot, err := v.Get("boot").Script() + boot, err := NewScript(v.Get("boot")) if err != nil { return nil, errors.Wrap(err, "invalid env boot script") } diff --git a/dagger/compiler.go b/dagger/compiler.go index 4aa30856..711ee8b6 100644 --- a/dagger/compiler.go +++ b/dagger/compiler.go @@ -35,7 +35,7 @@ func (cc *Compiler) Spec() *Spec { if err != nil { panic(err) } - cc.spec, err = v.Spec() + cc.spec, err = newSpec(v) if err != nil { panic(err) } @@ -56,12 +56,15 @@ func (cc *Compiler) Compile(name string, src interface{}) (*Value, error) { return cc.Wrap(inst.Value(), inst), nil } +// Compile a cue configuration, and load it as a script. +// If the cue configuration is invalid, or does not match the script spec, +// return an error. func (cc *Compiler) CompileScript(name string, src interface{}) (*Script, error) { v, err := cc.Compile(name, src) if err != nil { return nil, err } - return v.Script() + return NewScript(v) } // Build a cue configuration tree from the files in fs. diff --git a/dagger/component.go b/dagger/component.go index 68030251..335adfa6 100644 --- a/dagger/component.go +++ b/dagger/component.go @@ -3,21 +3,44 @@ package dagger import ( "context" "os" + + "github.com/pkg/errors" ) type Component struct { + // Source value for the component, without spec merged + // eg. `{ string, #dagger: compute: [{do:"fetch-container", ...}]}` v *Value + + // Annotation value for the component , with spec merged. + // -> the contents of #dagger.compute + // eg. `compute: [{do:"fetch-container", ...}]` + // + // The spec is merged at this level because the Cue API + // does not support merging embedded scalar with nested definition. + config *Value +} + +func NewComponent(v *Value) (*Component, error) { + config := v.Get("#dagger") + if !config.Exists() { + return nil, os.ErrNotExist + } + spec := v.cc.Spec() + config, err := spec.Get("#ComponentConfig").Merge(v.Get("#dagger")) + if err != nil { + return nil, errors.Wrap(err, "invalid component config") + } + return &Component{ + v: v, + config: config, + }, nil } func (c *Component) Value() *Value { return c.v } -func (c *Component) Exists() bool { - // Does #dagger exist? - return c.Config().Exists() -} - // Return the contents of the "#dagger" annotation. func (c *Component) Config() *Value { return c.Value().Get("#dagger") @@ -38,11 +61,7 @@ func (c *Component) Validate() error { // Return this component's compute script. func (c *Component) ComputeScript() (*Script, error) { - v := c.Value().Get("#dagger.compute") - if !v.Exists() { - return nil, os.ErrNotExist - } - return v.Script() + return newScript(c.Config().Get("compute")) } // Compute the configuration for this component. diff --git a/dagger/component_test.go b/dagger/component_test.go index 5ed1a6b5..95b9aa8a 100644 --- a/dagger/component_test.go +++ b/dagger/component_test.go @@ -5,6 +5,38 @@ import ( "testing" ) +func TestComponentNotExist(t *testing.T) { + cc := &Compiler{} + root, err := cc.Compile("root.cue", ` +foo: hello: "world" +`) + if err != nil { + t.Fatal(err) + } + _, err = NewComponent(root.Get("bar")) // non-existent key + if err != ErrNotExist { + t.Fatal(err) + } + _, err = NewComponent(root.Get("foo")) // non-existent #dagger + if err != ErrNotExist { + t.Fatal(err) + } +} + +func TestLoadEmptyComponent(t *testing.T) { + cc := &Compiler{} + root, err := cc.Compile("root.cue", ` +foo: #dagger: {} +`) + if err != nil { + t.Fatal(err) + } + _, err = NewComponent(root.Get("foo")) + if err != nil { + t.Fatal(err) + } +} + // Test that default values in spec are applied at the component level // See issue #19 func TestComponentDefaults(t *testing.T) { @@ -28,7 +60,7 @@ func TestComponentDefaults(t *testing.T) { if err != nil { t.Fatal(err) } - c, err := v.Component() + c, err := NewComponent(v) if err != nil { t.Fatal(err) } @@ -53,7 +85,7 @@ func TestValidateEmptyComponent(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = v.Component() + _, err = NewComponent(v) if err != nil { t.Fatal(err) } @@ -65,7 +97,7 @@ func TestValidateSimpleComponent(t *testing.T) { if err != nil { t.Fatal(err) } - c, err := v.Component() + c, err := NewComponent(v) if err != nil { t.Fatal(err) } diff --git a/dagger/env.go b/dagger/env.go index 3e5ee2bb..c0e61f8f 100644 --- a/dagger/env.go +++ b/dagger/env.go @@ -187,8 +187,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { ctx := lg.WithContext(ctx) lg.Debug().Msg("Env.Walk: processing") + // FIXME: get directly from state Value ? Locking issue? val := env.cc.Wrap(v, flowInst) - c, err := val.Component() + c, err := NewComponent(val) if os.IsNotExist(err) { // Not a component: skip return nil, nil @@ -207,3 +208,9 @@ func (env *Env) Walk(ctx context.Context, fn EnvWalkFunc) (*Value, error) { } return out, nil } + +// Return the component at the specified path in the config, eg. `www` +// If the component does not exist, os.ErrNotExist is returned. +func (env *Env) Component(target string) (*Component, error) { + return NewComponent(env.state.Get(target)) +} diff --git a/dagger/gen.go b/dagger/gen.go index d0f70eb4..c6959f2d 100644 --- a/dagger/gen.go +++ b/dagger/gen.go @@ -117,9 +117,4 @@ package dagger src: string | *"/" dest: string | *"/" } - -#TestScript: #Script & [ - {do: "fetch-container", ref: "alpine:latest"}, - {do: "exec", args: ["echo", "hello", "world"]}, -] ` diff --git a/dagger/mount.go b/dagger/mount.go index ecfa20e2..99ded4da 100644 --- a/dagger/mount.go +++ b/dagger/mount.go @@ -13,6 +13,16 @@ type Mount struct { v *Value } +func newMount(v *Value, dest string) (*Mount, error) { + if !v.Exists() { + return nil, ErrNotExist + } + return &Mount{ + v: v, + dest: dest, + }, nil +} + func (mnt *Mount) Validate(defs ...string) error { return mnt.v.Validate(append(defs, "#Mount")...) } @@ -26,7 +36,7 @@ func (mnt *Mount) LLB(ctx context.Context, s Solver) (llb.RunOption, error) { return nil, fmt.Errorf("FIXME: cache mount not yet implemented") } // Compute source component or script, discarding fs writes & output value - from, err := mnt.v.Lookup("from").Executable() + from, err := newExecutable(mnt.v.Lookup("from")) if err != nil { return nil, errors.Wrap(err, "from") } diff --git a/dagger/op.go b/dagger/op.go index c56499da..0f39a2dc 100644 --- a/dagger/op.go +++ b/dagger/op.go @@ -14,6 +14,25 @@ type Op struct { v *Value } +func NewOp(v *Value) (*Op, error) { + spec := v.cc.Spec().Get("#Op") + final, err := spec.Merge(v) + if err != nil { + return nil, errors.Wrap(err, "invalid op") + } + return newOp(final) +} + +// Same as newOp, but without spec merge + validation. +func newOp(v *Value) (*Op, error) { + if !v.Exists() { + return nil, ErrNotExist + } + return &Op{ + v: v, + }, nil +} + func (op *Op) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) { action, err := op.Action() if err != nil { @@ -29,7 +48,7 @@ func (op *Op) Walk(ctx context.Context, fn func(*Op) error) error { isCopy := (op.Validate("#Copy") == nil) isLoad := (op.Validate("#Load") == nil) if isCopy || isLoad { - if from, err := op.Get("from").Executable(); err == nil { + if from, err := newExecutable(op.Get("from")); err == nil { if err := from.Walk(ctx, fn); err != nil { return err } @@ -38,7 +57,7 @@ func (op *Op) Walk(ctx context.Context, fn func(*Op) error) error { } if err := op.Validate("#Exec"); err == nil { return op.Get("mount").RangeStruct(func(k string, v *Value) error { - if from, err := op.Get("from").Executable(); err == nil { + if from, err := newExecutable(op.Get("from")); err == nil { if err := from.Walk(ctx, fn); err != nil { return err } @@ -98,7 +117,7 @@ func (op *Op) Copy(ctx context.Context, fs FS, out *Fillable) (FS, error) { if err != nil { return fs, err } - from, err := op.Get("from").Executable() + from, err := newExecutable(op.Get("from")) if err != nil { return fs, errors.Wrap(err, "from") } @@ -167,7 +186,7 @@ func (op *Op) Exec(ctx context.Context, fs FS, out *Fillable) (FS, error) { // mounts if mounts := op.v.Lookup("mount"); mounts.Exists() { if err := mounts.RangeStruct(func(k string, v *Value) error { - mnt, err := v.Mount(k) + mnt, err := newMount(v, k) if err != nil { return err } @@ -233,7 +252,7 @@ func (op *Op) Export(ctx context.Context, fs FS, out *Fillable) (FS, error) { } func (op *Op) Load(ctx context.Context, fs FS, out *Fillable) (FS, error) { - from, err := op.Get("from").Executable() + from, err := newExecutable(op.Get("from")) if err != nil { return fs, errors.Wrap(err, "load") } diff --git a/dagger/op_test.go b/dagger/op_test.go index 28eab6ca..08b569c6 100644 --- a/dagger/op_test.go +++ b/dagger/op_test.go @@ -14,7 +14,7 @@ func TestLocalMatch(t *testing.T) { if err != nil { t.Fatal(err) } - op, err := v.Op() + op, err := newOp(v) if err != nil { t.Fatal(err) } @@ -40,7 +40,7 @@ func TestCopyMatch(t *testing.T) { if err != nil { t.Fatal(err) } - op, err := v.Op() + op, err := newOp(v) if err != nil { t.Fatal(err) } diff --git a/dagger/script.go b/dagger/script.go index 158df8fa..0491f803 100644 --- a/dagger/script.go +++ b/dagger/script.go @@ -3,6 +3,7 @@ package dagger import ( "context" + "cuelang.org/go/cue" "github.com/pkg/errors" "github.com/rs/zerolog/log" ) @@ -15,15 +16,46 @@ type Script struct { v *Value } -func (s *Script) Validate() error { - // FIXME this crashes when a script is incomplete or empty - return s.Value().Validate("#Script") +func NewScript(v *Value) (*Script, error) { + spec := v.cc.Spec().Get("#Script") + final, err := spec.Merge(v) + if err != nil { + return nil, errors.Wrap(err, "invalid script") + } + return newScript(final) +} + +// Same as newScript, but without spec merge + validation. +func newScript(v *Value) (*Script, error) { + if !v.Exists() { + return nil, ErrNotExist + } + // Assume script is valid. + // Spec validation is already done at component creation. + return &Script{ + v: v, + }, nil } func (s *Script) Value() *Value { return s.v } +// Return the operation at index idx +func (s *Script) Op(idx int) (*Op, error) { + v := s.v.LookupPath(cue.MakePath(cue.Index(idx))) + if !v.Exists() { + return nil, ErrNotExist + } + return newOp(v) +} + +// Return the number of operations in the script +func (s *Script) Len() uint64 { + l, _ := s.v.Len().Uint64() + return l +} + // Run a dagger script func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) { err := s.v.RangeList(func(idx int, v *Value) error { @@ -38,7 +70,7 @@ func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) Msg("script is unspecified, aborting execution") return ErrAbortExecution } - op, err := v.Op() + op, err := newOp(v) if err != nil { return errors.Wrapf(err, "validate op %d/%d", idx+1, s.v.Len()) } @@ -58,7 +90,7 @@ func (s *Script) Execute(ctx context.Context, fs FS, out *Fillable) (FS, error) func (s *Script) Walk(ctx context.Context, fn func(op *Op) error) error { return s.v.RangeList(func(idx int, v *Value) error { - op, err := v.Op() + op, err := newOp(v) if err != nil { return errors.Wrapf(err, "validate op %d/%d", idx+1, s.v.Len()) } diff --git a/dagger/script_test.go b/dagger/script_test.go index a6f3b14a..f6bd40ec 100644 --- a/dagger/script_test.go +++ b/dagger/script_test.go @@ -6,10 +6,47 @@ import ( "testing" ) +// Test a script which loads a nested script +func TestScriptLoadScript(t *testing.T) { + mkScript(t, 2, ` + [ + { + do: "load" + from: [ + { + do: "fetch-container" + ref: "alpine:latest" + } + ] + } + ] + `) +} + +// Test a script which loads a nested component +func TestScriptLoadComponent(t *testing.T) { + mkScript(t, 2, ` +[ + { + do: "load" + from: { + #dagger: compute: [ + { + do: "fetch-container" + ref: "alpine:latest" + } + ] + } + } +] +`) +} + // Test that default values in spec are applied func TestScriptDefaults(t *testing.T) { cc := &Compiler{} v, err := cc.Compile("", ` + [ { do: "exec" args: ["sh", "-c", """ @@ -17,15 +54,17 @@ func TestScriptDefaults(t *testing.T) { """] // dir: "/" } + ] `) if err != nil { t.Fatal(err) } - op, err := v.Op() + script, err := NewScript(v) if err != nil { t.Fatal(err) } - if err := op.Validate(); err != nil { + op, err := script.Op(0) + if err != nil { t.Fatal(err) } dir, err := op.Get("dir").String() @@ -64,7 +103,7 @@ func TestLocalScript(t *testing.T) { if err != nil { t.Fatal(err) } - s, err := v.Script() + s, err := NewScript(v) if err != nil { t.Fatal(err) } @@ -84,12 +123,14 @@ func TestLocalScript(t *testing.T) { func TestWalkBootScript(t *testing.T) { ctx := context.TODO() - cc := &Compiler{} - cfg, err := cc.Compile("clientconfig.cue", baseClientConfig) + cfg := &ClientConfig{} + _, err := cfg.Finalize(context.TODO()) if err != nil { t.Fatal(err) } - script, err := cfg.Get("boot").Script() + + cc := &Compiler{} + script, err := cc.CompileScript("boot.cue", cfg.Boot) if err != nil { t.Fatal(err) } @@ -160,3 +201,28 @@ func TestWalkBiggerScript(t *testing.T) { t.Fatal(got) } } + +// UTILITIES + +// Compile a script and check that it has the correct +// number of operations. +func mkScript(t *testing.T, nOps int, src string) *Script { + cc := &Compiler{} + s, err := cc.CompileScript("test.cue", src) + if err != nil { + t.Fatal(err) + } + // Count ops (including nested `from`) + n := 0 + err = s.Walk(context.TODO(), func(op *Op) error { + n++ + return nil + }) + if err != nil { + t.Fatal(err) + } + if n != nOps { + t.Fatal(n) + } + return s +} diff --git a/dagger/spec.cue b/dagger/spec.cue index 5a458568..fe27a0e8 100644 --- a/dagger/spec.cue +++ b/dagger/spec.cue @@ -112,8 +112,3 @@ package dagger src: string | *"/" dest: string | *"/" } - -#TestScript: #Script & [ - {do: "fetch-container", ref: "alpine:latest"}, - {do: "exec", args: ["echo", "hello", "world"]}, -] diff --git a/dagger/spec.go b/dagger/spec.go index 89f426c3..bac6a835 100644 --- a/dagger/spec.go +++ b/dagger/spec.go @@ -10,6 +10,16 @@ type Spec struct { root *Value } +func newSpec(v *Value) (*Spec, error) { + // Spec contents must be a struct + if _, err := v.Struct(); err != nil { + return nil, err + } + return &Spec{ + root: v, + }, nil +} + // eg. Validate(op, "#Op") func (s Spec) Validate(v *Value, defpath string) error { // Lookup def by name, eg. "#Script" or "#Copy" diff --git a/dagger/types.go b/dagger/types.go index 44b9fdcc..70ecbb68 100644 --- a/dagger/types.go +++ b/dagger/types.go @@ -2,16 +2,35 @@ package dagger import ( "context" + "fmt" + "os" cueflow "cuelang.org/go/tools/flow" ) +var ErrNotExist = os.ErrNotExist + // Implemented by Component, Script, Op type Executable interface { Execute(context.Context, FS, *Fillable) (FS, error) Walk(context.Context, func(*Op) error) error } +func newExecutable(v *Value) (Executable, error) { + // NOTE: here we need full spec validation, + // so we call NewScript, NewComponent, NewOp. + if script, err := NewScript(v); err == nil { + return script, nil + } + if component, err := NewComponent(v); err == nil { + return component, nil + } + if op, err := NewOp(v); err == nil { + return op, nil + } + return nil, fmt.Errorf("value is not executable") +} + // Something which can be filled in-place with a cue value type Fillable struct { t *cueflow.Task diff --git a/dagger/value.go b/dagger/value.go index 64b59a0d..f4d2fa14 100644 --- a/dagger/value.go +++ b/dagger/value.go @@ -2,7 +2,6 @@ package dagger import ( "fmt" - "os" "cuelang.org/go/cue" cueformat "cuelang.org/go/cue/format" @@ -210,6 +209,8 @@ func (v *Value) Save(fs FS, filename string) (FS, error) { }), nil } +// Check that a value is valid. Optionally check that it matches +// all the specified spec definitions.. func (v *Value) Validate(defs ...string) error { if err := v.val.Validate(); err != nil { return err @@ -238,90 +239,3 @@ func (v *Value) IsEmptyStruct() bool { } return false } - -// Component returns the component value if v is a valid dagger component or an error otherwise. -// If no '#dagger' annotation is present, os.ErrNotExist -// is returned. -func (v *Value) Component() (*Component, error) { - c := &Component{ - v: v, - } - if !c.Exists() { - return c, os.ErrNotExist - } - if err := c.Validate(); err != nil { - return c, err - } - return c, nil -} - -func (v *Value) Script() (*Script, error) { - s := &Script{ - v: v, - } - if err := s.Validate(); err != nil { - return s, err - } - return s, nil -} - -func (v *Value) Executable() (Executable, error) { - if script, err := v.Script(); err == nil { - return script, nil - } - if component, err := v.Component(); err == nil { - return component, nil - } - if op, err := v.Op(); err == nil { - return op, nil - } - return nil, fmt.Errorf("value is not executable") -} - -// ScriptOrComponent returns one of: -// (1) the component value if v is a valid component (type *Component) -// (2) the script value if v is a valid script (type *Script) -// (3) an error otherwise -func (v *Value) ScriptOrComponent() (interface{}, error) { - s, err := v.Script() - if err == nil { - return s, nil - } - c, err := v.Component() - if err == nil { - return c, nil - } - return nil, fmt.Errorf("not a script or component") -} - -func (v *Value) Op() (*Op, error) { - // Merge #Op definition from spec to get default values - spec := v.cc.Spec() - v, err := spec.Get("#Op").Merge(v) - if err != nil { - return nil, err - } - op := &Op{ - v: v, - } - return op, nil -} - -func (v *Value) Mount(dest string) (*Mount, error) { - mnt := &Mount{ - v: v, - dest: dest, - } - return mnt, mnt.Validate() -} - -// Interpret this value as a spec -func (v *Value) Spec() (*Spec, error) { - // Spec must be a struct - if _, err := v.Struct(); err != nil { - return nil, err - } - return &Spec{ - root: v, - }, nil -} diff --git a/dagger/value_test.go b/dagger/value_test.go index 92e8b2e4..287d5b14 100644 --- a/dagger/value_test.go +++ b/dagger/value_test.go @@ -29,28 +29,10 @@ func TestJSON(t *testing.T) { } } -func TestCompileBootScript(t *testing.T) { - cc := &Compiler{} - cfg, err := cc.Compile("boot.cue", baseClientConfig) - if err != nil { - t.Fatal(err) - } - s, err := cfg.Get("bootscript").Script() - if err != nil { - t.Fatal(err) - } - if err := s.Validate(); err != nil { - t.Fatal(err) - } -} - func TestCompileSimpleScript(t *testing.T) { cc := &Compiler{} - s, err := cc.CompileScript("simple.cue", `[{do: "local", dir: "."}]`) + _, err := cc.CompileScript("simple.cue", `[{do: "local", dir: "."}]`) if err != nil { t.Fatal(err) } - if err := s.Validate(); err != nil { - t.Fatal(err) - } } diff --git a/examples/tests/compute/noop/main.cue b/examples/tests/compute/noop/main.cue index 413361f8..b11cdceb 100644 --- a/examples/tests/compute/noop/main.cue +++ b/examples/tests/compute/noop/main.cue @@ -9,11 +9,3 @@ realempty: { empty: { #dagger: compute: [] } - -// additional prop, should not error -withprops: { - #dagger: { - compute: [] - foo: bar: "foo" - } -} diff --git a/examples/tests/compute/undefined_prop/main.cue b/examples/tests/compute/undefined_prop/main.cue index 10d1b06c..42bf7b8a 100644 --- a/examples/tests/compute/undefined_prop/main.cue +++ b/examples/tests/compute/undefined_prop/main.cue @@ -4,17 +4,14 @@ hello: "world" bar: string -#dagger: { - compute: [ - { - do: "fetch-container" - ref: "alpine" - }, - { - do: "exec" - dir: "/" - args: ["sh", "-c", "echo \(foo.bar)"] - }, - ] - foo: bar: bar -} +#dagger: compute: [ + { + do: "fetch-container" + ref: "alpine" + }, + { + do: "exec" + dir: "/" + args: ["sh", "-c", "echo \(bar)"] + }, +] diff --git a/examples/tests/test.sh b/examples/tests/test.sh index 23efca07..49976e7b 100755 --- a/examples/tests/test.sh +++ b/examples/tests/test.sh @@ -25,11 +25,11 @@ test::compute(){ "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/invalid/int test::one "Compute: invalid struct should fail" --exit=1 --stdout= \ "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/invalid/struct - test::one "Compute: noop should succeed" --exit=0 --stdout='{"empty":{},"realempty":{},"withprops":{}}' \ + test::one "Compute: noop should succeed" --exit=0 --stdout='{"empty":{},"realempty":{}}' \ "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/noop test::one "Compute: simple should succeed" --exit=0 --stdout="{}" \ "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/simple - test::one "Compute: unresolved should be ignored" --exit=0 --stdout='{"hello":"world"}' \ + test::one "Compute: script with undefined values should not fail" --exit=0 --stdout='{"hello":"world"}' \ "$dagger" "${DAGGER_BINARY_ARGS[@]}" compute "$d"/compute/undefined_prop }