Skip to content

Remove the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY#17804

Closed
tyxia wants to merge 1 commit intoenvoyproxy:mainfrom
tyxia:rest_legacy
Closed

Remove the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY#17804
tyxia wants to merge 1 commit intoenvoyproxy:mainfrom
tyxia:rest_legacy

Conversation

@tyxia
Copy link
Copy Markdown
Member

@tyxia tyxia commented Aug 23, 2021

Signed-off-by: Tianyu Xia tyxia@google.com

Risk Level: Low
Testing: CI

Signed-off-by: Tianyu Xia <tyxia@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #17804 was opened by tyxia.

see: more, trace.

@tyxia tyxia marked this pull request as ready for review August 23, 2021 13:32
@tyxia tyxia marked this pull request as draft August 23, 2021 13:58
@tyxia tyxia marked this pull request as ready for review August 23, 2021 17:00
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Aug 23, 2021

/assign @htuch
PTAL, thank you!

api_config_source);
const auto transport_api_version = Utility::getAndCheckTransportVersion(api_config_source);
switch (api_config_source.api_type()) {
case envoy::config::core::v3::ApiConfigSource::hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY:
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.

Since the PGV annotations for the v3 version of this field DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE don't force rejection in some situations, should we keep this? When we get rid of generated_api_version we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name here instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the review!

I think it should be safe to remove this because default case (i.e. default: NOT_REACHED_GCOVR_EXCL_LINE;) of switch statement will catch the case where people use this field by mistake.

But i feel keeping this might provide better/richer Exception message.
I will revert this change then/

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.

Yeah, we can't rely on NOT_REACHED_GCOVR_EXCL_LINE, since this is an assertion, rather than a graceful configuration rejection which we see when we throw EnvoyException.

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.

2 participants