Skip to content

wasm: support list lookup in get_property#13483

Merged
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
kyessenov:wasm_list_lookup
Oct 12, 2020
Merged

wasm: support list lookup in get_property#13483
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
kyessenov:wasm_list_lookup

Conversation

@kyessenov
Copy link
Copy Markdown
Contributor

Signed-off-by: Kuat Yessenov kuat@google.com

Commit Message: Add traversal by decimal index in a list
Additional Description:
Risk Level: low
Testing: unit
Docs Changes: none
Release Notes: none

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@mattklein123 mattklein123 self-assigned this Oct 10, 2020
Comment on lines +582 to +594
std::string list_value;
if (!getValue({"node", "metadata", "wasm_node_list_key", "0"}, &list_value)) {
logDebug("missing node metadata list value");
}
if (list_value != "wasm_node_get_value") {
logWarn("unexpected list value: " + list_value);
}
if (getValue({"node", "metadata", "wasm_node_list_key", "bad_key"}, &list_value)) {
logDebug("unexpected list value for a bad_key");
}
if (getValue({"node", "metadata", "wasm_node_list_key", "1"}, &list_value)) {
logDebug("unexpected list value outside the range");
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the test fail if the debug statements fail? Does the test actually verify any values? If not please make this test more robust so it actually checks things. Thank you!

/wait

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is just how these tests work. All log statements must have a mock expectation. Since I didn't add those expectations, the log calls are not hit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah OK, sounds good, thanks.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 65c2d2a into envoyproxy:master Oct 12, 2020
kyessenov added a commit to istio/envoy that referenced this pull request Oct 12, 2020
Signed-off-by: Kuat Yessenov <kuat@google.com>
istio-testing pushed a commit to istio/envoy that referenced this pull request Oct 13, 2020
Signed-off-by: Kuat Yessenov <kuat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants