-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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 ping_access_ip to control ping test #7020
Add ping_access_ip to control ping test #7020
Conversation
Welcome @cristiklein! |
Hi @cristiklein. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Why not do an ansible ping ? https://docs.ansible.com/ansible/latest/collections/ansible/builtin/ping_module.html |
@champtar I wasn't sure what was the original intent of the ping test. I assume people rely on it, so I wanted my change to be backwards-compatible. Doing an "ansible ping" test could be an interesting addition to the preinstalll test, but that feels like a different PR to me. |
Anyway you need to rebase master to fix the CI error you have 👍 |
b109cee
to
716d731
Compare
716d731
to
4272769
Compare
@floryut I rebased this onto master. |
4272769
to
553c775
Compare
Rebased to today's master /assign @chadswen |
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristiklein, floryut The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I do not understand why СI ended without errors |
@LuckySB what do you mean ? CI is green, or did I miss something ? |
well, access_ip is optional variable
|
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 set default value for var
and I think it is better to name the variable
ping_access_ip
with default value 'true'
In some environments, it might not be possible to ping the IP address of the nodes, e.g., because ICMP echo is blocked. This commit allows kubespray to be configured to disable the ping check, while performing all other checks.
553c775
to
4b9f28b
Compare
/lgtm |
In some environments, it might not be possible to ping the IP address of the nodes, e.g., because ICMP echo is blocked. This commit allows kubespray to be configured to disable the ping check, while performing all other checks.
What type of PR is this?
What this PR does / why we need it:
In some environments, it might not be possible to ping the IP address
of the nodes, e.g., because ICMP echo is blocked.
This commit allows kubespray to be configured to disable the ping
check, while performing all other checks.
Which issue(s) this PR fixes:
N/A
Special notes for your reviewer:
Does this PR introduce a user-facing change?: