Implement bootstraping code for submessage support inside templates (part2). by guptasu · Pull Request #1702 · istio/istio

guptasu · 2017-11-14T02:21:32Z

What this PR does / why we need it:
NOTE Please read the entire description before reviewing. I can bet that it will help in the review.

This PR implements code generation of template bootstraping code (code that gets compiled in Mixer. It instantiates instances and types from config for all templates and dispatches them to adapter). Previous part 1 PR was responsible for generating definitions of instance and type constructs from templates with sub-messages. This PR is responsible for instantiating those generated artifacts based on user config. It finishes the submessage support work.

To support submessages, the generated code is restructured in this PR so that we can do recursions. Changes are in the following generated functions : InferType and ProcessCheck, ProcessQuota and ProcessReport. The new layout is something like this :

// This function creates an instance from config and dispatches to the adapter
ProcessCheck (config, adapter) {
   // Main function that build the instance. It might call into other Build functions to build sub instances for
   // the sub-messages
   BuildTemplate = func(config InstanceParam) Instance {
     var resultInst Instance
     resultInst.Value = Eval(config.Value)
     ..
     if (typeOf config.subMsgField is ResourceMessage) // this is a submessage
        instance.subMsgField = BuildSubMsg(config.subMsgField)
     }
     return resultInst
   }
   BuildSubMsg = func(config SubMsgInstanceParam) SubMsgInstance {
     var resultInst SubMsgInstance
     resultInst.field1 = Eval(config.field1)
     ..
     if (typeOf config.anotherSubMsgField is ResourceMessage) // this is a submessage
        instance.anotherSubMsgField = BuildAnotherSubMsg(config.anotherSubMsgField)
     }
     return resultInst
   }

   inst := BuildTemplate(config)

   ..
   adapter.Handle... (inst, ...)
}

NOTE: This PR is tricky to review because it is hard to visualize the diff of golang template file and the corresponding generated code. I recommend start reviewing the tests first and the baseline generated code after that (sample test files (check.proto, quota.proto and report.proto) and baseline generated file is AllTemplate.go.golden).

Also NOTE : There are two types of tests that are implemented in this PR:

Baseline test under /tools/codegen/pkg/bootstrap which just compares the generated code with baseline view.
Other is run-time test for the generated code and it lives inside /template/sample/template.gen_test.go

Release note:

support sub messages in Mixer template

@douglas-reid

Automatic merge from submit-queue. Fix submessage support part1 **What this PR does / why we need it**: NOTE: The changes in this PR were already approved by @douglas-reid @geeknoid. Old PR contained 12 files (#1617) but was reverted because of missing 2 generated pb.go file. I have added the missing 2 generated file in this PR (this PR therefore contains 14 files). Note this is part 1 of sub message support. It is based on the proposal https://docs.google.com/document/d/1GL_EOitfBJDfgbQJOSROceK5RdqNPuiggeAebaNqyf8/edit# This PR contains the code generation for the go Instances and the augmented proto messages (Type and InstanceParam) Part 2 (following [PR](#1702)) will contain the bootstraping code generation (code that runs at run-time, inside Mixer framework and instantiates these sub-message artifacts generated in this PR). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note none ```

guptasu · 2017-11-22T01:36:25Z

@ozevren @geeknoid @douglas-reid @mandarjog friendly ping

codecov · 2017-11-28T02:15:47Z

Codecov Report

Merging #1702 into master will decrease coverage by 0.52%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   81.67%   81.15%   -0.53%     
==========================================
  Files         201      187      -14     
  Lines       20447    18889    -1558     
==========================================
- Hits        16701    15329    -1372     
+ Misses       3275     3128     -147     
+ Partials      471      432      -39

Flag	Coverage Δ
#broker	`45.51% <ø> (ø)`	⬆️
#mixer	`82.42% <ø> (-0.78%)`	⬇️
#pilot	`80.35% <ø> (-0.06%)`	⬇️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/model/egress_rules.go	`100% <ø> (ø)`	⬆️
pilot/adapter/config/memory/monitor.go	`79.48% <0%> (-10.26%)`	⬇️
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
mixer/pkg/runtime/resourceType.go
pilot/platform/kube/inject/inject.go
mixer/pkg/runtime/controller.go
mixer/pkg/runtime/monitor.go
mixer/pkg/runtime/handlerTable.go
mixer/pkg/runtime/env.go
pilot/platform/kube/inject/http.go
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e36ed7b...07ef3e0. Read the comment docs.

guptasu · 2017-11-28T02:16:08Z

@geeknoid friendly ping

geeknoid

/lgtm

istio-merge-robot · 2017-11-28T13:54:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~mixer/OWNERS~~ [geeknoid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ozevren · 2017-11-28T16:58:04Z

mixer/pkg/config/manager_test.go

Nit: space?

This reverts commit 06ffe78f47a3aa33c925fddd6b9541a09bb4ebb1.

istio-merge-robot · 2017-11-28T18:51:53Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @geeknoid @guptasu

guptasu · 2017-11-28T21:14:09Z

@geeknoid @ozevren re-lgtm please. I have addressed linter issues + added .gen.go files.

@douglas-reid

Automatic merge from submit-queue. Fix submessage support part1 **What this PR does / why we need it**: NOTE: The changes in this PR were already approved by @douglas-reid @geeknoid. Old PR contained 12 files (istio/istio#1617) but was reverted because of missing 2 generated pb.go file. I have added the missing 2 generated file in this PR (this PR therefore contains 14 files). Note this is part 1 of sub message support. It is based on the proposal https://docs.google.com/document/d/1GL_EOitfBJDfgbQJOSROceK5RdqNPuiggeAebaNqyf8/edit# This PR contains the code generation for the go Instances and the augmented proto messages (Type and InstanceParam) Part 2 (following [PR](istio/istio#1702)) will contain the bootstraping code generation (code that runs at run-time, inside Mixer framework and instantiates these sub-message artifacts generated in this PR). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note none ```

istio-testing added the do-not-merge/release-note-label-needed label Nov 14, 2017

googlebot added the cla: yes label Nov 14, 2017

guptasu requested review from douglas-reid, geeknoid, mandarjog and ozevren November 14, 2017 02:30

guptasu changed the title ~~Implement bootstraping code for submessage support inside templates.~~ Implement bootstraping code for submessage support inside templates (part2). Nov 14, 2017

istio-testing added release-note and removed do-not-merge/release-note-label-needed labels Nov 14, 2017

guptasu mentioned this pull request Nov 15, 2017

Fix submessage support part1 #1736

Merged

guptasu force-pushed the submsgpart2 branch from ee4f5f3 to 24ad44b Compare November 16, 2017 06:43

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 17, 2017

guptasu force-pushed the submsgpart2 branch from 24ad44b to 06ffe78 Compare November 22, 2017 01:41

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 22, 2017

guptasu force-pushed the submsgpart2 branch from af764d7 to bcba3d8 Compare November 28, 2017 02:15

yutongz added the do-not-merge/post-submit label Nov 28, 2017

geeknoid reviewed Nov 28, 2017

View reviewed changes

istio-testing assigned geeknoid Nov 28, 2017

istio-testing added the lgtm label Nov 28, 2017

istio-merge-robot added the approved label Nov 28, 2017

yutongz added the do-not-merge/post-submit label Nov 28, 2017

ozevren reviewed Nov 28, 2017

View reviewed changes

mixer/pkg/config/manager_test.go Outdated

Copy link
Copy Markdown

Contributor

ozevren Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: space?

ozevren approved these changes Nov 28, 2017

View reviewed changes

guptasu added 5 commits November 28, 2017 10:44

Implement bootstraping code for submessage support inside templates.

ea76c33

Fix error reporting

fbae649

make fmt

79c8a6f

Revert "make fmt"

d785a78

This reverts commit 06ffe78f47a3aa33c925fddd6b9541a09bb4ebb1.

Add generated files.

5017640

guptasu force-pushed the submsgpart2 branch from bcba3d8 to 5017640 Compare November 28, 2017 18:51

istio-merge-robot removed the lgtm label Nov 28, 2017

guptasu added 3 commits November 28, 2017 11:33

make fmt

6410f93

Make linter happy

3c17b74

make linter happy.

07ef3e0

guptasu merged commit d5d7d57 into istio:master Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement bootstraping code for submessage support inside templates (part2).#1702

Implement bootstraping code for submessage support inside templates (part2).#1702
guptasu merged 8 commits intoistio:masterfrom
guptasu:submsgpart2

guptasu commented Nov 14, 2017 •

edited

Loading

Uh oh!

guptasu commented Nov 22, 2017

Uh oh!

codecov bot commented Nov 28, 2017 •

edited

Loading

Uh oh!

guptasu commented Nov 28, 2017

Uh oh!

geeknoid left a comment

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

ozevren Nov 28, 2017

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

guptasu commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

guptasu commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guptasu commented Nov 22, 2017

Uh oh!

codecov bot commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

guptasu commented Nov 28, 2017

Uh oh!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

ozevren Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Nov 28, 2017

Uh oh!

guptasu commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

guptasu commented Nov 14, 2017 •

edited

Loading

codecov bot commented Nov 28, 2017 •

edited

Loading