Added validation for extra field #481

surajnarwade · 2017-11-22T11:43:26Z

For validation purpose, we are using gojsonschema library and json schema for kedge file,
we are downloading from kedgeproject/json-schema

surajnarwade · 2017-11-22T11:44:10Z

@kadel @cdrage @surajssd @containscafeine can you please try this PR and let me know feedback for further changes

surajssd · 2017-11-22T13:35:53Z

For this there are few things we need to figure out @kadel @containscafeine @cdrage need your help here:

So here the validation happens on the external config files a.k.a. jsonschema which is located outside of the project. Now how do we handle/manage those external files. Do we download those files on each invocation of kedge(of course the last thing we wanna do)? Download it locally in home directory and keep it cached, update on every new version of kedge release? Skip validation with warning if the file cannot be downloaded? Need your help here guys!

surajssd

Once we figure out the way to manage these configs, accodingly we can either download them on every request or cache it or put it in code and update it on every release of kedge or something like that!

kadel · 2017-11-22T15:59:41Z

So here the validation happens on the external config files a.k.a. jsonschema which is located outside of the project. Now how do we handle/manage those external files. Do we download those files on each invocation of kedge(of course the last thing we wanna do)? Download it locally in home directory and keep it cached, update on every new version of kedge release? Skip validation with warning if the file cannot be downloaded? Need your help here guys!

Those files need to be embedded into Kedge binary, somehow 😏

kadel · 2017-11-22T16:05:41Z

we can do something like this:

file: jsonSchemas.go

package jsonSchema

const DeploymentspecmodJson := `{
"$schema": "http://json-schema.org/schema#", 
"required": [
     "containers", 
     "name"
...
...
`

const JobspecModJson := `{
 "$schema": "http://json-schema.org/schema#", 
...
...
`

...
...

kadel · 2017-11-22T16:05:58Z

how is Kubernetes doing it?

surajssd · 2017-11-23T08:36:14Z

@kadel AFAIK the client downloads the schema from server

kadel · 2017-11-23T08:37:50Z

@kadel AFAIK the client downloads the schema from server

ah, right.

cdrage

To be honest, having more comments / descriptions on what's happening would benefit when reviewing it 👍 But confused on the validate part!

cdrage · 2017-11-29T16:38:18Z

pkg/cmd/commands.go

+		}
+	}
+
+}


Although WIP, entire code here needs to be spaced out more / added comments in order to understand what's happening 👍

@cdrage , added comments now, I hope it will help now

cdrage

Please check e2e tests / run on your own machine. Something weird is messing up.

kadel · 2018-01-02T13:11:41Z

can you please add vendor changes as a separate commit?

kadel · 2018-01-08T15:11:54Z

pkg/validation/validation.go

+// validate function will validate input kedgefile against JSON schema provided in pkg/validation
+func Validate(p []byte) {
+	var speco interface{}
+	err := yaml.Unmarshal(p, &speco)


If we can't avoid this then we need explanation why we are doing it like this

kadel · 2018-01-08T16:11:43Z

pkg/cmd/commands.go

@@ -56,6 +56,12 @@ func CreateArtifacts(paths []string, generate bool, args ...string) error {
 		}

 		ros, includeResources, err := spec.CoreOperations(kedgeData)
+
+		// Validate input kedge file
+		if !skipValidation {


shouldn't we move this before CoreOperations?

Yeah , that will save much time and operation

kadel · 2018-01-18T10:49:33Z

#565 was merged, we can now update this accordingly once #566 is merged we can use it as tests for this PR. Tests from #566 should be passing with this PR before we merge this

surajnarwade · 2018-02-05T11:18:16Z

@kadel , Ready for review :)
Probably works for every case now

cdrage

Works locally (pulling down the PR and running the binary) against a few examples.

Code LGTM.

Have a few spelling errors / suggestions.

Perhaps the only suggestion is to add a few verbosity logging lines?

cdrage · 2018-02-07T17:58:02Z

pkg/validation/validation.go

+	loader := gojsonschema.NewGoLoader(body)
+
+	var schema gojsonschema.JSONLoader
+	schema = gojsonschema.NewStringLoader(SchemaJson)


Can you add a comment above this line that we are retrieving SchemaJson from pkg/validation/kedgeschema.go ? Also that it is automatically generated when using make update-schema

cdrage · 2018-02-07T17:58:18Z

Makefile

+.PHONY: update-schema
+update-schema:
+	./scripts/update-schema.sh
+	#docker run --rm  -v `pwd`:/data:Z  surajnarwade/update-kedge-schema


remove commented line

cdrage · 2018-02-07T17:59:50Z

cmd/apply.go

@@ -53,5 +53,6 @@ var applyCmd = &cobra.Command{
 func init() {
 	applyCmd.Flags().StringArrayVarP(&InputFiles, "files", "f", []string{}, "Specify files")
 	applyCmd.Flags().StringVarP(&Namespace, "namespace", "n", "", "Namespace or project to deploy your application to")
+	applyCmd.Flags().BoolVar(&SkipValidation, "skip-validation", false, "Skip Validation of input file")


Skip validation of Kedge file may be better..

cdrage · 2018-02-07T18:00:00Z

cmd/create.go

@@ -53,6 +53,7 @@ func init() {
 	createCmd.Flags().StringArrayVarP(&InputFiles, "files", "f", []string{}, "Specify files")
 	createCmd.MarkFlagRequired("files")
 	createCmd.Flags().StringVarP(&Namespace, "namespace", "n", "", "Namespace or project to deploy your application to")
+	createCmd.Flags().BoolVar(&SkipValidation, "skip-validation", false, "Skip Validation of input file")


Skip validation of Kedge file

cdrage · 2018-02-07T18:00:12Z

cmd/generate.go

@@ -43,5 +43,6 @@ var generateCmd = &cobra.Command{
 func init() {
 	generateCmd.Flags().StringArrayVarP(&InputFiles, "files", "f", []string{}, "input files")
 	generateCmd.MarkFlagRequired("files")
+	generateCmd.Flags().BoolVar(&SkipValidation, "skip-validation", false, "Skip Validation of input file")


Skip validation of Kedge file

kadel · 2018-02-12T16:26:51Z

scripts/modify-schema.js

@@ -0,0 +1,42 @@
+var fs = require('fs')


javascript? 😞

Because it's easy to play with big JSON ;)

It just sucks if you have to run it :-(

thats why I put Docker image for it

kadel · 2018-02-12T16:28:50Z

Makefile

+# Update schema which is used for json-schema validation
+.PHONY: update-schema
+update-schema:
+	docker run --rm  -v `pwd`:/data:Z  surajnarwade/update-kedge-schema


There is kedge org in docker hub, we should use that.

it seems that I don't have access to that org, can someone please add me there

same here :-( @cdrage should have access.

kadel · 2018-02-12T16:30:06Z

docker image should be in kedge org in docker hub (https://hub.docker.com/r/kedge), so every kedge maintainer can update it

For validation purpose, we are `gojsonschema` library and json schema for kedge file, JSON schema are located in `pkg/validation` directory

surajnarwade · 2018-02-13T15:46:55Z

@kadel @cdrage pushed docker image to new home updated docker image name

cdrage · 2018-02-13T19:59:52Z

@surajnarwade Awesome. That was the last conflict! Going to merge this in, in-time for today's release!

surajnarwade changed the title ~~Added validation for extra field~~ [WIP]Added validation for extra field Nov 22, 2017

surajssd suggested changes Nov 22, 2017

View reviewed changes

surajnarwade mentioned this pull request Nov 28, 2017

kedge does not fail when extra fields specified #168

Closed

surajnarwade force-pushed the validation branch 4 times, most recently from 491dffc to c620d91 Compare November 29, 2017 08:09

cdrage suggested changes Nov 29, 2017

View reviewed changes

surajnarwade force-pushed the validation branch 2 times, most recently from f472232 to 2ee7b40 Compare December 4, 2017 07:03

surajnarwade added the status/work-in-progress label Dec 4, 2017

cdrage suggested changes Dec 5, 2017

View reviewed changes

surajnarwade force-pushed the validation branch 2 times, most recently from 5cca0e9 to 32f2613 Compare December 7, 2017 12:40

surajnarwade force-pushed the validation branch 7 times, most recently from 2d6ff09 to 890443c Compare December 27, 2017 08:48

surajnarwade force-pushed the validation branch from 890443c to 8f475fe Compare January 2, 2018 07:16

kadel reviewed Jan 8, 2018

View reviewed changes

surajnarwade force-pushed the validation branch 3 times, most recently from 9891ce0 to 897e4ea Compare January 10, 2018 06:58

surajnarwade mentioned this pull request Jan 17, 2018

New Kedge file structure #565

Merged

8 tasks

kadel mentioned this pull request Jan 17, 2018

Test kedgefiles from examples #566

Merged

kadel added the status/needs-rebase label Jan 18, 2018

surajnarwade force-pushed the validation branch 3 times, most recently from 75518c5 to b4cf7cf Compare January 18, 2018 13:57

surajnarwade removed the status/needs-rebase label Jan 18, 2018

surajnarwade force-pushed the validation branch 2 times, most recently from f865282 to f78c5a4 Compare February 5, 2018 06:16

surajnarwade changed the title ~~[WIP]Added validation for extra field~~ Added validation for extra field Feb 5, 2018

surajnarwade removed the status/work-in-progress label Feb 6, 2018

cdrage approved these changes Feb 7, 2018

View reviewed changes

surajnarwade force-pushed the validation branch from f78c5a4 to 7db1cb6 Compare February 8, 2018 06:12

kadel reviewed Feb 12, 2018

View reviewed changes

kadel approved these changes Feb 12, 2018

View reviewed changes

kadel reviewed Feb 12, 2018

View reviewed changes

surajnarwade added 2 commits February 13, 2018 14:23

Updated Vendoring for gojsonschema package

2e2675a

Added validation for extra field

4c42307

For validation purpose, we are `gojsonschema` library and json schema for kedge file, JSON schema are located in `pkg/validation` directory

surajnarwade force-pushed the validation branch from 7db1cb6 to 4c42307 Compare February 13, 2018 15:46

cdrage merged commit 81857c6 into kedgeproject:master Feb 13, 2018

+              		}
+              	}
+              }

Added validation for extra field #481

Added validation for extra field #481

Conversation

surajnarwade commented Nov 22, 2017 • edited Loading

surajnarwade commented Nov 22, 2017

surajssd commented Nov 22, 2017

surajssd left a comment

Choose a reason for hiding this comment

kadel commented Nov 22, 2017

kadel commented Nov 22, 2017

kadel commented Nov 22, 2017

surajssd commented Nov 23, 2017

kadel commented Nov 23, 2017

cdrage left a 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

cdrage left a comment

Choose a reason for hiding this comment

kadel commented Jan 2, 2018

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

kadel commented Jan 18, 2018

surajnarwade commented Feb 5, 2018 • edited Loading

cdrage left a 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

Choose a reason for hiding this comment

kadel commented Feb 12, 2018 • edited Loading

surajnarwade commented Feb 13, 2018

cdrage commented Feb 13, 2018

surajnarwade commented Nov 22, 2017 •

edited

Loading

surajnarwade commented Feb 5, 2018 •

edited

Loading

kadel commented Feb 12, 2018 •

edited

Loading