Add support for submessages in template - part1. by guptasu · Pull Request #1617 · istio/istio

guptasu · 2017-11-09T02:44:02Z

What this PR does / why we need it:
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) 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:

supports submessages inside the Mixer Template system.

douglas-reid

There's a lot going on here, but it looks reasonable to me. Just a few very small questions.

douglas-reid · 2017-11-09T17:25:39Z

mixer/tools/codegen/pkg/interfacegen/generator.go

fwiw: you can use raw string literals if you would like to avoid escaping nested quotes.

douglas-reid · 2017-11-09T17:29:36Z

mixer/tools/codegen/pkg/interfacegen/testdata/ReportTmpl.proto

should Resource1 be referenced anywhere? what is this testing? Is this just verifying that all messages within the proto file get types and instance params, etc. ?

So, every message in the proto will have an equivalent *InstanceParam and *Type. For simplicity I choose to generate these artifacts for every message irrespective if they are reachable from the parent 'Template' message or not. We can certainly traverse from the Template and only include what is reachable, however, doing it the existing way is simple and I hope people just don't write dead messages (not referenced from anywhere). The Template developer should be a reasonable person :-), and will fix the dead message if he/she accidentally defines any.

Thoughts ?

As you stated in your comment, this PR and the test is testing what gets generated for Go Instances, Type and InstanceParam messages.

geeknoid

lgtm

guptasu

@douglas-reid PTAL. I have addressed your comments.

guptasu · 2017-11-09T18:18:40Z

mixer/tools/codegen/pkg/interfacegen/testdata/ReportTmpl.proto

So, every message in the proto will have an equivalent *InstanceParam and *Type. For simplicity I choose to generate these artifacts for every message irrespective if they are reachable from the parent 'Template' message or not. We can certainly traverse from the Template and only include what is reachable, however, doing it the existing way is simple and I hope people just don't write dead messages (not referenced from anywhere). The Template developer should be a reasonable person :-), and will fix the dead message if he/she accidentally defines any.

Thoughts ?

guptasu · 2017-11-09T18:21:00Z

mixer/tools/codegen/pkg/interfacegen/testdata/ReportTmpl.proto

As you stated in your comment, this PR and the test is testing what gets generated for Go Instances, Type and InstanceParam messages.

yutongz · 2017-11-10T02:05:40Z

/test istio-presubmit

douglas-reid · 2017-11-13T20:08:05Z

/lgtm

istio-merge-robot · 2017-11-13T20:08:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid

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~~ [douglas-reid]

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

…l contain the bootstraping code generation (code that constructs the recurrsive submessages)

This reverts commit c349b99.

) This reverts commit c349b99.

@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 ```

@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 release-note label Nov 9, 2017

googlebot added the cla: yes label Nov 9, 2017

guptasu changed the title ~~Add support for submessages in interface generation.~~ Add support for submessages in template - part1. Nov 9, 2017

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

douglas-reid added the mixer label Nov 9, 2017

douglas-reid reviewed Nov 9, 2017

View reviewed changes

geeknoid approved these changes Nov 9, 2017

View reviewed changes

guptasu force-pushed the submessages3 branch from c55236b to 537cc0e Compare November 9, 2017 18:33

guptasu commented Nov 9, 2017

View reviewed changes

istio-testing assigned douglas-reid Nov 13, 2017

istio-testing added the lgtm label Nov 13, 2017

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

guptasu added 2 commits November 13, 2017 17:00

Add support for submessages in interface generation. Following PR wil…

3b33f57

…l contain the bootstraping code generation (code that constructs the recurrsive submessages)

Address PR comment

6f24751

guptasu force-pushed the submessages3 branch from 537cc0e to 6f24751 Compare November 14, 2017 01:00

guptasu merged commit c349b99 into istio:master Nov 14, 2017

guptasu mentioned this pull request Nov 14, 2017

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

Merged

yutongz added a commit that referenced this pull request Nov 14, 2017

Revert "Add support for submessages in template - part1. (#1617)"

7cb45f9

This reverts commit c349b99.

yutongz mentioned this pull request Nov 14, 2017

Revert "Add support for submessages in template - part1. " #1707

Merged

sebastienvas pushed a commit that referenced this pull request Nov 14, 2017

Revert "Add support for submessages in template - part1. (#1617)" (#1707

a0b5727

) This reverts commit c349b99.

guptasu mentioned this pull request Nov 15, 2017

Fix submessage support part1 #1736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for submessages in template - part1. #1617

Add support for submessages in template - part1. #1617
guptasu merged 2 commits intoistio:masterfrom
guptasu:submessages3

guptasu commented Nov 9, 2017 •

edited

Loading

Uh oh!

douglas-reid left a comment

Uh oh!

douglas-reid Nov 9, 2017

Uh oh!

douglas-reid Nov 9, 2017

Uh oh!

guptasu Nov 9, 2017

Uh oh!

guptasu Nov 9, 2017

Uh oh!

geeknoid left a comment

Uh oh!

guptasu left a comment

Uh oh!

guptasu Nov 9, 2017

Uh oh!

guptasu Nov 9, 2017

Uh oh!

yutongz commented Nov 10, 2017

Uh oh!

douglas-reid commented Nov 13, 2017

Uh oh!

istio-merge-robot commented Nov 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

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

Uh oh!

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

douglas-reid Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

douglas-reid Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

guptasu Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

guptasu Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

guptasu left a comment

Choose a reason for hiding this comment

Uh oh!

guptasu Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

guptasu Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

yutongz commented Nov 10, 2017

Uh oh!

douglas-reid commented Nov 13, 2017

Uh oh!

istio-merge-robot commented Nov 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

guptasu commented Nov 9, 2017 •

edited

Loading