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

Fix(migrations): allow existing content/settings folder #777

Merged

Conversation

vikaspotluri123
Copy link
Member

@vikaspotluri123 vikaspotluri123 commented Aug 8, 2018

closes #776

In production sites, when the ghost user is used, the content/* folder needs to be owned by the Ghost user so the ghost application will not run into permission issues. In the migration, to make sure this happens, we run the mkdir command using sudo (i.e. sudo -E -u ghost mkdir {path}). When the Ghost user isn't needed, we use the fs-extra library to make sure the directory exists (specifically ensureDirSync). fs.ensureDirSync is equivilant to mkdir -p (not mkdir), so the behaviour differs. By adding the p flag to the mkdir command that's run when the Ghost user is used, the behaviour between the two cases are equalized, including support for a settings folder that already exists

@vikaspotluri123 vikaspotluri123 changed the title Fix(migrations): allow existing content/settings folder to exist Fix(migrations): allow existing content/settings folder Aug 8, 2018
@acburdine acburdine self-assigned this Aug 8, 2018
@coveralls
Copy link

coveralls commented Aug 8, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7634ef0 on vikaspotluri123:fix/settings-folder-sudo-permission into 29f2b01 on TryGhost:master.

@acburdine
Copy link
Member

Gonna go ahead and rebase this against the master branch - it's separate from the ghost 2.0 work so I think it's worth it to keep the eventual 1.9 PR a bit smaller :)

@acburdine acburdine changed the base branch from 1.9 to master August 8, 2018 15:10
@acburdine
Copy link
Member

....and that promptly broke things lol. @vikaspotluri123 would you mind rebasing this branch against master?

issue TryGhost#776

In production sites, when the ghost user is used, the `content/*` folder needs to be owned by the Ghost user so the ghost application will not run into permission issues. In the migration, to make sure this happens, we run the `mkdir` command using sudo (i.e. `sudo -E -u ghost mkdir {path}`). When the Ghost user isn't needed, we use the `fs-extra` library to make sure the directory exists (specifically `ensureDirSync`). `fs.ensureDirSync` is equivilant to running `mkdir` with the `p` flag, so the behaviour between sudo and no-sudo differs. By adding the `p` flag to the sudo command, the behaviour between the two cases are equalized, and the migrations won't fail if the content/settings folder exists when the Ghost user is used.
@vikaspotluri123 vikaspotluri123 force-pushed the fix/settings-folder-sudo-permission branch from b5cd11d to 7634ef0 Compare August 8, 2018 19:53
@vikaspotluri123
Copy link
Member Author

Done!

@acburdine acburdine merged commit dc2d0f5 into TryGhost:master Aug 13, 2018
@vikaspotluri123 vikaspotluri123 deleted the fix/settings-folder-sudo-permission branch August 13, 2018 03:57
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.

Protect against existing settings folder
3 participants