Add support for APA template. Part 1 #1882

guptasu · 2017-11-28T01:20:46Z

What this PR does / why we need it:
This PR adds the artifacts that we generate for APA templates, example

Template message
Output message
Handler interface
HandlerBuilder interface

Note:

Note the attribute_binding field generated inside the InstanceParam message. It is a map<string, string>. Operators will use this field to provide the mapping between the new attribute and the expression using the $out. style of expressions.
In APA we do not allow ValueTypes because it does not make sense for APA to request for, or return, a dynamic data.
Therefore, in APA the HandlerBuilder does not contain Set*Type methods. Instead HandlerBuilder is a simple passthrough to adapter.HandlerBuilder (which contains the Build method)

Next PR will contain the bootstraping code that will parse the operator config and generate instances, read the response from adapter and fill in the new attribute values.

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:

none

istio-testing · 2017-11-28T01:20:48Z

@guptasu: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2017-11-28T01:33:03Z

Codecov Report

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

@@            Coverage Diff            @@
##           master   #1882      +/-   ##
=========================================
- Coverage   81.72%   81.2%   -0.52%     
=========================================
  Files         200     187      -13     
  Lines       20330   18778    -1552     
=========================================
- Hits        16615   15249    -1366     
+ Misses       3251    3104     -147     
+ Partials      464     425      -39

Flag	Coverage Δ
#broker	`45.51% <ø> (+1.06%)`	⬆️
#mixer	`82.51% <ø> (-0.79%)`	⬇️
#pilot	`80.36% <ø> (-0.05%)`	⬇️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
broker/pkg/model/config/mock_store.go	`18.57% <ø> (ø)`	⬆️
mixer/adapter/prometheus/server.go	`89.83% <0%> (-5.09%)`	⬇️
pilot/platform/kube/inject/inject.go
mixer/pkg/runtime/env.go
mixer/pkg/runtime/init.go
mixer/pkg/runtime/resourceType.go
mixer/pkg/runtime/handlerTable.go
pilot/platform/kube/inject/http.go
mixer/pkg/runtime/controller.go
pilot/platform/kube/inject/initializer.go
... and 9 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 5741551...78e9169. Read the comment docs.

codecov · 2017-11-28T01:33:05Z

Codecov Report

Merging #1882 into master will increase coverage by 1.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1882      +/-   ##
==========================================
+ Coverage   81.16%   82.26%   +1.09%     
==========================================
  Files         190      194       +4     
  Lines       19430    20160     +730     
==========================================
+ Hits        15771    16584     +813     
+ Misses       3197     3113      -84     
- Partials      462      463       +1

Flag	Coverage Δ
#broker	`?`
#mixer	`83.21% <ø> (+0.78%)`	⬆️
#pilot	`80.45% <ø> (+0.01%)`	⬆️
#security	`90.39% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pilot/adapter/config/memory/monitor.go	`79.48% <0%> (-10.26%)`	⬇️
broker/pkg/controller/controller.go
broker/pkg/model/config/store.go
broker/pkg/model/config/schema.go
broker/pkg/model/osb/catalog.go
broker/pkg/version/version.go
broker/pkg/model/osb/service.go
broker/pkg/model/config/mock_store.go
broker/pkg/model/osb/servicePlan.go
mixer/pkg/runtime/init.go	`28.33% <0%> (ø)`
... and 12 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 62efa96...ae23d30. Read the comment docs.

geeknoid · 2017-11-28T13:50:55Z

LGTM

douglas-reid · 2017-11-28T16:35:35Z

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

@@ -27,6 +27,8 @@ import (
 	"github.com/gogo/protobuf/protoc-gen-gogo/descriptor"
 	"golang.org/x/tools/imports"

+	"regexp"


this should be a linter error, no?

Hmm.. that is what make fmt did for me.

I manually moved it up now.

douglas-reid · 2017-11-28T16:37:15Z

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

-	}
-	return "string"
+func trimPackageName(fullName string, pkgName string) string {
+	re := regexp.MustCompile(`(?i)` + pkgName + "\\.")


should we compile this regexp in an init() or something, instead of here (i feel like MustCompile is meant for globals)?

douglas-reid · 2017-11-28T16:41:26Z

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

 }

 func (g *Generator) getAugmentedProtoContent(model *modelgen.Model) ([]byte, error) {
 	imports := make([]string, 0)

+	var stringify func(protoType modelgen.TypeInfo) string


worth extracting signature to a non-exported type (type stringifyFn func(modelgen.TypeInfo) string) ?

douglas-reid · 2017-11-28T16:46:11Z

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

@@ -6,7 +6,7 @@ syntax = "proto3";
 /*
 Additional overview of what metric is etc..
 */
-package istio.mixer.adapter.metric;
+package istio.mixer.adapter.metricEntry;


any reason to include these changes in this PR ?

In this PR, I have fixed a bug where if the name is camelCase and it is referenced using full name inside the proto, things were not compiling in the generated code. This change tests that broken functionality. I can take it out if you think it should be its own PR.

This PR adds the artifacts that we generate for APA templates, example * Template message * Output message * Handler interface * HandlerBuilder interface Note: * Note the attribute_binding field generated inside the InstanceParam message. It is a map<string, string>. Operators will use this field to provide the mapping between the new attribute and the expression using the $out.<fieldName> style of expressions. * In APA we do not allow ValueTypes because it does not make sense for APA to request for, or return, a dynamic data. * Therefore, in APA the HandlerBuilder does not contain Set*Type methods. Instead HandlerBuilder is a simple passthrough to adapter.HandlerBuilder (which contains the Build method) Next PR will contain the bootstraping code that will parse the operator config and generate instances, read the response from adapter and fill in the new attribute values.

guptasu

addressed comments from @douglas-reid

PTAL.

guptasu · 2017-11-28T18:59:27Z

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

@@ -27,6 +27,8 @@ import (
 	"github.com/gogo/protobuf/protoc-gen-gogo/descriptor"
 	"golang.org/x/tools/imports"

+	"regexp"


Hmm.. that is what make fmt did for me.

guptasu · 2017-11-28T19:24:17Z

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

@@ -27,6 +27,8 @@ import (
 	"github.com/gogo/protobuf/protoc-gen-gogo/descriptor"
 	"golang.org/x/tools/imports"

+	"regexp"


I manually moved it up now.

guptasu · 2017-11-28T20:53:13Z

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

-	}
-	return "string"
+func trimPackageName(fullName string, pkgName string) string {
+	re := regexp.MustCompile(`(?i)` + pkgName + "\\.")


guptasu · 2017-11-28T20:55:38Z

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

 }

 func (g *Generator) getAugmentedProtoContent(model *modelgen.Model) ([]byte, error) {
 	imports := make([]string, 0)

+	var stringify func(protoType modelgen.TypeInfo) string


guptasu · 2017-11-28T20:58:14Z

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

@@ -6,7 +6,7 @@ syntax = "proto3";
 /*
 Additional overview of what metric is etc..
 */
-package istio.mixer.adapter.metric;
+package istio.mixer.adapter.metricEntry;


In this PR, I have fixed a bug where if the name is camelCase and it is referenced using full name inside the proto, things were not compiling in the generated code. This change tests that broken functionality. I can take it out if you think it should be its own PR.

guptasu · 2017-12-01T02:39:07Z

@douglas-reid @geeknoid friendly ping.

geeknoid

LGTM

douglas-reid

/lgtm

istio-merge-robot · 2017-12-01T17:29:19Z

[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.

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

~~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

istio-merge-robot · 2017-12-01T17:30:33Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2017-12-01T17:57:31Z

Automatic merge from submit-queue.

guptasu requested review from mandarjog, ozevren, douglas-reid and geeknoid November 28, 2017 01:20

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

googlebot added the cla: yes label Nov 28, 2017

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

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

douglas-reid reviewed Nov 28, 2017

View reviewed changes

ozevren approved these changes Nov 28, 2017

View reviewed changes

guptasu added 4 commits November 28, 2017 12:46

make linter happy.

9485fba

make fmt

e552b02

Address feedback.

ae23d30

guptasu force-pushed the apa3 branch from c5f14b5 to ae23d30 Compare November 28, 2017 21:03

guptasu commented Nov 28, 2017

View reviewed changes

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 28, 2017

ZackButcher mentioned this pull request Nov 29, 2017

rename svcctrl to servicecontrol #1922

Merged

istio-merge-robot added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 30, 2017

geeknoid reviewed Dec 1, 2017

View reviewed changes

douglas-reid reviewed Dec 1, 2017

View reviewed changes

istio-testing assigned douglas-reid Dec 1, 2017

istio-testing added the lgtm label Dec 1, 2017

istio-merge-robot added the approved label Dec 1, 2017

istio-merge-robot merged commit 6889c52 into istio:master Dec 1, 2017

Add support for APA template. Part 1 #1882

Add support for APA template. Part 1 #1882

Uh oh!

Conversation

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

Uh oh!

Uh oh!

istio-testing commented Nov 28, 2017

Uh oh!

codecov bot commented Nov 28, 2017

Codecov Report

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!

geeknoid commented Nov 28, 2017

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!

guptasu 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!

guptasu commented Dec 1, 2017

Uh oh!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Dec 1, 2017

Uh oh!

istio-merge-robot commented Dec 1, 2017

Uh oh!

istio-merge-robot commented Dec 1, 2017

Uh oh!

Uh oh!

guptasu commented Nov 28, 2017 •

edited

Loading

codecov bot commented Nov 28, 2017 •

edited

Loading