Skip to content

Support include directives in ssh_config files#1194

Closed
ghost wants to merge 4 commits intomasterfrom
unknown repository
Closed

Support include directives in ssh_config files#1194
ghost wants to merge 4 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 22, 2018

This is a followup of #872. I have squashed original author commits and added a fix for supporting relative file path in case of system ssh_config files.

self._config = []

def parse(self, file_obj):
def parse(self, file_obj, parsed_files=[]):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's a potential gotcha here - if parse() is called on two separate occasions (non-recursively), the default value the second time will retain the modifications (appends) from the first time parse() was called.

Generally, whenever you want a default value for a param, and you mutate that param, you want it like so:

def func(param=None):
    if param is None:
        param = []

@ghost
Copy link
Copy Markdown
Author

ghost commented May 22, 2018

Thanks for the review, I pushed a fix for the issue you mentioned.

@ploxiln
Copy link
Copy Markdown
Contributor

ploxiln commented May 22, 2018

Looks good to me.

(With the caveat that this won't work for Include inside a Host block - but that can be a feature for another day.)

@GhostLyrics
Copy link
Copy Markdown

GhostLyrics commented Mar 25, 2019

Could you revisit this and let us know what is still missing? Work on this has been available for about 2 years and I'd really like to use this feature in a project.

@viable-hartman
Copy link
Copy Markdown

I too would like to see this revisited and or merged. I'd add a "strip('"') function to the value part though to handle when paths like this "~/.ssh/group/*," which are valid in ssh config.

@czubik8805
Copy link
Copy Markdown

How about a merge?

@danihodovic
Copy link
Copy Markdown

How about a merge? 🙏

@oresk
Copy link
Copy Markdown

oresk commented Feb 9, 2021

How about a merge?

@bskinn
Copy link
Copy Markdown
Contributor

bskinn commented Feb 10, 2021

I can't speak directly for @bitprophet, but I'm confident this PR would be considerably strengthened by the addition of tests and documentation.

Since OP is no longer on Github, I suspect that probably kills this specific PR. If someone else interested in the feature is willing to take it up, please create a new PR with the code change, plus tests & docs. (Action on this may not be possible until after #872 is handled, however.)

@bskinn bskinn closed this Feb 10, 2021
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.

9 participants