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

Make exec agent not to use setsid for other than Linux OSes #9371

Merged
merged 7 commits into from
Apr 11, 2018
Merged

Make exec agent not to use setsid for other than Linux OSes #9371

merged 7 commits into from
Apr 11, 2018

Conversation

gazarenkov
Copy link
Contributor

What does this PR do?

Currently exec agent uses "setsid" launching new command. It does not work OOTB for other than Linux OS since there are no setsid installed by default. This PR improve it making it possible to launch exec agent on other than Linux systems not requiring to install additional software

What issues does this PR fix or reference?

n/a

Release Notes

n/a

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 10, 2018
@gazarenkov gazarenkov requested review from skabashnyuk and removed request for vparfonov April 10, 2018 16:30
@gazarenkov gazarenkov added kind/task Internal things, technical debt, and to-do tasks to be performed. team/platform and removed team/platform labels Apr 10, 2018
@skabashnyuk
Copy link
Contributor

ci-test

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9371
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@skabashnyuk
Copy link
Contributor

ci-test

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

OK for me.
Please fix file formatting before a merge.

// wrap command to be able to kill child processes see https://github.com/golang/go/issues/8854
cmd := exec.Command("setsid", shellInterpreter, "-c", newProcess.CommandLine)

var cmd *exec.Cmd
Copy link
Member

Choose a reason for hiding this comment

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

Looks like that formatting is wrong. Please execute

go fmt agents/go-agents/core/process/process.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for suggestion

@garagatyi
Copy link

ci-build

@codenvy-ci
Copy link

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Please, merge with squash.

@gazarenkov gazarenkov merged commit 33e7d7e into eclipse-che:master Apr 11, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 11, 2018
@benoitf benoitf added this to the 6.4.0 milestone Apr 11, 2018
@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:9371
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants