-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
tools: (mostly) enforcing flag alpha order #16182
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
"envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits", | ||
"envoy.reloadable_features.http2_skip_encoding_empty_trailers", | ||
"envoy.reloadable_features.upstream_http2_flood_checks", | ||
"envoy.reloadable_features.header_map_correctly_coalesce_cookies", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to fix the ordering in source/common/runtime/runtime_features.cc instead?
For instance, when I remove activate_timers_next_event_loop, this code will complain about add_and_validate_scheme_header being out of order since the real problem is that always_apply_route_header_rules was introduced out of order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but that'll screw up the issue filing for those 6 PRs. Honestly I'd prefer to revert the entire PR than do that but I'm also OK with reverting., Thoughts?
Will fix format if we think it's better than nothing :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's value in enforcing alpha checks on the reloadable flags list, so I'm ok with this PR getting merged as-is.
"envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits", | ||
"envoy.reloadable_features.http2_skip_encoding_empty_trailers", | ||
"envoy.reloadable_features.upstream_http2_flood_checks", | ||
"envoy.reloadable_features.header_map_correctly_coalesce_cookies", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's value in enforcing alpha checks on the reloadable flags list, so I'm ok with this PR getting merged as-is.
Looks like some formatting issues that python linter is complaining about. |
/wait |
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Alyssa Wilk <[email protected]> Signed-off-by: Gokul Nair <[email protected]>
Risk Level: n/a (tooling only)
Testing: manual
Docs Changes: n/a
Release Notes: n/a