Skip to content

homeDirSafe option for startProcess() method#63

Closed
ozgurcd wants to merge 1 commit into
xebialabs:masterfrom
ozgurcd:master
Closed

homeDirSafe option for startProcess() method#63
ozgurcd wants to merge 1 commit into
xebialabs:masterfrom
ozgurcd:master

Conversation

@ozgurcd

@ozgurcd ozgurcd commented Nov 13, 2012

Copy link
Copy Markdown

LDAP backed Unix hosts might not allow users to run commands if they
are not logged in to that host previously since they don't have a home
directory yet. This option, when set to true, creates a shell before
running a command and immediately closes it to trigger home directory
creation.

LDAP backed Unix hosts might not allow users to run commands if they
are not logged in to that host previously since they don't have a home
directory yet. This option, when set to true, creates a shell before
running a command and immediately closes it to trigger home directory
creation.
@buildhive

Copy link
Copy Markdown

XebiaLabs » overthere #63 SUCCESS
This pull request looks good
(what's this?)

@vpartington

Copy link
Copy Markdown
Contributor

Hi Ozgur,

Thanks for the pull request. Before I merge it, I'd like it to be implemented differently:

  1. Instead of passing a boolean to startProcess, this should be configured by using a connection property (defined on the SshConnectionBuilder class).
  2. Instead of copying the implementation of startProcess to a new method, there should be one method that checks that connection property.
  3. The special logic should only be invoked for the first process started on the remote machine.

The first change allows existing code to use the same connection method. The second change just makes the code cleaner and easier to maintain. The third one just makes it more efficient.

Can you make those changes? Alternatively you can wait until I have some time to make them. :-)

Regards, Vincent.

@ozgurcd

ozgurcd commented Nov 14, 2012

Copy link
Copy Markdown
Author

Hello Vincent,

Actually I was thinking about it too (: this is a very fast implementation
since I needed it for my work project at that time.

If I can spare some time I'll try to do it (:

Thank you,
Ozgur

On Wed, Nov 14, 2012 at 12:35 PM, Vincent Partington <
notifications@github.com> wrote:

Hi Ozgur,

Thanks for the pull request. Before I merge it, I'd like it to be
implemented differently:

  1. Instead of passing a boolean to startProcess, this should be
    configured by using a connection property (defined on the
    SshConnectionBuilder class).
  2. Instead of copying the implementation of startProcess to a new
    method, there should be one method that checks that connection property.
  3. The special logic should only be invoked for the first process
    started on the remote machine.

The first change allows existing code to use the same connection method.
The second change just makes the code cleaner and easier to maintain. The
third one just makes it more efficient.

Can you make those changes? Alternatively you can wait until I have some
time to make them. :-)

Regards, Vincent.


Reply to this email directly or view it on GitHubhttps://github.com//pull/63#issuecomment-10364590.

@vpartington

Copy link
Copy Markdown
Contributor

Hi Ozgur,

OK. If you can continue your project for now, then I'll either wait until you send me a new pull request or until I find some time to make it nice myself. No hurry. :-)

Regards, Vincent.

@hierynomus hierynomus closed this in fcdb1dc Dec 7, 2012
hierynomus added a commit that referenced this pull request Dec 10, 2012
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.

4 participants