-
Notifications
You must be signed in to change notification settings - Fork 314
[IntegTest][Release-3.14] Fix test_dcv in ADC regions by ensuring known_hosts path exists to avoid cat command failure #7085
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
Conversation
…ero exit when testing in ADC region.
|
|
||
| node_ip = cluster.get_login_node_public_ip() if use_login_node else cluster.head_node_ip | ||
|
|
||
| # add ssh key to jenkins user known hosts file to avoid ssh keychecking prompt |
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.
Why are we removing this comment. It seems we are still adding the ssh key to the known hosts file. So I think it is important to keep this information.
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.
Delete it by mistake, I have added it back.
| result = subprocess.check_output("cat {0}".format(host_keys_file), shell=True) | ||
| logging.info(f"New content of known hosts file {host_keys_file}: {result}") | ||
|
|
||
| try: |
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.
This is repeated logic that is not the most readable. I think it would be helpful to create a function like _get_known_hosts_content.
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.
Done
|
Did you run the test_dcv integration test in commercial to ensure this does not introduce any issues? If you did, I recommend adding that to the description. I would especially recommend running it with AL2023 because that was the OS that made us have to look into the known_hosts file. |
Test ongoing |
…wn_hosts path exists to avoid cat command failure (aws#7085) Ensure known_hosts path exists to avoid 'cat' command returning non-zero exit when testing in ADC region
…wn_hosts path exists to avoid cat command failure (aws#7085) Ensure known_hosts path exists to avoid 'cat' command returning non-zero exit when testing in ADC region
Description of changes
Tests
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.