-
Notifications
You must be signed in to change notification settings - Fork 655
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(docker): include globally installed puppeteer #736
fix(docker): include globally installed puppeteer #736
Conversation
I just signed my CLA, but maybe it needs some time to process. That check should be green soon I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @Eliasvdb!
@@ -14,6 +17,9 @@ RUN apt-get update \ | |||
RUN npm install -g @lhci/[email protected] | |||
RUN npm install -g lighthouse | |||
|
|||
# Install puppeteer | |||
RUN npm install -g puppeteer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to add this after the next RUN block or it won't be available
RUN cd /home/lhci/reports && npm link puppeteer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can test it out with a script in scripts/foo.js
and running docker run -v "$(pwd)/lhci-data:/home/lhci/reports/.lighthouseci" -v "$(pwd)/scripts:/home/lhci/reports/scripts" lhci-client lhci collect --puppeteerScript=scripts/foo.js --url=https://example.com
@patrickhulce I added the changes you suggested. I am unable to test it locally with your command though. When I build the docker image and run
I tried setting |
No puppeteerLaunchOptions isn't parsed as a string. I would expect |
Ah yes, that's it! Seems to work correctly then. Whenever a puppeteerscript is provided it seems puppeteer will need to be launched with both those options though. Should those be configured somewhere then instead of leaving it up to the user to specify them? What do you think? Let me know if you would like to see anything changed, I'll make sure to fix it sooner this time 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @Eliasvdb! 🎉
Nah, let's just add a doc example of how to properly invoke it 👍 CLA issues + an example in the docs and I think we're good to go 🎉 |
I'll make sure to add it!
I'm not sure what the problem is, I have signed the CLA with my GitHub username 🤔 |
I added the info to the documentation. Now I only need to figure out why my CLA is seen as missing... |
759ee52
to
404f853
Compare
@patrickhulce Found and solved the CLA issue! If there's anything else, let me know! |
Those failing tests seem unrelated to my change, does it happen more often? Seems timeout related? |
They probably are, there are quite a few flakes. |
Hey team, are we still planning on merging this ? Thanks 🙏 |
I noticed puppeteer was not included in the lhci-client docker image. I altered the
Dockerfile
to include a global npm install of puppeteer. I set the ENV varPUPPETEER_SKIP_DOWNLOAD=true
, because otherwise the puppeteer install will try to download chromium (but it will not have permission). As far as I tested this chrmium step is not needed as the docker image itself containsgoogle-chrome-stable
.