Skip to content

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented May 7, 2024

Risk Level: medium
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Copy link

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: #34012 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk force-pushed the callbackmanager branch 2 times, most recently from dc70721 to 0bb23a0 Compare May 7, 2024 19:51
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk marked this pull request as ready for review May 8, 2024 12:50
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks great. I'm surprised though. I expected to see some code which previous did throw SomeException now doing return absl::SomeStatus. But I don't think I see any of those. It looks like we always return OkStatus(), which makes me think we could continue just returning void.... which makes me think I've misunderstood something in this PR. Can you correct my misunderstanding?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor Author

alyssawilk commented May 8, 2024

yeah for clarity this is expected to be no-op change.
https://github.com/envoyproxy/envoy/compare/main...alyssawilk:envoy:lb?expand=1
is a load balancer change which required status returns - I'd started to clone the callback manager and just decided to update it cleanly then rebase and then reopen.

@alyssawilk alyssawilk enabled auto-merge (squash) May 8, 2024 15:36
@alyssawilk alyssawilk merged commit c942dd1 into envoyproxy:main May 8, 2024
@alyssawilk alyssawilk deleted the callbackmanager branch June 25, 2024 14:50
sayboras added a commit to cilium/proxy that referenced this pull request Oct 31, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 13, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 13, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 13, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 13, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 14, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to cilium/proxy that referenced this pull request Nov 14, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
github-merge-queue bot pushed a commit to cilium/proxy that referenced this pull request Nov 15, 2024
Relates: envoyproxy/envoy#34708
Relates: envoyproxy/envoy#34761
Relates: envoyproxy/envoy#34437
Relates: envoyproxy/envoy#34012

Signed-off-by: Tam Mach <tam.mach@cilium.io>
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