Skip to content

Proxy-WASM ABI Part 1.#9257

Merged
mattklein123 merged 27 commits into
envoyproxy:masterfrom
jplevyak:wasm-upstream-abi
Jan 26, 2020
Merged

Proxy-WASM ABI Part 1.#9257
mattklein123 merged 27 commits into
envoyproxy:masterfrom
jplevyak:wasm-upstream-abi

Conversation

@jplevyak
Copy link
Copy Markdown
Contributor

@jplevyak jplevyak commented Dec 6, 2019

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Proxy-WASM ABI Part 1.
Risk Level: Low
Testing: N/A
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9257 was opened by jplevyak.

see: more, trace.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks for the split. I'd like to see if we can understand this with a context free Envoy developers hat on as a first step.

Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
@htuch htuch added the waiting label Dec 6, 2019
Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak
Copy link
Copy Markdown
Contributor Author

Added Doxygen comments. PTAL

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks for the Doxygen. A few followup questions.
/wait

Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
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.

Per my other comment, IMO we need a high level design doc to make sure we have a shared understanding. Thank you!

/wait

Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak
Copy link
Copy Markdown
Contributor Author

Where would be the best place for the design docs? envoy/api/wasm/v0/* or envoy/docs/root/wasm ?

@mattklein123
Copy link
Copy Markdown
Member

Where would be the best place for the design docs? envoy/api/wasm/v0/* or envoy/docs/root/wasm ?

I commented elsewhere on where I would like to see the docs (I can't remember where, in one of these PRs). Please also check your email. In the short term I really want to see a 1-2 page overview which can be in the form of a gdoc, before we merge the final docs in.

@mattklein123
Copy link
Copy Markdown
Member

Putting this into waiting until we finalize the overview doc that is floating around. That will help make reviews go much faster.

/wait

Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
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.

LGTM modulo file moves. Thank you!

/wait

Signed-off-by: John Plevyak <jplevyak@gmail.com>
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h
Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Signed-off-by: John Plevyak <jplevyak@gmail.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM (with the file move and addressing last nit thread).

Comment thread api/wasm/v0/cpp/proxy_wasm_externs.h Outdated
Signed-off-by: John Plevyak <jplevyak@gmail.com>
mattklein123
mattklein123 previously approved these changes Jan 24, 2020
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.

LGTM, thanks @htuch @lizan @yskopets any further comments?

@jplevyak jplevyak requested a review from mattklein123 January 24, 2020 21:24
Signed-off-by: John Plevyak <jplevyak@gmail.com>
htuch
htuch previously approved these changes Jan 24, 2020
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, needs format fix, thanks!

Signed-off-by: John Plevyak <jplevyak@gmail.com>
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 5a0f92c into envoyproxy:master Jan 26, 2020
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.

5 participants