Private lib schema #319

pivotaljohn · 2021-03-12T16:34:35Z

As part of this PR we decided on some UX decisions described below:

When --enable-experiement-schema flag is not present, and a schema file is provided, a warning is shown. New in this PR the relative path to the file is also shown. (Consider the case where root library has a schema, and a private library has a schema, there will be two warning messages now).
Rules around how the --enable-experiement-schema behaves:
When the --enable-experiement-schema flag is present, and a schema file is given in the root or in a private library, it is used as the base data values and type checked.
When --enable-experiement-schema flag is not enabled, then if a schema file appears anywhere, the schema is IGNORED and a warning is given. This is because the schema flag is protecting the user against non-GA code.

⚠️ Update based on feedback ⚠️
We decided to split this original PR into three separate ones.

develop 
|_overlay-position       <- PR to address position changes within overlay module (separate for this PR entirely)
|_ refactor-dvpp         <- PR to address refactoring done in data_values_pre_processing.go (must merge prior to this PR)
    |_ private-lib-schema <- this PR

pivotaljohn · 2021-03-23T16:40:45Z

When --enable-experiement-schema flag is not enabled, then if a schema file appears anywhere, the schema is IGNORED and a warning is given. This is because the schema flag is protecting the user against non-GA code.

I really like this choice. It gives the author the ability to support both modes. Include both a schema.yml and values/default.yml with basically the same content. If I enable schemas, I get the schema checks. If I don't I get the overlay checks.

cppforlife · 2021-03-25T18:07:44Z

pkg/yttlibrary/overlay/array.go

@@ -43,6 +43,8 @@ func (o Op) mergeArrayItem(
 		}
 		if replace {
 			leftArray.Items[leftIdx].Value = newItem.Value
+			//todo: should we be setting the position elsewhere?
+			leftArray.Items[leftIdx].Position = newItem.Position


can we keep position related improvements separate from this PR. it's a bit hard to tell if these are the only places where we need to change this -- would love to see holistic evaluation within overlay module.

... yup and that PR would precede this one — this fix addresses an actual bug: the wrong file is attributed a value.

These position changes are now necessary to report the position of the overlay in the schema error message, if it's acceptable, we want to keep the change made to the position of arrays and maps here in this PR, and then address this more comprehensively in the Overlay Positions PR.

cppforlife · 2021-03-25T18:12:37Z

pkg/cmd/template/cmd.go

 	})

 	libraryCtx := workspace.LibraryExecutionContext{Current: rootLibrary, Root: rootLibrary}
 	libraryLoader := libraryExecutionFactory.New(libraryCtx)

-	schemaDocs, err := libraryLoader.Schemas()
+	currSchema, err := libraryLoader.Schemas()


is it worth renaming libraryLoader -> rootLibraryLoader

cppforlife · 2021-03-25T18:17:54Z

pkg/schema/schema.go

@@ -73,10 +73,6 @@ func NewMapType(m *yamlmeta.Map) (*MapType, error) {
 		}
 		mapType.Items = append(mapType.Items, mapItemType)
 	}
-	annotations := template.NewAnnotations(m)


note for self: removed because it's dead code.

cppforlife · 2021-03-25T19:03:15Z

pkg/workspace/data_values_pre_processing.go

-		if err != nil {
-			return nil, nil, fmt.Errorf("Templating file '%s': %s", fileInLib.File.RelativePath(), err)
-		}
+	_, checkSchema := o.loader.schema.(*schema.DocumentSchema)


something to think about... why do we need a boolean here? why cant we just tell schema to type check against itself. all schemas should be able to do it succesfully.

cppforlife · 2021-03-25T19:08:05Z

pkg/workspace/data_values_pre_processing.go


-		for _, valuesDoc := range valuesDocs {
-			dv, err := NewDataValues(valuesDoc)
+	allDvs, err := o.collectDataValuesDocs(files)


nice little refactor!

cppforlife · 2021-03-25T19:49:20Z

pkg/workspace/data_values_pre_processing.go

-			return nil, fmt.Errorf("Overlaying additional data values on top of "+
-				"data values from files (marked as @data/values): %s", err)
-		}
+func (o DataValuesPreProcessing) NewEmptyDataValuesDocument() *yamlmeta.Document {


seems like this method could be inlined where it's used, or at least made private.

... in part because there's no need for any code outside this module to generate an empty Data Values.

cppforlife · 2021-03-25T19:49:51Z

pkg/workspace/library_loader.go

@@ -46,30 +46,36 @@ func NewLibraryLoader(libraryCtx LibraryExecutionContext,
 	}
 }

-func (ll *LibraryLoader) Schemas() ([]*yamlmeta.Document, error) {
+func (ll *LibraryLoader) Schemas() (Schema, error) {


if we are returning a single schema should this method be named Schema() as well?

cppforlife · 2021-03-25T19:50:50Z

pkg/workspace/library_loader.go

-		docs, _, err := DocExtractor{resultDocSet}.Extract(AnnotationSchemaMatch)
-		if err != nil {
-			return nil, err
+			docs, _, err := DocExtractor{resultDocSet}.Extract(AnnotationSchemaMatch)


if we get multiple schema docs i would expect that we would blow up for now since we do not allow merging schemas.

Correct, we have a story coming up to allow multiple schema docs #294. Prior to this story we would also blow up if we get multiple docs, so we haven't added guards for that case.

cppforlife · 2021-03-25T19:51:16Z

pkg/workspace/library_module.go

@@ -318,7 +317,8 @@ func (l *libraryValue) libraryValues(ll *LibraryLoader) (*DataValues, []*DataVal
 		}
 	}

-	dvs, foundChildDVss, err := ll.Values(append(dvss, afterLibModDVss...), &schema.AnySchema{})
+	schema, err := ll.Schemas()


missing err check.

cppforlife · 2021-03-25T19:52:00Z

pkg/workspace/library_module.go

@@ -318,7 +317,8 @@ func (l *libraryValue) libraryValues(ll *LibraryLoader) (*DataValues, []*DataVal
 		}
 	}

-	dvs, foundChildDVss, err := ll.Values(append(dvss, afterLibModDVss...), &schema.AnySchema{})
+	schema, err := ll.Schemas()
+	dvs, foundChildDVss, err := ll.Values(append(dvss, afterLibModDVss...), schema)


it seems that we are using ll.Schemas() and then passing it to ll.Values(). i would argue Values() shouldnt require schema as input and should just use ll.Schemas() internally.

Signed-off-by: John Ryan <[email protected]># Please enter the commit message for your changes. Lines starting

- ensure that every overlay of data values (regardless of source) is checked against schema (if enabled). - inline ApplyWithDefaults() - Provide direct error messages by reporting schema errors instead of overlay errors when possible Co-authored-by: Garrett Cheadle <[email protected]>

- Does not yet type check schema - Does not respect `--schema-enabled` flag Co-authored-by: Garrett Cheadle <[email protected]>

Signed-off-by: Garrett Cheadle <[email protected]>

* ytt does not place annotations on maps Signed-off-by: Steven Locke <[email protected]>

- DataValues.HasLib() => DataValues.HasLibRef() (without interpretion, conveys that data values has a reference to a library) - Schema.AsDataValues() => Schema.DefaultDataValues() (slightly more revealing of intented use) - in data_values_pre_processing.go, sorted some function definitions in order of usage. (eases reading when order of usage matches order of definition)

Renaming: - use snake_case for test names to significantly improve readability. (such methods are *never* invoked) - use [slightly] more verbose sentence structure to expose relevant details, in a fluid expression, remaining frugal with words. - reword sub-test names to be phrases extending the containing test name as a sentence. Also: - delete two (2) redundant test cases - reorder tests and sub-tests like an outline of a feature spec.

Signed-off-by: John Ryan <[email protected]>

- Test that when schema is disabled, schema is not used Signed-off-by: John Ryan <[email protected]>

Co-authored-by: Garrett Cheadle <[email protected]>

- Rename Schemas() to Schema() - Rename variables to use qualifier - noun naming scheme

- dvpp has Schema instead of templateLoader

- move logic for schema assinging type and dvs checking themselves into typeAndCheck() - change swtich statement in dvpp to if statements - make newEmptyDataValuesDocument() private Signed-off-by: Cari Dean <[email protected]>

- In favor of a more complete solution, we are removing this typecheck to be addressed in a later issue. - Skipping tests where the overlay fails, but we want a schema error. Signed-off-by: Garrett Cheadle <[email protected]>

vmwclabot added the cla-not-required label Mar 12, 2021

pivotaljohn marked this pull request as draft March 12, 2021 16:34

pivotaljohn assigned StevenLocke, pivotaljohn, cari-lynn and gcheadle-vmware Mar 12, 2021

pivotaljohn linked an issue Mar 12, 2021 that may be closed by this pull request

Type check private library data values #296

Closed

pivotaljohn force-pushed the private-lib-schema branch 2 times, most recently from 02d2f76 to e86994c Compare March 13, 2021 06:32

cari-lynn added breaking change and removed breaking change labels Mar 15, 2021

cari-lynn marked this pull request as ready for review March 22, 2021 23:07

cari-lynn marked this pull request as draft March 22, 2021 23:08

cari-lynn marked this pull request as ready for review March 23, 2021 18:01

cari-lynn marked this pull request as draft March 23, 2021 18:13

cari-lynn force-pushed the private-lib-schema branch from 5b9eb3a to 2e038a0 Compare March 23, 2021 18:56

cari-lynn marked this pull request as ready for review March 23, 2021 18:58

cari-lynn marked this pull request as draft March 23, 2021 22:48

cari-lynn force-pushed the private-lib-schema branch from 2e038a0 to 3223f2f Compare March 23, 2021 23:11

cari-lynn marked this pull request as ready for review March 23, 2021 23:25

cppforlife reviewed Mar 25, 2021

View reviewed changes

cari-lynn changed the base branch from develop to refactor-dvpp March 30, 2021 18:39

cari-lynn changed the base branch from refactor-dvpp to develop March 30, 2021 18:40

cari-lynn force-pushed the private-lib-schema branch from 3223f2f to 58b1ec2 Compare March 30, 2021 18:43

cari-lynn marked this pull request as draft April 5, 2021 17:40

cari-lynn force-pushed the private-lib-schema branch 2 times, most recently from 8d599c5 to a548806 Compare April 5, 2021 23:32

cari-lynn mentioned this pull request Apr 6, 2021

Refactor Data Values Pre-Processing #337

Merged

gcheadle-vmware force-pushed the private-lib-schema branch from 9059336 to 88b3db5 Compare April 6, 2021 21:03

gcheadle-vmware marked this pull request as ready for review April 6, 2021 21:04

cari-lynn and others added 18 commits April 7, 2021 10:32

move schema checking out of template

c48064b

Signed-off-by: John Ryan <[email protected]># Please enter the commit message for your changes. Lines starting

Add test for schema in private library

6ff1db3

Use schema as data values only in library eval()

4da3414

- Does not yet type check schema - Does not respect `--schema-enabled` flag Co-authored-by: Garrett Cheadle <[email protected]>

Don't error when no root lib schema or dv with schema enabled

61f1b30

Type check library schema against programmatic data values

c4f14fd

Signed-off-by: Garrett Cheadle <[email protected]>

Remove nullable annotation check from MapType

cd10681

* ytt does not place annotations on maps Signed-off-by: Steven Locke <[email protected]>

Add test for schema only validates current library

1a745ab

Library functions validate their data values against their schema

037f970

Signed-off-by: John Ryan <[email protected]>

Respect --enable-experiment-schema for libraries

bd3f6c9

- Test that when schema is disabled, schema is not used Signed-off-by: John Ryan <[email protected]>

Move schema post-processing into Schemas function

81aea25

Add path in --enable-experiment-schema warning

29f991f

Co-authored-by: Garrett Cheadle <[email protected]>

Rename variables and functions

6db9081

- Rename Schemas() to Schema() - Rename variables to use qualifier - noun naming scheme

Refactor Values() to call Schema()

18a1c4b

- dvpp has Schema instead of templateLoader

Address feedback in private-lib-schema

e9a9ec8

- move logic for schema assinging type and dvs checking themselves into typeAndCheck() - change swtich statement in dvpp to if statements - make newEmptyDataValuesDocument() private Signed-off-by: Cari Dean <[email protected]>

Remove typechecking on failed data value overlays

5a79068

- In favor of a more complete solution, we are removing this typecheck to be addressed in a later issue. - Skipping tests where the overlay fails, but we want a schema error. Signed-off-by: Garrett Cheadle <[email protected]>

gcheadle-vmware force-pushed the private-lib-schema branch from 14b98a8 to 5a79068 Compare April 7, 2021 17:38

cppforlife merged commit 82645a2 into develop Apr 7, 2021

gcheadle-vmware mentioned this pull request Apr 21, 2021

Release ytt 0.32.0 #362

Closed

20 tasks

gcheadle-vmware deleted the private-lib-schema branch May 26, 2021 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private lib schema #319

Private lib schema #319

pivotaljohn commented Mar 12, 2021 •

edited by cari-lynn

Loading

pivotaljohn commented Mar 23, 2021

cppforlife Mar 25, 2021

pivotaljohn Mar 25, 2021

cari-lynn Apr 6, 2021 •

edited

Loading

cppforlife Mar 25, 2021

cppforlife Mar 25, 2021

cppforlife Mar 25, 2021

cppforlife Mar 25, 2021

cppforlife Mar 25, 2021

pivotaljohn Mar 25, 2021

cppforlife Mar 25, 2021

cppforlife Mar 25, 2021

cari-lynn Mar 30, 2021

cppforlife Mar 25, 2021

cari-lynn Mar 30, 2021

cppforlife Mar 25, 2021

Private lib schema #319

Private lib schema #319

Conversation

pivotaljohn commented Mar 12, 2021 • edited by cari-lynn Loading

pivotaljohn commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cari-lynn Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pivotaljohn commented Mar 12, 2021 •

edited by cari-lynn

Loading

cari-lynn Apr 6, 2021 •

edited

Loading