cli: Add Validate command by aequitas · Pull Request #361 · purpleidea/mgmt

aequitas · 2018-02-26T17:51:47Z

Todo:

docs
lint on deploy
test-lint-mcl test for examples

purpleidea

Patch is looking good, however we need to call this something different than lint. Ideas?

purpleidea · 2018-02-26T20:32:53Z

test/test-examples-lint.sh

+failures=''
+
+# lint .mcl examples
+for file in $(find examples/lang/ -maxdepth 3 -type f -name '*.mcl'); do


I'm really pleased you added a test for this, because I was thinking we needed this :)

purpleidea · 2018-02-26T20:33:31Z

examples/lang/syntaxerror.mcl.txt

@@ -0,0 +1,4 @@
+# this file generates a syntax error, used in lib/cli_test.go
+file "/tmp/mgmt" {
+  content => "test"  # missing trailing comma


tab for indentation, unless your point was that spaces should be a compile error ;)

That would be a good point :)

purpleidea · 2018-02-26T20:34:19Z

lib/cli_test.go

+	"github.com/urfave/cli"
+)
+
+func TestLintPass(t *testing.T) {


what's going on in these two?

purpleidea · 2018-02-26T20:35:57Z

lang/lang.go

 }
+
+func (obj *Lang) Lint() error {
+	_, err := LexParse(obj.Input)


This is actually the part which needs the most work... You probably want all of these to run:

https://github.com/purpleidea/mgmt/blob/master/docs/language-guide.md#stages

With the exception of actually running the function engine. All of those are needed to ascertain if a compile is successful.

purpleidea

Most of this patch is good. I've got to step back and think about if the refactor into compile is what we want. Please fixup these issues and I'll think about the refactor.

Thanks!

purpleidea · 2018-02-27T23:47:52Z

examples/lang/bad/scopeerror.mcl

@@ -0,0 +1,3 @@
+# this file generates a scope error, used in lib/cli_test.go


all "bad" things should be in test/ dir, not easy to find in examples/

in fact, the real tests should have nothing to do with examples/

I have a test case that tests examples just to make sure the examples we submit aren't actually junk, but the real testing should happen in test/

purpleidea · 2018-02-27T23:49:48Z

examples/lang/bad/syntaxerror.mcl

@@ -0,0 +1,4 @@
+# this file generates a syntax error, used in lib/cli_test.go


let's not refer to a specific file like this

purpleidea · 2018-02-27T23:50:19Z

examples/lang/bad/unificationerror.mcl

@@ -0,0 +1,2 @@
+# this file generates a unification error, used in lib/cli_test.go


okay, however why not write this as a test in unification_test.go instead?

i want to test validate is able to detect a unification error, not that this file generates a unification error per se.

purpleidea · 2018-02-27T23:50:48Z

lang/gapi.go

 		}

+		// validate syntax before deploying
+		filecontent, err := ioutil.ReadFile(s)


fileContent

purpleidea · 2018-02-28T00:00:41Z

lang/gapi.go

+		obj := &Lang{
+			Input: code,
+			Logf: func(format string, v ...interface{}) {
+				log.Printf(Name+": funcs: "+format, v...)


what? why funcs?

probably a copy paste error i overlooked

purpleidea · 2018-02-28T00:01:30Z

lang/gapi.go

 		// read through this local path, and store it in our file system
 		// since our deploy should work anywhere in the cluster, let the
 		// engine ensure that this file system is replicated everywhere!
-


why is this line missing?
if there's a reason, okay, but it sends the message you don't review your diffs before submitting.
the goal should be for me to see your patch and say "perfect, merged" :)

must have slipped past

purpleidea · 2018-02-28T00:03:49Z

lib/cli_test.go

+	files := []struct {
+		path string
+	}{
+		{"../examples/lang/bad/syntaxerror.mcl"},


no ../ paths please and no references to examples/ in our code.
we need to think of a better way to get the code in. i'd actually prefer to just inline it for now. see:

https://github.com/purpleidea/mgmt/blob/master/lang/lexparse_test.go#L93

…run locally

aequitas · 2018-03-13T11:53:58Z

#368 (comment)

aequitas force-pushed the lint branch 3 times, most recently from bbfabcf to 03ad9d1 Compare February 26, 2018 19:20

purpleidea requested changes Feb 26, 2018

View reviewed changes

aequitas force-pushed the lint branch 2 times, most recently from 4b219a4 to 6f7dbd2 Compare February 26, 2018 20:55

purpleidea changed the title ~~cli: Add lint command~~ cli: Add Validate command Feb 26, 2018

aequitas force-pushed the lint branch 6 times, most recently from faf5def to 71436a1 Compare February 27, 2018 13:11

aequitas mentioned this pull request Feb 27, 2018

Lang: don't panic if more arguments are past to an function than expected #362

Closed

aequitas force-pushed the lint branch from 71436a1 to 9806882 Compare February 27, 2018 23:41

purpleidea requested changes Feb 28, 2018

View reviewed changes

aequitas force-pushed the lint branch 9 times, most recently from 7294036 to 2f67d1b Compare March 5, 2018 16:19

aequitas added 5 commits March 5, 2018 19:33

test: Go integrationtests

6442026

fun: Meh

9691622

lang: Fix example failing validation

aa3b2a1

test: Make commit message test better debuggable, also allow them to …

eaef08a

…run locally

cli: Add lint command

69e2390

aequitas force-pushed the lint branch from 2f67d1b to 69e2390 Compare March 5, 2018 19:49

aequitas closed this Mar 13, 2018

		@@ -0,0 +1,3 @@
		# this file generates a scope error, used in lib/cli_test.go

		@@ -0,0 +1,4 @@
		# this file generates a syntax error, used in lib/cli_test.go

		@@ -0,0 +1,2 @@
		# this file generates a unification error, used in lib/cli_test.go

Uh oh!

Conversation

aequitas commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

purpleidea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

purpleidea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aequitas commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aequitas commented Feb 26, 2018 •

edited

Loading