-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 missing upstream string formatters #33857
Add missing upstream string formatters #33857
Conversation
708d982
to
9a74a55
Compare
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.
Thank you for this contribution!
The change in source/common/formatter/stream_info_formatter.cc seems pretty large compared to what this PR should achieve. It seems to be mainly due to a format related change.
Can you make sure that the right formatting tools are used?
/wait
Do you want to backport to v1.30? If so, it is probably possible, and will require a subsequent PR to that release branch. |
Sure, I'm happy to submit the 1.30 patch |
@adisuissa I'm running |
Can you please try running it using these instructions? |
Quick check why CI failed, and I see:
Can you locally run It should fail in your local env as well and should be straightforward to debug why it happens. |
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.
Please fix formatting.
9a74a55
to
21601e2
Compare
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
21601e2
to
d3e0eb0
Compare
Looks like a flake in the udp tests? /retest envoy-presubmit |
@cpakulski @adisuissa thanks for your patience on this review; still learning the ins and outs of the devtools. Formatting and tests should be good to go now! |
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.
Formattting is still an issue in source/common/formatter/stream_info_formatter.cc.
/wait |
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
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.
Thanks!
Mostly minor comments.
/wait
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.
Looks good. Few minor comments.
Signed-off-by: Keith Mattix II <[email protected]>
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.
LGTM, thanks!
/assign-from @envoyproxy/senior-maintainers
@envoyproxy/senior-maintainers assignee is @zuercher |
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.
@keithmattix I think you should also update changelogs/current.yaml.
Does it need to be something different than what I have here? |
Ah, sorry. I missed it. All good. Thanks! |
Sorry this needs a merge. I'll try to keep a better eye on it and get it merged. |
Signed-off-by: Keith Mattix II <[email protected]>
No worries @zuercher - it should be caught up now |
/retest envoy-presubmit |
FYI we found a way around this and don't need to backport |
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback! PRs: - envoyproxy/envoy#35074 - envoyproxy/envoy#33857 - envoyproxy/envoy#33362 - envoyproxy/envoy#32961 - istio/proxy#5617 - istio/proxy#5514
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback! PRs: - envoyproxy/envoy#35074 - envoyproxy/envoy#33857 - envoyproxy/envoy#33362 - envoyproxy/envoy#32961 - istio/proxy#5617 - istio/proxy#5514
%UPSTREAM_{PEER,LOCAL}_{URI,DNS,IP}_SAN% were not implemented; add them. Given that we're not too far from when v1.30 was cut, would it be possible to backport this to 1.30?