Improve compatibility with jenkins docker-plugin #63
Conversation
this is more consistent with how the linux script outputs ssh activity via non-detatched sshd
even if you do want to run an arbitrary command, making environment variables available is probably good or atleast doesn't hurt. Especially if that command happens to be some uncaught variant of sshd
|
The same tests fail when I run ./make.ps1 test locally on the current master branch. Are the windows tests currently not working? |
|
Here's the output of the windows container showing the added logging changes, at the top and bottom, with rsa pubkey provided as first param Here it is catching the default docker-plugin |
|
I've been meaning to look at the tests but haven't had a chance to do so yet. |
|
Close and reopen to rebuild after merging in a pester fix |
|
I think this looks pretty good overall. I will do some testing locally as well. |
|
Do you think you could add a test around these changes? |
|
Last commit fixes linux test which supplied args quoted, while the code (and jenkins-plugin) provide it unquoted. For consistency I made it support both ways and a test for each, although the second test isn't super useful. Notably after looking at the code for docker-plugin, there are other forms of this command, the port can be changed, and additional params can get added if using the Inject Credentials setting. This code only only deals with the literal form of the default command or it would get too complicated and add risk. I considered removing the linux condition entirely, but kept it because ignoring the param sshd command and running the script sshd command ensures the sshd output goes to the docker logs (with -e flag). The linux tests pass now, the failure was an intermittent (startup time?) failure on a previously existing test. |
|
I am open to any requests for style fixes or squashing |
|
Is there anything preventing this from being merged? Conflicts? Complications? Needed discussion? |
|
If desired, I could also incorporate the change from PR #68 into my branch so that there is not a merge conflict between them. The only reason I didn't in the first place is to make this PR less complicated to review. But since it's been many months, maybe having them in a single branch to merge would make things easier? |
|
Hello, the line I think, a better solution is using the operator "=~" for regexp-matching: Here all calls with additional parameters match, but you must mask out the spaces in the (un-quoted!) search-pattern. |
|
Hm. You are saying it doesn't match at all, as in the string is not detected? My memory is fuzzy because this was back in september, I can do some testing to refresh. But I was pretty sure I had tested this many ways, including edge casey ways which were not actually likely to occur. Something which is mentioned above is that there are other arguments docker-plugin can produce if you have something other than the default settings, but trying to support all of those is too much complexity for this stop gap fix. I'm curious if the reason you have additional characters after the command is because you don't have the default configuration? Edit: also note that immediately after that condition you paste, we have to shift the arguments by a number of tokens, which is 4 unless the string arrives quoted and then it is 1. So if you have more arguments that are part of this command those would have to be counted/shifted as well, which is where I decided the diminishing returns of complexity were too much. I didn't want to this to dominate the rest of the script. Not averse to providing additional changes if it's not too much Can you provide more info on your setup? |
|
Hello, the reason for my investigation is, that the last line of the entrypoint script is not executed. I use the docker-plugin inside jenkins for starting up slave-containers (connected by ssh). Because I want to receive the extended output of ssh with the option -e, I hope on the new version of this script by one of the last pullrequests. |
Fixes issue #62