Update sshprep #45
Merged
Update sshprep #45
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-Authored-By: William Stearns <3538265+william-stearns@users.noreply.github.com>
0x6d6f7468
reviewed
May 2, 2025
installer/install_scripts/sshprep
Outdated
| ip4=$(dig +short ${2} A) | ||
| ip6=$(dig +short ${2} AAAA) | ||
| ip4=$(dig +nocomment +short ${2} A 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$') | ||
| ip6=$(dig +nocomment +short ${2} AAAA 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$') |
There was a problem hiding this comment.
A few things here:
- Should we handle cases where one hostname corresponds to multiple IP addresses? There are a few cases that I'm aware of in which this could happen and it might cause commands to fail. If for some reason we needed to treat each IP address as different, we could possibly handle it by iterating over the value of
ip4andip6variables. However, I think in most cases that this could happen, we would be better off just picking one IP address. - To my knowledge, adding
| sed -e 's/;;.*//' | grep -v '^$'is made redundant by the combination of+nocommentand+shortdig arguments. Unless I'm mistaken, we could probably cut those.
There was a problem hiding this comment.
- You are correct - the Hostname field where $ipv4 and/or $ipv6 are used only supports a single address. I agree, limiting this to a single IP makes sense. Would you please add
| head -1after the grep -v '^$' on both the ipv4 and ipv6 lines and before the right parenthesis? - I thought that was true as well and that's why the first version did not have the sed and grep commands. Unfortunately I was wrong and these have to be part of the lookup.
There was a problem hiding this comment.
I've updated the file in the update-sshprep branch (to include head -1 for both the ipv4 and ipv6 variable assignments. It's not clear whether the change propagated to the PR or not.
There was a problem hiding this comment.
Thanks for making that change. For future reference, any pushes you make to a branch after PR will be associated with that PR. Makes for easy response to change requests (meaning that you did it correctly here).
Just curious, can you provide a scenario that you found during testing where having +nocomment +short still resulted in empty lines and comments (;;)? From the looks of it, +nocomment was added to the dig command at the same time as the sed and grep commands.
Thanks!
Add Bradley's suggestion of using head -1 to limit to a single address.
|
Thanks for letting me know that!
Off the top of my head I can't. I can say that the change was
specifically required because a bunch of dns-style comments were getting
stuck in places that only expected an IP address.
…On Mon, May 5, 2025 at 4:50 PM moth ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In installer/install_scripts/sshprep
<#45 (comment)>:
> @@ -230,8 +230,8 @@ check_config_block() {
prepend_config_block "\nHost ${2} ${local_ip}\n\tHostname\t\t${local_ip}\n\tHostKeyAlias\t\t${2}\n${user_line}\n"
else
status "No user-supplied target IP or hostname for $2, look one up"
- ip4=$(dig +short ${2} A)
- ip6=$(dig +short ${2} AAAA)
+ ip4=$(dig +nocomment +short ${2} A 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$')
+ ip6=$(dig +nocomment +short ${2} AAAA 2>/dev/null | sed -e 's/;;.*//' | grep -v '^$')
Thanks for making that change. For future reference, any pushes you make
to a branch after PR will be associated with that PR. Makes for easy
response to change requests.
Just curious, can you provide a scenario that you found during testing
where having +nocomment +short still resulted in empty lines and comments
(;;)? Thanks.
—
Reply to this email directly, view it on GitHub
<#45 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA272WJTBRRMAKURQ5HFOXL247FJRAVCNFSM6AAAAAB3XMR46CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQMJWGA2TOOBWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
lisaSW
approved these changes
May 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.