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

Added parameter to ignore NPM #32

Merged
merged 9 commits into from
Nov 16, 2022
Merged

Added parameter to ignore NPM #32

merged 9 commits into from
Nov 16, 2022

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Mar 8, 2022

@Mte90 Mte90 marked this pull request as ready for review March 8, 2022 18:25
@Mte90 Mte90 requested a review from tomjn March 8, 2022 18:25
Copy link
Member

@tomjn tomjn left a comment

Choose a reason for hiding this comment

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

I wonder if we should also do some UX changes, in particular making npm failure non-blocking, so if npm fails then we can catch and report this to the user and mention the new flags so it isn't a mystery?

Maybe not this but something similar:

npm failed to run, check the site log for output in the provisioner logs folder. You may want to run npm commands on the host and disable npm during provisioning. We'll continue provisioning but some things may not work.

provision/vvv-init.sh Show resolved Hide resolved
Comment on lines 181 to 184
if [[ "${NPM}" == "true" ]]; then
try_npm_install
try_grunt_build
fi
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps separate flags for grunt and npm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Grunt packages are in the same package.json file so it isn't possible to install just them.

@Mte90
Copy link
Member Author

Mte90 commented Mar 9, 2022

I wonder if we should also do some UX changes, in particular making npm failure non-blocking, so if npm fails then we can catch and report this to the user and mention the new flags so it isn't a mystery?

I don't know if it is easy to track NPM errors or if return a specific exit code

@Mte90
Copy link
Member Author

Mte90 commented Mar 9, 2022

I see also another issue in the tests config file that is not filled with the VVV data.
I will update this PR.

Also is missing to run composer to install phpunit etc

@tomjn
Copy link
Member

tomjn commented Mar 9, 2022 via email

@tomjn
Copy link
Member

tomjn commented Mar 9, 2022 via email

@Mte90
Copy link
Member Author

Mte90 commented Mar 9, 2022

You are right, I checked now the code and I saw that grunt was executed twice in the provision.
The last commit should fix everything, I am testing it.

@Mte90 Mte90 requested a review from tomjn July 22, 2022 10:49
@Mte90
Copy link
Member Author

Mte90 commented Nov 16, 2022

@tomjn can you check now this pr?

@tomjn tomjn merged commit edb0729 into master Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants