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.

Sign up for GitHub

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

Jump to bottom

docs: update deprecation note for operation_name #8207

Merged
merged 1 commit into from
Sep 17, 2019
Merged

docs: update deprecation note for operation_name #8207

merged 1 commit into from
Sep 17, 2019

Conversation

kyessenov
Copy link
Contributor

@kyessenov kyessenov commented Sep 11, 2019 •
edited
Loading

Signed-off-by: Kuat Yessenov [email protected]

Description: Minor clarification explaining why operation_name: INGRESS is not triggering the deprecation warnings.

Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

@kyessenov
minor note
5e78d44 
Signed-off-by: Kuat Yessenov <[email protected]>
junr03
junr03 reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

Should we trigger the deprecation warning? Is that possible? I know people rely on the warning way more reliably than reading the release notes.

@junr03 junr03 self-assigned this Sep 11, 2019
@kyessenov
Copy link
Contributor Author

We can't detect the default enum value once the config is converted to proto. So that means we might need to perform a pass at the JSON/YAML level (or hack into jsonpb). I don't know how easy it is in C++. Perhaps, API folks have seen this issue before.

@mattklein123
Copy link
Member

We can't detect the default enum value once the config is converted to proto. So that means we might need to perform a pass at the JSON/YAML level (or hack into jsonpb). I don't know how easy it is in C++. Perhaps, API folks have seen this issue before.

Can't we just look at the enum value directly and just call into the deprecation warning/stat code? cc @alyssawilk

@alyssawilk alyssawilk mentioned this pull request Sep 16, 2019
Closed
@alyssawilk
Copy link
Contributor

I think the current deprecated code looks at deprecated fields, not deprecated values.
#8253 for tracking

@mattklein123
Copy link
Member

OK let's merge this and track the improvement in the follow up issue.

@mattklein123 mattklein123 merged commit 4d19eda into envoyproxy:master Sep 17, 2019
@alyssawilk alyssawilk mentioned this pull request Sep 18, 2019
Merged
alyssawilk added a commit that referenced this pull request Sep 24, 2019
@alyssawilk
config: handling deprecated enum values (#8281)
7d8e9de 
Handling deprecated and deprecated-default enum values as part of our config checking.

Risk Level: Medium
Testing: new unit tests
Docs Changes: updated runtime docs
Release Notes: reverted #8207
Fixes #8253
Signed-off-by: Alyssa Wilk <[email protected]>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019
@kyessenov @danzh1989
docs: minor deprecation note (envoyproxy#8207)
a21abc3 
Signed-off-by: Kuat Yessenov <[email protected]>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@kyessenov
docs: minor deprecation note (envoyproxy#8207)
e08fa6a 
Signed-off-by: Kuat Yessenov <[email protected]>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@alyssawilk
config: handling deprecated enum values (envoyproxy#8281)
a6197de 
Handling deprecated and deprecated-default enum values as part of our config checking.

Risk Level: Medium
Testing: new unit tests
Docs Changes: updated runtime docs
Release Notes: reverted envoyproxy#8207
Fixes envoyproxy#8253
Signed-off-by: Alyssa Wilk <[email protected]>
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@kyessenov
docs: minor deprecation note (envoyproxy#8207)
a7ebdb3 
Signed-off-by: Kuat Yessenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junr03 junr03 junr03 left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@junr03 junr03

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kyessenov @mattklein123 @alyssawilk @junr03