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

Fix #59033: salt-ssh error when targetting IPs or hostnames directly #60729

Merged
merged 6 commits into from
Dec 13, 2021

Conversation

javicacheiro
Copy link

@javicacheiro javicacheiro commented Aug 12, 2021

What does this PR do?

Fixes salt-ssh error when targetting IPs or hostnames directly

What issues does this PR fix or reference?

Fixes: #59033

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@javicacheiro javicacheiro requested a review from a team as a code owner August 12, 2021 21:43
@javicacheiro javicacheiro requested review from dhiltonp and removed request for a team August 12, 2021 21:43
@welcome
Copy link

welcome bot commented Aug 12, 2021

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@javicacheiro javicacheiro marked this pull request as draft August 13, 2021 09:27
@javicacheiro javicacheiro force-pushed the fix_salt-ssh_targetting_ips branch from 90ef3b1 to c80f6cb Compare August 13, 2021 10:12
@javicacheiro javicacheiro changed the title Fixes #59033: salt-ssh error when targetting IPs or hostnames directly Fix #59033: salt-ssh error when targetting IPs or hostnames directly Aug 17, 2021
@Ch3LL Ch3LL force-pushed the fix_salt-ssh_targetting_ips branch from 1880a39 to 15c170a Compare August 23, 2021 14:09
salt/client/ssh/__init__.py Outdated Show resolved Hide resolved
@javicacheiro javicacheiro requested a review from Ch3LL October 19, 2021 09:29
@Ch3LL
Copy link
Contributor

Ch3LL commented Oct 21, 2021

@javicacheiro can you remove this PR from draft status

@javicacheiro javicacheiro marked this pull request as ready for review October 21, 2021 20:21
@javicacheiro
Copy link
Author

Done!

@Ch3LL Ch3LL added the Phosphorus v3005.0 Release code name and version label Dec 8, 2021
Comment on lines +375 to +379
try:
roster_host = roster_data[host_id].get("host")
except AttributeError:
roster_host = roster_data[host_id]
if hostname in [host_id, roster_host]:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking) this could be written as if hostname in [host_id, roster_data.get(host_id, {}).get("host")]:

Not sure if that's easier to read or not 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

well, this does fail with the existing test suite, so maybe not :)

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Checked this code out, and ran the tests. I even swapped the code back to what I suggested and saw the same error, so...

👍 this is actually good as-is.

@Ch3LL Ch3LL merged commit 23bbc45 into saltstack:master Dec 13, 2021
@welcome
Copy link

welcome bot commented Dec 13, 2021

Congratulations on your first PR being merged! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] salt-ssh error when targetting IPs or hostnames directly
4 participants