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

Add support for Include in ssh config (#70) #80

Merged
merged 5 commits into from
Oct 19, 2016
Merged

Add support for Include in ssh config (#70) #80

merged 5 commits into from
Oct 19, 2016

Conversation

edlmo
Copy link
Contributor

@edlmo edlmo commented Oct 8, 2016

This patch adds support for the Include directive in ssh_config files (issue #70).
It adds the included (readable) files to the config variable to be later processed by the _known_hosts_real() function.

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. Comments:

_included_config_files is a too generic function name; this is for a very specific purpose.

But further, because the function is used only from one place and is specific to that particular case, I think it should be inlined in the caller function instead of having it as a separate function in the environment.

Test cases for the test suite are needed for verifying the intended functionality.

@edlmo
Copy link
Contributor Author

edlmo commented Oct 11, 2016

Agree with the function name, it could be _included_ssh_config_files.

But I'm not so sure about moving that functionality inlined on _known_hosts_real. The ssh config allows to use the Include directive inside the included files... so the recursive function seems to be a good approach to me.

If you are ok with that I could change the function name and look into the test cases.

PD: thanks to you for bash-completion ;)

@scop
Copy link
Owner

scop commented Oct 14, 2016

Ah, I somehow missed that the includes and the function is recursive. In that case, agreed with the current approach.

@edlmo
Copy link
Contributor Author

edlmo commented Oct 16, 2016

I've added a test case for the ssh_config Include functionality (fixing some bugs along the way).
(passes on ./runUnit _known_hosts_real.exp)

@scop scop merged commit 1016e43 into scop:master Oct 19, 2016
@scop
Copy link
Owner

scop commented Oct 19, 2016

LGTM, thanks

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

Successfully merging this pull request may close these issues.

2 participants