Skip to content

feat: add support for min ready instances - #1496

Merged
enocom merged 16 commits into
mainfrom
config-readiness
Nov 8, 2022
Merged

enocom merged 16 commits into
mainfrom
config-readiness

Conversation

@enocom

@enocom enocom commented Oct 19, 2022

Copy link
Copy Markdown
Member

Fixes #1375.

@enocom
enocom requested a review from a team October 19, 2022 19:56
@enocom
enocom marked this pull request as draft October 19, 2022 20:38
@kurtisvg

Copy link
Copy Markdown
Contributor

Would this make more sense as a query parameter on the endpoint? Might be less invasive if the change is limited there as well

@enocom

enocom commented Oct 21, 2022

Copy link
Copy Markdown
Member Author

Great idea. I love it.

@hessjcg hessjcg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Needs a unit test for when min-ready is not set. Optional: the ready() logic could be simplified and made clearer.

Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck_test.go
Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck.go Outdated
@enocom
enocom requested review from hessjcg and kurtisvg October 24, 2022 18:00
Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck.go Outdated
@enocom
enocom requested a review from kurtisvg October 26, 2022 15:30

@hessjcg hessjcg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good. The logic is correct. I provided a comment on the logic in ready() I'm curious for my own learning what you think.

Comment thread internal/healthcheck/healthcheck.go Outdated
Comment thread internal/healthcheck/healthcheck_test.go
@enocom
enocom requested a review from hessjcg November 1, 2022 16:59

@kurtisvg kurtisvg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks much better.

@enocom
enocom merged commit 73e2999 into main Nov 8, 2022
@enocom
enocom deleted the config-readiness branch November 8, 2022 00:01
enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 15, 2023
enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 18, 2023
enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jan 18, 2023
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.

Readiness check supports configuring minimum number of ready instances

3 participants