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

homeDirSafe option for startProcess() method #63

Closed
wants to merge 1 commit into from
Closed

homeDirSafe option for startProcess() method #63

wants to merge 1 commit into from

Conversation

ozgurcd
Copy link

@ozgurcd ozgurcd commented Nov 13, 2012

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

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

@vpartington
Copy link
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
Copy link
Author

ozgurcd commented Nov 14, 2012

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 <
[email protected]> 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
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