Skip to content

Only use output struct values when actually generated#3530

Merged
istio-merge-robot merged 2 commits intoistio:masterfrom
douglas-reid:new-gen
Feb 15, 2018
Merged

Only use output struct values when actually generated#3530
istio-merge-robot merged 2 commits intoistio:masterfrom
douglas-reid:new-gen

Conversation

@douglas-reid
Copy link
Copy Markdown
Contributor

This PR is an approach to improving data quality for data generated by preprocess
adapters. The PR updates the generated code for Output structs for preprocess adapters
by adding SetXXXX() methods to the Output type that keep track of attributes that
were set. The generated code that checks for attribute values then also checks to see
if an attribute value was set.

In support of those changes, this PR adds a new expr Fn to Mixer: emptyStringMap(). This
new expr allows creation of default values for string_map expressions.

Finally, the PR updates the various config components for kubernetesenv adapters throughout
the repo.

This should eliminate the source.service == "" data issues that appeared when the new preprocessor
adapter mechanisms were put in place (as well as other zero-value related issues).

@geeknoid
Copy link
Copy Markdown
Contributor

/lgtm

@istio-merge-robot
Copy link
Copy Markdown

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

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

func(name string) (value interface{}, found bool) {
field := strings.TrimPrefix(name, fullOutName)
if len(field) != len(name) {
if len(field) != len(name) && out.WasSet(field) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make the same change to the runtime2 bindings below? Around line 547 or so I believe.

func TestExternEmptyStringMap(t *testing.T) {
m := externEmptyStringMap()
if len(m) != 0 {
t.Errorf("emptyStringMap() returned non-empty map: %#v", m)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you also add a test to pkg/il/testing/tests.go?

@istio-merge-robot
Copy link
Copy Markdown

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

@istio-merge-robot
Copy link
Copy Markdown

Automatic merge from submit-queue.

@istio-merge-robot istio-merge-robot merged commit 5abfdf1 into istio:master Feb 15, 2018
@douglas-reid
Copy link
Copy Markdown
Contributor Author

@ozevren this PR merged at light-speed. I was halfway through implementing the requested tests and BOOM! it was merged. Amazing. I'll send a follow-up PR.

I did notice something in testing however. It appears that externs must always appear in the right-most position of an expression to work as expected. To clarify, for the expression:

emptyStringMap() | source.labels

I would expect that the result would be map[string]string{} with no referenced attributes.

What I observed instead, however, was the result was whatever was specified in source.labels and source.labels was a referenced attribute.

Checking the generated IL, I saw something like:

func ... {
   call emptyStringMap
   resolve_f "source.labels"
   ret
}

Is that WAI? It seems like a potential bug.

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented Feb 16, 2018

this PR merged at light-speed.

it's the new vendor submodule :-)

@douglas-reid douglas-reid deleted the new-gen branch June 18, 2018 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants