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

In `jekyll new`, make copied site template user-writable #6072

Merged
merged 1 commit into from Jun 14, 2017

Conversation

Projects
None yet
7 participants
@league
Contributor

league commented May 9, 2017

This one-liner resolves #5932. Users on most platforms don't
experience the problem, but on NixOS ruby gems are installed in the
Nix store, where their permissions are set to 444 (r--r--r--).

Thus when jekyll new does cp_r from the site_template, the
permissions remain read-only and the end user cannot immediately edit
_config.yml and friends like the tutorials suggest.

Our packaging for NixOS patches in this change, but if you'll accept
it upstream we can eliminate that patch. It could be helpful for other
platforms where installed gems have read-only permissions.

In `jekyll new`, make copied site template user-writable
This one-liner resolves #5932. Users on most platforms don't
experience the problem, but on NixOS ruby gems are installed in the
Nix store, where their permissions are set to 444 (`r--r--r--`).

Thus when `jekyll new` does `cp_r` from the `site_template`, the
permissions remain read-only and the end user cannot immediately edit
`_config.yml` and friends like the tutorials suggest.

Our packaging for NixOS patches in this change, but if you'll accept
it upstream we can eliminate that patch. It could be helpful for other
platforms where installed gems have read-only permissions.
@oe

oe approved these changes May 12, 2017

@oe oe referenced this pull request May 12, 2017

Closed

Release Jekyll v3.5 #6074

@parkr

parkr approved these changes May 12, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 12, 2017

Member

Is there a way we can test this?

Member

parkr commented May 12, 2017

Is there a way we can test this?

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- May 12, 2017

Member

seems like the best way to test this would be with a cucumber step, yes?

Member

mattr- commented May 12, 2017

seems like the best way to test this would be with a cucumber step, yes?

@league

This comment has been minimized.

Show comment
Hide comment
@league

league May 23, 2017

Contributor

I'm really not well-versed in ruby and test frameworks, so I'm not sure I have the time to sink into figuring out and setting up tests for this one-line change. (Which is not to imply that it doesn't deserve a test, only that it's diminishing returns for me to do it.)

Contributor

league commented May 23, 2017

I'm really not well-versed in ruby and test frameworks, so I'm not sure I have the time to sink into figuring out and setting up tests for this one-line change. (Which is not to imply that it doesn't deserve a test, only that it's diminishing returns for me to do it.)

@pavel97 pavel97 referenced this pull request May 31, 2017

Closed

jekyll new: set permissions on new files #5932

5 of 17 tasks complete
@veprbl

This comment has been minimized.

Show comment
Hide comment
@veprbl

veprbl May 31, 2017

I don't know ruby too, but looking at https://github.com/jekyll/jekyll/blob/master/test/test_new_command.rb it seems like adding a check for resulting permissions should be trivial. The problem is how to produce a setup with read only source file permissions so that the test is effective.

veprbl commented May 31, 2017

I don't know ruby too, but looking at https://github.com/jekyll/jekyll/blob/master/test/test_new_command.rb it seems like adding a check for resulting permissions should be trivial. The problem is how to produce a setup with read only source file permissions so that the test is effective.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 14, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Jun 14, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 73368f8 into jekyll:master Jun 14, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Jun 14, 2017

@DirtyF DirtyF added this to the 3.5 milestone Jun 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment