Feature/conditionals #362

rebeccaskinner · 2016-10-07T19:13:53Z

Support switch conditionals with case branches as per #268

BrianHicks

Seeing a lot of concatenation of fmt.Sprintf calls in general. How come?

BrianHicks · 2016-10-11T20:19:10Z

docs_source/content/getting-started.md

+depending on parameters passed to coverge at execution time, information present
+about the system during run time, or the status of other processes that have
+executed.  If you are already familiar with `switch` statements from other
+languages you may wish to skip ahead to the section on *Rules of Conditionals*


Hugo generates IDs on headers. This one would be #rules-of-conditionals I think. You could just link to it.

Oh okay, I was trying to figure out how to do that.

BrianHicks · 2016-10-11T20:20:14Z

docs_source/content/getting-started.md

+To understand how this works, let's consider the following example:  You wish to
+create a file, `greeting.txt` that contains a greeting like `hello`, however you
+want to be aware of the users preferred language so you would like to ensure
+that the content of the greeting file is in an appropriate language.  Let's look


this sentence has three verbs. So… three SVO sentences?

BrianHicks · 2016-10-11T20:21:33Z

docs_source/content/getting-started.md

+      content     = "hello\n"
+    }
+  }
+}


Here's this HCL ran through converge fmt:

param "lang" { default = "" } switch "test-switch" { case "eq `spanish` `{{param `lang`}}`" "spanish" { file.content "foo-file" { destination = "greeting.txt" content = "hola\n" } } case "eq `french` `{{param `lang`}}`" "french" { file.content "foo-file" { destination = "greeting.txt" content = "salut\n" } } case "eq `japanese` `{{param `lang`}}`" "japanese" { file.content "foo-file" { destination = "greeting.txt" content = "もしもし\n" } } default { file.content "foo-file" { destination = "greeting.txt" content = "hello\n" } } }

BrianHicks · 2016-10-11T20:23:18Z

docs_source/content/getting-started.md

+
+`converge graph --local helloLanguages.hcl | dot -Tpng -o hello-languages.png`
+
+{{< figure src="/images/getting-started/hello-languages.png"


this file is too small to be useful, at least on my screen

hmm, I'll pull a couple of conditions out to simplify

BrianHicks · 2016-10-11T20:27:05Z

docs_source/content/getting-started.md

+*macro expressions* in the graph, denoted by their names.  Each of these nodes
+represent a potential runnable action on the system.  The determination of what,
+if any, of these actions will be run is dependent on the results of branch
+evaluation.


this is phrased from the point of view of the implementer. Could you please reword it (and the following paragraph) in terms of usage instead?

Ah yeah that's a good point.

BrianHicks · 2016-10-11T20:45:48Z

load/resource.go

 			return err
 		}

 		out.Add(id, preparer)
-


why are you removing spacer lines like these? There are a bunch of them.

It's not really on purpose I just have a bad habit of compulsively removing vertical space, I'll try to not do that so much anymore.

BrianHicks · 2016-10-11T20:46:35Z

parse/node.go

+		if n.IsCase() && len(n.Keys) == 3 {
+			break
+		}
+


this area of the code is starting to feel like we should be using an enum and validating length from type, instead of the other way around.

this might be something to open an issue about, though

yeah, I was going to change and it and realized it was a bit too tangential to justify as part of this change set. I'll add an issue.

BrianHicks · 2016-10-11T21:00:14Z

render/preprocessor/control/case.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package control


why is this under render? Shouldn't it be like parse/preprocess/switch?

It should probably be moved. I had preprocessor under render for the lookup stuff and just kind of mentally complexted the two types of preprocessing.

BrianHicks · 2016-10-11T21:09:17Z

render/preprocessor/control/case.go

+// GenerateNode generates a parse.Node for the macro-expanded placeholde from
+// the case clause
+func (c *Case) GenerateNode() (*parse.Node, error) {
+	switchHCL := fmt.Sprintf("macro.case %q {\n", c.Name)


what is even happening here. Why are there so many sprintfs?

Creating an ast.Node from scratch was a lot more fiddly than I anticipated, and it made sense for the moment to just go ahead and sprintf the hcl and load it. I think we should have a separate issue to refactor, this was just for the first pass.

I meant more that there should probably be one sprintf rather than appending multiple together. Go's multiline strings can help a lot here. Or templates!

BrianHicks · 2016-10-11T21:27:06Z

render/preprocessor/control/case_resources.go

+
+func (c *CaseTask) String() string {
+	var out string
+	if nil == c {


why is this flipped?

Bad habit from my C days...

rebeccaskinner · 2016-10-11T23:57:32Z

The heavy use of sprintf is that creating an ast.Node is harder than it really ought to be, sprintf-ing the HCL and parsing it was an easy first pass at getting the nodes generated with the idea that we could have a separate issue to refactor and do it without parsing new hcl.

BrianHicks · 2016-10-12T20:23:20Z

graph/graph.go

+}
+
+// GetParentID uses edge traversal to lookup the parent of the current node.
+func (g *Graph) GetParentID(id string) string {


it'll make it easier for our changes (mine in #369) to merge if this is (id string, ok bool). Explicitly identifies the edge condition, as well.

yeah that makes sense, and easy change.

BrianHicks · 2016-10-12T20:24:58Z

graph/graph.go

+	if fst == g.GetParentID(snd) {
+		return false
+	}
+	if AreSiblingIDs(fst, snd) {


this is still doing the calculation based on IDs. Could you change it to use only the metadata in the graph, or would that be too much?

yeah, I can do that, it was just easier to re-use the logic from GetParentID and I thought that would be sufficient since GetParentID is only looking at the edges and not doing string calculations so we're only really using the string value for the terminal case.

BrianHicks · 2016-10-12T20:28:53Z

load/nodes.go

+// expandSwitchMacro is responsible for adding the generated switch nodes into
+// the graph.  Nodes inside of the switch macro are added as children to the
+// case statements, who are parents of the outer switch statement.  Actual node
+// generation happens in parse/preprocessor/switch/ and we add the nodes into


nit: trailing slash

BrianHicks · 2016-10-12T20:30:26Z

render/preprocessor/control/case.go

+// GenerateNode generates a parse.Node for the macro-expanded placeholde from
+// the case clause
+func (c *Case) GenerateNode() (*parse.Node, error) {
+	switchHCL := fmt.Sprintf("macro.case %q {\n", c.Name)


I meant more that there should probably be one sprintf rather than appending multiple together. Go's multiline strings can help a lot here. Or templates!

ryane · 2016-10-14T14:36:42Z

docs_source/content/getting-started.md

+Converge supports the ability to conditionally execute a set of actions
+depending on the value of expressions that are evaluated at runtime.  These
+`switch` expressions will allow you to write a single converge file that will
+execute differenting depending on information such as:


differently

ryane · 2016-10-14T14:37:13Z

docs_source/content/getting-started.md

+- the status of another resource with `lookup`
+
+To understand how this works, let's consider the following example: You wish to
+create a file, `greeting.txt`. You want that file to contain a greeting like in


do we need "like" here?

ryane

for the most part, just found a few misspellings in comments. also looks like it is out of date with master; otherwise, lgtm.

sorry for the mix of single comments and review. still getting used to the review feature.

ryane · 2016-10-14T14:46:27Z

graph/graph.go

@@ -85,16 +85,46 @@ func (g *Graph) Get(id string) interface{} {

 // GetParent returns the direct parent vertex of the current node.
 func (g *Graph) GetParent(id string) interface{} {
+	parentID, _ := g.GetParentID(id)


not sure if this is a problem or not but it jumped out at me. will this (and similarly graph.Get) just end up returning nil if false is returned from GetParentID?

ryane · 2016-10-14T14:48:34Z

graph/graph_test.go

@@ -148,6 +149,50 @@ func TestDescendents(t *testing.T) {
 	assert.Equal(t, []string{"one/two"}, g.Descendents("one"))
 }

+// TestChildren tests to ensure the correct behavior when getting children
+func TestChildren(t *testing.T) {
+	defer logging.HideLogs(t)()


add t.Parallel()?

ryane · 2016-10-14T14:53:02Z

parse/node_test.go

@@ -87,6 +87,38 @@ func TestNodeValidateTooManyKeysModule(t *testing.T) {
 	validateTable(t, `module x y z {}`, "1:1: too many keys")
 }

+// TestNodeCase tests various scenarios where the node is a case statement
+func TestNodeCase(t *testing.T) {


do we want t.Parallel() in these tests?

ryane · 2016-10-14T14:53:32Z

parse/preprocessor/switch/case.go

+	InnerNodes []*parse.Node
+}
+
+// GenerateNode generates a parse.Node for the macro-expanded placeholde from


placeholder

ryane · 2016-10-14T15:00:49Z

parse/preprocessor/switch/switch_resources.go

+	"github.com/asteris-llc/converge/resource"
+)
+
+// SwitchPreparer represents a switch resourc; the task it generates simply


ryane · 2016-10-14T15:02:00Z

parse/preprocessor/switch/switch_resources.go

+	return &resource.Status{}, nil
+}
+
+func (s *SwitchTask) String() string {


needs comment?

ryane · 2016-10-14T15:05:26Z

transform/control.go

+)
+
+// ResolveConditionals will walk the graph and wrap tasks whose parent is a case
+// in a conditional resource.  For cases it will look at the parent switch and


ryane · 2016-10-14T15:07:12Z

parse/preprocessor/switch/case_resources.go

+	if c == nil {
+		return "<nil>"
+	}
+	out += fmt.Sprintf("Case: \n")


would a multiline string be more readable?

Agreed here, and in the cases elsewhere where there are multiline strings with multiple calls to fmt.Sprintf

Conditional evaluation is working with the following caveats: * Lookups between inner/outer modules do not work * All cases with true predicates are evaluated * Unevaluated branches are not fully prepared * Error messages are a little broken

Need to fix graph traversal for render so that when dependent niblings from switch macros are expanded they have a valid parent type for thunk generation.

…nches

This reverts commit 3f2d99c.

…or nibling tests

ryane · 2016-10-14T20:50:21Z

lgtm on green

Feature/conditionals

rebeccaskinner mentioned this pull request Oct 7, 2016

add conditionals to module resource #268

Closed

rebeccaskinner force-pushed the feature/conditionals branch 3 times, most recently from 7fd5ac0 to 49b44cc Compare October 11, 2016 20:16

BrianHicks suggested changes Oct 11, 2016

View reviewed changes

BrianHicks reviewed Oct 12, 2016

View reviewed changes

ryane reviewed Oct 14, 2016

View reviewed changes

rebeccaskinner force-pushed the feature/conditionals branch from 7564005 to 8bb1303 Compare October 14, 2016 15:01

ryane reviewed Oct 14, 2016

View reviewed changes

stevendborrelli added the enhancement label Oct 14, 2016

stevendborrelli added this to the 0.3.0 milestone Oct 14, 2016

rebeccaskinner force-pushed the feature/conditionals branch from 8bb1303 to 8d21fca Compare October 14, 2016 20:13

Rebecca Skinner added 16 commits October 14, 2016 15:45

look upwards at nodes

51fd338

initial prototype code for parsing a switch

a6f9c0e

wip: prototype

88a7827

initial control structure preprocessor

e7c5514

additional control structure preprocessing feature work

760bcec

in-progress on conditional rendering

e37e3d7

add children call to graph

b32acd4

fixes to handle deserialization overwriting public fields

dbc0a44

WIP: Conditional Evaluation

b07c070

Conditional evaluation is working with the following caveats: * Lookups between inner/outer modules do not work * All cases with true predicates are evaluated * Unevaluated branches are not fully prepared * Error messages are a little broken

WIP: graph execution (broken)

590a764

Need to fix graph traversal for render so that when dependent niblings from switch macros are expanded they have a valid parent type for thunk generation.

lookups fail when branch is unevaluated

1985f25

tests passing; TODO: only evaluate first case

d5c5acd

validate renderer lookup to prevent looking inside of unevaluated bra…

33903f0

…nches

move control transforms out of load

5cbe72a

wip: move branch trimming to after rendering

0b32700

better support for switch cases

72c6311

Rebecca Skinner added 23 commits October 14, 2016 15:45

Revert "compile docs"

517c471

This reverts commit 3f2d99c.

remove root-depth family of functions since they are no longer necessary

f6b616c

move nibling lookup to graph and away from ids

d9d1e1b

move render/preprocessor/control to parse/preprocessor/switch

7379a44

fix imports

d64f2f0

remove debugging output

90b5e75

remove extra nil check

d2a9410

remove debug print statement

3330ad2

flip nil check

1ff89bf

remove unused graph parameter from getDepends

4049164

add comments explaining the reason for switch macro code in nodes.go

24564f6

update docs

4aa5ff8

remove unused image file

04945a6

add sibling check using parent edges instead of node IDs and use it f…

09adc64

…or nibling tests

remove trailing slash

ddf4b3b

clean up sprintf generate of switch and case expansions

63bdf61

fix typos in docs

dfac680

fix post-rebase build errors

c7c46ed

fix dependency resolution merge bug

41167a3

update graph api in tests

6e7e1e0

typos

580b006

multi-line string instead of sprintfs

3db3637

multi-line-ification

0bbc6e0

rebeccaskinner force-pushed the feature/conditionals branch from e343bef to 0bbc6e0 Compare October 14, 2016 20:46

change the type of dependencyGenerator instead of wrapping them

4b94758

BrianHicks approved these changes Oct 14, 2016

View reviewed changes

BrianHicks merged commit 95aec5f into master Oct 14, 2016

BrianHicks deleted the feature/conditionals branch October 14, 2016 21:00

BrianHicks added a commit that referenced this pull request Dec 22, 2016

Merge pull request #362 from asteris-llc/feature/conditionals

c0ece90

Feature/conditionals


		`converge graph --local helloLanguages.hcl \| dot -Tpng -o hello-languages.png`

		{{< figure src="/images/getting-started/hello-languages.png"

Feature/conditionals #362

Feature/conditionals #362

Conversation

rebeccaskinner commented Oct 7, 2016

BrianHicks 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

Choose a reason for hiding this comment

BrianHicks Oct 11, 2016 • 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

rebeccaskinner commented Oct 11, 2016

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

ryane 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

ryane commented Oct 14, 2016

BrianHicks Oct 11, 2016 •

edited

Loading