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

Add does_not_exist_at_server field to per-resource error state #14900

Merged
merged 7 commits into from
Feb 5, 2021

Conversation

lidizheng
Copy link
Contributor

Commit Message: Add does_not_exist_at_server field to per-resource error state
Additional Description:
Risk Level: N/A
Testing: N/A
Docs Changes: Added with proto definition
Release Notes: Add does_not_exist_at_server field to per-resource error state for config dump
Platform Specific Features: N/A

CC @htuch @markdroth

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14900 was opened by lidizheng.

see: more, trace.

Copy link
Contributor

@adisuissa adisuissa 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 working on this!
Added a couple of API comments.

api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
// This resource is not included in the update or being explicitly removed by
// an update, either case means that this resource no longer existed in the
// xDS server. For more information, please refer to the :ref:`"Knowing When a
// Requested Resource Does Not Exist" <xds_protocol_resource_not_existed>`
Copy link
Member

Choose a reason for hiding this comment

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

Where is this document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

@adisuissa @htuch @markdroth PTAL.

As suggested, the does not exist is added as a status enum in config_dump.proto.

api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/service/status/v3/csds.proto Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
@lidizheng
Copy link
Contributor Author

@htuch @markdroth PTALA.

markdroth
markdroth previously approved these changes Feb 3, 2021
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks great!

api/envoy/admin/v3/config_dump.proto Outdated Show resolved Hide resolved
api/envoy/service/status/v3/csds.proto Outdated Show resolved Hide resolved
@repokitteh-read-only repokitteh-read-only bot removed the api label Feb 3, 2021
htuch
htuch previously approved these changes Feb 4, 2021
Copy link
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, thanks!

@repokitteh-read-only repokitteh-read-only bot removed the api label Feb 4, 2021
@htuch
Copy link
Member

htuch commented Feb 4, 2021

@lidizheng can you fix format and get CI passing? Thanks!
@lizan can you approve as non-Googler API shepherd? Thanks!

lizan
lizan previously approved these changes Feb 4, 2021
@lidizheng lidizheng dismissed stale reviews from lizan and htuch via 590ad6a February 4, 2021 20:38
@lidizheng
Copy link
Contributor Author

@htuch Sorry for the trouble, I have fixed the format and doc generation. PTALAA.

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.

5 participants