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 #1885,#1887 - install grunt as root #1889

Closed
wants to merge 1 commit into from

Conversation

@Mte90
Copy link
Contributor

commented Jun 28, 2019

fix #1885,#1887

@Mte90 Mte90 added the bug label Jun 28, 2019

@Mte90 Mte90 requested a review from tomjn Jun 28, 2019

@Mte90 Mte90 self-assigned this Jun 28, 2019

@Mte90 Mte90 requested a review from Varying-Vagrant-Vagrants/testers Jun 28, 2019

@tomjn

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

These changes mean that grunt would be install under the root home folder, not the vagrant home folder.

Since root should be able to write to the vagrant home folder, I don't believe this solves the problem at all, and introduces new ones.

It would be better to diagnose why the permissions are not working, and ensure that vagrant owns the relevant files

@tomjn

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Something like: chown -R vagrant:vagrant /usr/local/lib/node_modules

@Mte90 Mte90 force-pushed the mte90/fix-grunt branch from 0704458 to 5b0ca66 Jun 29, 2019

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

I am testing on another instance where the problem wasn't happening and where the permissions are right.

@tomjn

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@Mte90 it seems we already do a chown after the install at https://github.com/Varying-Vagrant-Vagrants/VVV/blob/develop/provision/provision.sh#L554

GitHub
An open source Vagrant configuration for developing with WordPress - Varying-Vagrant-Vagrants/VVV
@tomjn

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

I do think we should merge the update and install functions together and swap out the check for it grunt is installed, either by using a better check if grunt is in path than the one we currently use, or, by using npm list -g and parsing the result

@Mte90

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

What do you think instead of using the node-grunt-cli package from ubuntu?
So we can remove the npm step and the process will be more fast.

@Mte90 Mte90 closed this Jul 3, 2019

@Mte90 Mte90 deleted the mte90/fix-grunt branch Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.