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

update puppeteer for headless #1031

Merged
merged 8 commits into from
Apr 30, 2019
Merged

update puppeteer for headless #1031

merged 8 commits into from
Apr 30, 2019

Conversation

jywarren
Copy link
Member

@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #1031 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1031   +/-   ##
=======================================
  Coverage   35.58%   35.58%           
=======================================
  Files         102      102           
  Lines        1942     1942           
  Branches      297      297           
=======================================
  Hits          691      691           
  Misses       1251     1251

@vibhorgupta-gh
Copy link

@jywarren also try adding the arg '--disable-setuid-sandbox. Not certain that it'd work, but it did for some people over at this issue
Related discussion at #1032

@jywarren
Copy link
Member Author

Oh this would be great to try. Thanks @VibhorCodecianGupta. I'm a bit occupied this week with our staff retreat meetings, but if someone else wants to try this please go for it!

@@ -9,7 +9,7 @@ module.exports = function runInBrowserContext(input, callback, step, options) {

var obj = { input: input, modOptions: minOptions }

puppeteer.launch().then(function(browser) {
puppeteer.launch({headless: true, args:['--no-sandbox --disable-setuid-sandbox']}).then(function(browser) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Tried a couple new things here. Maybe we should just merge this if it passes and then see if it works upstream? @VibhorCodecianGupta @harshkhandeparkar @Divy123 ?

Choose a reason for hiding this comment

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

Sounds good. I'd test this but I do not have a GCF instance. image-sequencer-app is deployed on GCF, right @jywarren ?

Copy link
Member

Choose a reason for hiding this comment

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

@jywarren I also do not have GCF instance. Any suggestions on testing it?

Copy link
Member

Choose a reason for hiding this comment

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

We can test it on some cloud service like Google Cloud or firebase just to make sure it isn't broken

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, i just added @Divy123 and @VibhorCodecianGupta -- we're using this in Cloud Functions:

https://gist.github.com/jywarren/7721a38725affdbc4fad2598ff1be59e

@harshkhandeparkar i can add you too if you want to send me your email address.

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

More info here: publiclab/image-sequencer-app#6

Thanks, all!

Copy link

@vibhorgupta-gh vibhorgupta-gh Apr 24, 2019

Choose a reason for hiding this comment

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

@jywarren amazing, I'll try running it.

Also, I think issues #1030 #1033 #1029 are pretty urgent, I listed some possible fixes and your review is required there. Plugging in my proposal updates to your response as well (hope that's fine! :D) https://bit.ly/2VlDshL

@jywarren jywarren merged commit 43a8d0f into main Apr 30, 2019
@harshkhandeparkar
Copy link
Member

@jywarren please review #1038. I will have to rebase now?

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.

4 participants