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.

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

help request: the minimum in healthcheck schema is incorrect #8937

Closed
spacewander opened this issue Feb 24, 2023 · 10 comments
Closed

help request: the minimum in healthcheck schema is incorrect #8937

spacewander opened this issue Feb 24, 2023 · 10 comments
Assignees
Labels

Comments

@spacewander
Copy link
Member

Description

It's meaningless to use zero as the minimum:

minimum = 0,

We set them to zero because we wanted to use a default healthcheck in #5589.
However, as this PR is surpassed by #7850, there is no need to keep the zero value.

Environment

  • APISIX version (run apisix version): master
  • Operating system (run uname -a):
  • OpenResty / Nginx version (run openresty -V or nginx -V):
  • etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
  • APISIX Dashboard version, if relevant:
  • Plugin runner version, for issues related to plugin runners:
  • LuaRocks version, for installation issues (run luarocks --version):
@Neilblaze
Copy link
Contributor

Neilblaze commented Feb 24, 2023

@spacewander @navendu-pottekkat Can you assign me please if this is up for grabs?

@pottekkat
Copy link
Contributor

Sure @Neilblaze cc: @spacewander

@Neilblaze
Copy link
Contributor

Neilblaze commented Feb 24, 2023

@navendu-pottekkat I'm a bit confused about whether I'm going to break the tests by directly removing minimum = 0 or we need to set the props to a minimum of 1 or default at true?

Nonetheless, for now, I'm going with the former one i.e. I'm removing the minimum prop from successes.

@pottekkat
Copy link
Contributor

@spacewander can chime in here.

@spacewander
Copy link
Member Author

@navendu-pottekkat I'm a bit confused about whether I'm going to break the tests by directly removing minimum = 0 or we need to set the props to a minimum of 1 or default at true?

Nonetheless, for now, I'm going with the former one i.e. I'm removing the minimum prop from successes.

What do you mean 'default at true'? There is no default value in the healthcheck schema. Can you give some code as an example?

@spacewander
Copy link
Member Author

If you mean the default value in

default = 2

it is not relative to the topic here.

@Neilblaze
Copy link
Contributor

Neilblaze commented Feb 27, 2023

Gotcha! Thanks for the clarity @spacewander 😄
I'm moving forward with assigning minimum = 1 to here instead of removing minimum = 0 then.

I assume that we need to provide at least a floor to the minimum value, hence assigning 1

Copy link

This issue has been marked as stale due to 350 days of inactivity. It will be closed in 2 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 12, 2024
Copy link

This issue has been closed due to lack of activity. If you think that is incorrect, or the issue requires additional review, you can revive the issue at any time.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2024
@pottekkat
Copy link
Contributor

Closing this as it is confirmed that this is not an issue: #8949 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants