support ssh config include directive#1892
support ssh config include directive#1892jsiverskog wants to merge 1 commit intoparamiko:mainfrom jsiverskog:support_config_includes
Conversation
this is heavily based on the work by Fabien Di Tore <fabien@ditore.ch>.
|
👏 |
|
Have tested this and can confirm it works. @bitprophet or maybe @ploxiln it would be great if you had time to review this PR? |
|
It would be lovely if this could be merged, a nice, tested, and documented solution for a problem quite a few of us hack around. |
|
Labeled and flagged for bitprophet review. Unfortunately, I can't provide a timeline for the review on this, as bitprophet is neck-deep in dropping Python 2 and updating to modern Sphinx for his various projects. We will get to the point of considering other feature adds like this; I just can't say when. |
| path = value | ||
| # According to SSH documentation if the path is relative, | ||
| # we look forward in ~/.ssh | ||
| if (not os.path.isabs(path)) and path[0] != '~': |
There was a problem hiding this comment.
path.startswith('~') would be more pythonic here...
| "config file: %s" % filename) | ||
| with open(filename) as fd: | ||
| parsed_files.append(filename) | ||
| self.parse(fd, parsed_files) |
There was a problem hiding this comment.
i'm trying to wrap my head around the scope of parsed_files here and it hurts a little. it seems like it's limited to the caller function, so in theory you could still create a loop where file A includes file B includes file C includes file A, and not be detected... does that make sense?
i wonder if a class variable should be used here instead, in other words. i understand it would break parsing the same file multiple times, but is that even supported?
There was a problem hiding this comment.
I think this catches a circular inclusion, but a test case would be nice and would make it clear.
| # Expand the user home path | ||
| path = os.path.expanduser(path) | ||
| if parsed_files is None: | ||
| parsed_files = [] |
There was a problem hiding this comment.
wouldn't a set() be more appropriate here?
|
|
||
| result = self.config.lookup("example.org") | ||
| assert result["hostname"] == "example.org" | ||
| assert result["user"] == "exampleuser2" |
There was a problem hiding this comment.
maybe we should test the include loops here to clarify the problem we're trying to solve with the parsed_files detection above...
|
This looks like good progress from #872, and from what I can tell, it addresses all the issues raised there. I'd still like to see more unit tests, namely for the glob pattern bug identified in #872 (comment) which is claimed to be fixed, but not demonstrated by the test suite... Having a test for the exception being correctly raised on loops would be nice too, as mentioned above. Thanks! |
|
Oh, and also this PR should close #1609. |
|
Sorry for the ping but I don't understand how has been +4 years to get this merged. This PR is ready to merge. @bskinn @bitprophet |
|
No worries, @scratchmex, thanks for checking in and expressing interest in the PR. Definitely helps to know where there's still active interest. The delay is due to several factors.
We'll get this evaluated and merged when we can. Still no guarantees on that timing, unfortunately. I'll take a close look next time I'm at-keyboard, to see if there's anything else needing doing before flagging it for bitprophet. |
I'm not sure if this is a rhetorical question or not, but I'll assume you're genuinely wondering why this is not merged. First, GitHub says, right there above the comment field, that "This branch has conflicts that must be resolved", so that needs to be fixed, normally by the submitter. Then two people reviewed this code and one (myself) found a few improvements that could be done to the code for it to be more readable. Not a blocker, but typically this is the kind of stuff you fix before merging and maintainers (at least in my case, I'm not a maintainer here, but in my projects) tend to disregard PRs with pending comments like this that don't have feedback from the submitter. So this needs some ACK or NACK from @jsiverskog at the very least. Otherwise, anyone else can also take this patch, improve on it, and submit it as a separate PR, and I'm sure the fine folks here will eventually take a look. |
|
@anarcat @jsiverskog I am missing this feature, so I can try to finish this. |
|
@mrmeszaros : of course. i don't use paramiko myself anymore. |
|
Okay, so after reading into the specs and experimenting with dumping custom configs with I am still working on it, will publish a PR in the upcoming week hopefully :) |
this is heavily based on the work by Fabien Di Tore fabien@ditore.ch.
this is based on #872 , but has been cleaned up, tested and docs added.