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.

Sign up for GitHub

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

Jump to bottom

Fix(Systemd): fix execa command (issue #660) #668

Closed
wants to merge 2 commits into from
Closed

Fix(Systemd): fix execa command (issue #660) #668

wants to merge 2 commits into from

Conversation

uadp
Copy link

@uadp uadp commented Mar 11, 2018

Regarding #660 Issue

  • Add .service file extension
  • Add permission for execa command (sudo)
  • Fix mismatch error catch execa

@uadp
Fix(Systemd): fix for issue #660
38689b5 
Fix mismatch systemctl file and execa command permission
vikaspotluri123
vikaspotluri123 suggested changes Mar 11, 2018
View reviewed changes
Copy link
Member

@vikaspotluri123 vikaspotluri123 left a comment

Choose a reason for hiding this comment

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

I'm not too well-versed in using systemd so I can't say if using .service everywhere will help 🙃

extensions/systemd/systemd.js
@@ -60,12 +60,12 @@ class SystemdProcessManager extends cli.ProcessManager {

isEnabled() {
try {
execa.shellSync(`systemctl is-enabled ${this.systemdName}`);
execa.shellSync(`sudo systemctl is-enabled ${this.systemdName}`);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

extensions/systemd/systemd.js
@@ -85,12 +85,12 @@ class SystemdProcessManager extends cli.ProcessManager {

isRunning() {
try {
execa.shellSync(`systemctl is-active ${this.systemdName}`);
execa.shellSync(`sudo systemctl is-active ${this.systemdName}`);

This comment was marked as abuse.

Sign in to view

@uadp
Copy link
Author

uadp commented Mar 11, 2018

Well i'm not too well-versed with nodejs. i'm still learning anyway.

i have tried with sudo helper, but the result didn't same. (or i didn't capable to make it)
i haven't idea to use sudo helper with same return as shellAsync (Returns the same result object as child_process.spawnSync), i guess we must add some lines in process-manager.js.

But adding sudo and fixing execa error catch, it's easy way to resolve the issue 😃
and it will help new user to install ghost smoothly with recommend stack.

acburdine
acburdine reviewed Mar 12, 2018
View reviewed changes
Copy link
Member

@acburdine acburdine left a comment

Choose a reason for hiding this comment

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

@uadp can you provide some more context as to why these changes fix the issue mentioned in #660? I'm not rejecting these changes outright, but some additional information on why these changes are needed would be helpful 😄

extensions/systemd/systemd.js
@@ -60,12 +60,12 @@ class SystemdProcessManager extends cli.ProcessManager {

isEnabled() {
try {
execa.shellSync(`systemctl is-enabled ${this.systemdName}`);
execa.shellSync(`sudo systemctl is-enabled ${this.systemdName}`);

This comment was marked as abuse.

Sign in to view

@uadp
Copy link
Author

uadp commented Mar 12, 2018

@acburdine Regarding this error output

Message: 'Command failed: /bin/sh -c systemctl is-active ghost_localhost Failed to connect to bus: No such file or directory

it seem the command didn't have enough permission to run systemctl .service file.
since ghost command didn't run as root user.

It short cut to solve the issue,
solving with modification server side systemd is more complicated.

@acburdine
Copy link
Member

I'm gonna look into exactly how difficult it would be to allow isRunning to return a promise. I know in other process managers it could simplify some things.

acburdine added a commit to acburdine/Ghost-CLI that referenced this pull request Mar 13, 2018
@acburdine
feat(process-manager): allow process isRunning to return a promise
23480c2 
refs TryGhost#668 [ci skip]
- update instance.running() to handle process.isRunning promise
- update everywhere that uses instance.running to handle the promise
@acburdine acburdine mentioned this pull request Mar 13, 2018
Merged
1 task
acburdine added a commit to acburdine/Ghost-CLI that referenced this pull request Mar 13, 2018
@acburdine
feat(process-manager): allow process isRunning to return a promise
d326d66 
refs TryGhost#668
- update instance.running() to handle process.isRunning promise
- update everywhere that uses instance.running to handle the promise
- fix tests
aileen pushed a commit that referenced this pull request Mar 14, 2018
@acburdine @aileen
feat(process-manager): allow process isRunning to return a promise (#669
29b4a0b 
)

refs #668
- update instance.running() to handle process.isRunning promise
- update everywhere that uses instance.running to handle the promise
- fix tests
@aileen
Copy link
Member

aileen commented Mar 16, 2018

I submitted a PR that will change those two commands to run as sudo: #672

@acburdine
Copy link
Member

Closed by #672.

@acburdine acburdine closed this Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vikaspotluri123 vikaspotluri123 vikaspotluri123 requested changes

@acburdine acburdine acburdine left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@uadp @acburdine @aileen @vikaspotluri123