Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add cncf/xds protos #20277

Merged
merged 23 commits into from
Mar 24, 2022
Merged

docs: add cncf/xds protos #20277

merged 23 commits into from
Mar 24, 2022

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Mar 9, 2022

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

Commit Message: Generate docs for cncf/xds and include them in Envoy documentation
Additional Description: Requires cncf/xds#32.
Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
Fixes cncf/xds#29

/assign @phlax

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
tools/protodoc/protodoc.bzl Outdated Show resolved Hide resolved
tools/protodoc/protodoc.py Outdated Show resolved Hide resolved
@phlax
Copy link
Member

phlax commented Mar 10, 2022

docs build is currently failing with

/tmp/tmpi0b_qhq4/generated/rst/api-v3/common_messages/common_messages.rst:4: WARNING: toctree contains reference to nonexisting document 'cncf/type/v3/range.proto'
/tmp/tmpi0b_qhq4/generated/rst/api-v3/common_messages/common_messages.rst:4: WARNING: toctree contains reference to nonexisting document 'cncf/type/matcher/v3/range.proto'
/tmp/tmpi0b_qhq4/generated/rst/cncf/core/v3/collection_entry.proto.rst:24: WARNING: Definition list ends without a blank line; unexpected unindent.
/tmp/tmpi0b_qhq4/generated/rst/cncf/core/v3/collection_entry.proto.rst:72: ERROR: Unknown target name: "a-za-z0-9".
/tmp/tmpi0b_qhq4/generated/rst/cncf/core/v3/context_params.proto.rst:23: ERROR: Unexpected indentation.
/tmp/tmpi0b_qhq4/generated/rst/cncf/type/v3/typed_struct.proto.rst:26: ERROR: Unexpected indentation.
/tmp/tmpi0b_qhq4/generated/rst/cncf/type/v3/typed_struct.proto.rst:27: WARNING: Block quote ends without a blank line; unexpected unindent.

when the build succeeds - it will be rendered here:

https://storage.googleapis.com/envoy-pr/20277/docs/index.html

@kyessenov
Copy link
Contributor Author

@phlax I tested locally with cncf/xds#32 and it passes. Waiting for upstream to merge it so that I can bump the dependency.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Mar 14, 2022
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @phlax

🐱

Caused by: #20277 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

kyessenov commented Mar 14, 2022

PTAL @phlax. This adds basic rendering, I'll make another pass for matching specifically in #20110 .

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20277 was synchronize by kyessenov.

see: more, trace.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov requested a review from mattklein123 as a code owner March 15, 2022 04:22
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

kyessenov commented Mar 15, 2022

API council:

There is an interesting problem with protos defined in cncf/xds but which are implemented in envoy. Envoy outputs extension documentation using the proto as canonical extension page. I think we need an alternative way, with an extension page referencing its external proto. For now, I'm going to explicitly enumerate them (it's just trie matcher) because it's hard to trigger protodoc without proto input.

@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20277 (comment) was created by @kyessenov.

see: more, trace.

@@ -20,7 +21,7 @@ envoy_cc_library(
],
)

envoy_cc_library(
envoy_cc_extension(
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need to change this just for docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's really an extension. It's main difference is that its proto is in cncf/xds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if some of these extensions in common really should have been in extensions/, but might be a bit late for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are core extensions probably so it's a bit blurry.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Contributor Author

Any more thoughts? This is needed for #20110.

@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20277 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20277 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

Not sure what the next step should be here - awaiting for review.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

/lgtm api

@phlax
Copy link
Member

phlax commented Mar 24, 2022

@kyessenov looking at the docs im noticing that its using the proto path for titles - eg xds/core/v3/authority.proto - see here https://storage.googleapis.com/envoy-pr/f3ddc9f/docs/api-v3/api.html - which is different to how the existing titles/links work

im wondering if we could/should make this more like xDS: Authority

@kyessenov
Copy link
Contributor Author

@phlax Right, that's because there is no protodoc title in those protos. We can make a better fallback, "XDS: authority.proto" I think, if you think that's better.

@phlax
Copy link
Member

phlax commented Mar 24, 2022

yep - altho for consistency i would go with xDS: Authority i think

iiuc that means we will need to update upstream - so i guess not a blocker to landing this PR

@kyessenov
Copy link
Contributor Author

Yeah we will need an upstream PR. I have a couple of changes not merged in upstream, so would prefer not block.

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

cool, i think this is a very welcome addition - lets land and iterate

/lgtm deps

@phlax phlax removed the deps Approval required for changes to Envoy's external dependencies label Mar 24, 2022
@phlax phlax merged commit c39f2b2 into envoyproxy:main Mar 24, 2022
@mattklein123
Copy link
Member

Thanks @kyessenov this is an amazing addition and will really go a long way to helping end users. Thank you so much and amazing work!

ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need documentation
6 participants