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

Option for sandboxArgs; passing options to puppeteer #319

Merged
merged 1 commit into from May 4, 2020
Merged

Option for sandboxArgs; passing options to puppeteer #319

merged 1 commit into from May 4, 2020

Conversation

wondersloth
Copy link
Contributor

@wondersloth wondersloth commented Apr 28, 2020

Fixes #307

Submitting this PR for the sandboxArgs feature. In my use case I need to use a different version of chrome then what's provided with Puppeteer.

I use this to leverage the executeablePath argument.

@jsf-clabot
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Matthew Edwards seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@wondersloth wondersloth reopened this Apr 28, 2020
@vladikoff vladikoff self-requested a review April 28, 2020 17:18
@steveoh
Copy link
Contributor

steveoh commented Apr 28, 2020

I don't know enough about this to offer an opinion or a useful review. Thanks @vladikoff for offering a look.

Copy link
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

it looks good to me other than bumping the version in the package.json

@wondersloth
Copy link
Contributor Author

@steveoh would you prefer it be a patch. My thoughts were that it's a feature add, so bump minor.

@steveoh
Copy link
Contributor

steveoh commented Apr 28, 2020

I think @vladikoff's deploy pipeline handles that.

@wondersloth
Copy link
Contributor Author

Removed bump to version in package.json

@steveoh
Copy link
Contributor

steveoh commented Apr 28, 2020

Did you sign the CLA @wondersloth?

@wondersloth
Copy link
Contributor Author

Did you sign the CLA @wondersloth?

Yes, I did sign it with my personal email address, though the commit is with my work email address.

I've also per the instructions added that work email address to my github account.

@wondersloth
Copy link
Contributor Author

@steveoh Do you recommend I discard, this PR and create a new one and see if that trips the action again?

@wondersloth
Copy link
Contributor Author

Changed my commit locally to my personal email and updated the PR to see if that passes the CLA check.

@steveoh
Copy link
Contributor

steveoh commented Apr 29, 2020

I'll defer to @vladikoff on that one.

@wondersloth
Copy link
Contributor Author

@vladikoff It appears that the CLA check has passed now.

@wondersloth
Copy link
Contributor Author

Hi @vladikoff just following up if there's anything I need to do, or if there's anything you want before approving this PR. Understandable if you're busy, just wanted to follow up. Thanks.

@vladikoff
Copy link
Member

@wondersloth yea this is in inbox to look at, will get to it in the next few days. Last time i glanced the PR looked great 👍

Copy link
Contributor

@steveoh steveoh left a comment

Choose a reason for hiding this comment

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

i would probably revert the changelog and then revert all the markdown changes other than the new ones you are adding unless there is reason to modify those.

CHANGELOG Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vladikoff vladikoff merged commit f4d996b into gruntjs:master May 4, 2020
@wondersloth wondersloth deleted the feat-puppeteer-options branch May 4, 2020 19:31
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.

refactor puppeteer option passing
4 participants