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

health: Make the health checker extensions force-registerable #28900

Merged
merged 1 commit into from
Aug 9, 2023
Merged

health: Make the health checker extensions force-registerable #28900

merged 1 commit into from
Aug 9, 2023

Conversation

abeyad
Copy link
Contributor

@abeyad abeyad commented Aug 8, 2023

No description provided.

@abeyad abeyad enabled auto-merge (squash) August 8, 2023 16:11
@abeyad
Copy link
Contributor Author

abeyad commented Aug 8, 2023

/retest

1 similar comment
@abeyad
Copy link
Contributor Author

abeyad commented Aug 8, 2023

/retest

@abeyad
health: Make the health checker extensions force-registerable
90e060b 
Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad
Copy link
Contributor Author

abeyad commented Aug 9, 2023

/retest

RyanTheOptimist
RyanTheOptimist approved these changes Aug 9, 2023
View reviewed changes
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.

OOC, what's the context for this? Do we need this for EM, I'm guessing?

@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

Caused by: a #28900 (review) was submitted by @RyanTheOptimist.

see: more, trace.

@abeyad
Copy link
Contributor Author

abeyad commented Aug 9, 2023

OOC, what's the context for this? Do we need this for EM, I'm guessing?

Good question. We needed one of these for the Traffic Director integration test, and I added them for the other health checker factories b/c I think any extension should technically be force-registrable. Are there any reasons where we should avoid this?

@RyanTheOptimist
Copy link
Contributor

OOC, what's the context for this? Do we need this for EM, I'm guessing?

Good question. We needed one of these for the Traffic Director integration test, and I added them for the other health checker factories b/c I think any extension should technically be force-registrable. Are there any reasons where we should avoid this?

Heh, I can't think of any reason to avoid doing this... was just idly curious. Thanks for the backstory.

@abeyad abeyad mentioned this pull request Aug 9, 2023
Closed
@abeyad abeyad merged commit 8c22a9b into envoyproxy:main Aug 9, 2023
114 of 115 checks passed
@abeyad abeyad deleted the declare_factories branch August 9, 2023 23:47
jbohanon pushed a commit to solo-io/envoy-fork that referenced this pull request Aug 11, 2023
@abeyad @jbohanon
health: Make the health checker extensions force-registerable (envoyp…
d0622f8 
…roxy#28900)

Signed-off-by: Ali Beyad <abeyad@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

@zuercher zuercher Awaiting requested review from zuercher zuercher is a code owner

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@phlax phlax

@RyanTheOptimist RyanTheOptimist

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abeyad @RyanTheOptimist @phlax