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

misc: add official lighthouse-docker goodies #5962

Closed
wants to merge 11 commits into from
Closed

misc: add official lighthouse-docker goodies #5962

wants to merge 11 commits into from

Conversation

paulirish
Copy link
Member

Fixes #3715

AFAIK, It is NOT yet on dockerhub. (and so any readme mentions assuming that don't work yet)


This is a new PR for #4864, redone for easier conversation and review.

@googlebot

This comment has been minimized.

@paulirish
Copy link
Member Author

@devnook i've merged master back into this branch, as it was otherwise 5mo stale. I think now this branch should serve as good base for experimenting.

I've noticed some odd things already. Let's both try using it and I'm sure we'll run into some changes we want to make. Feel free to push changes right to the branch. I figure it's ~a week out from being ready to review in its entirety.

@justinribeiro
Copy link
Contributor

A few notes/things:

  1. If the target is to publish on DockerHub, then making sure that the tagging is either version specific or channel specific may be helpful (I get this request a lot).

  2. The use of dumb-init within the container shouldn't be required with any recent docker version (1.13+); you can forward/reap processes via passing the --init flag to docker run to the same effect (they merged tini a while back). See docs.

  3. The reliance on CAP_SYS_ADMIN in the docs to run the container continues to bother me; it's effectively equivalent to running root. We can't use the default seccomp profile that docker ships since Chrome uses things like unshare cap among others, but we could either include a seccomp profile we generate via strace or utilize Jess' version and make note of it in the docs.

  4. The README should cover the RHEL/CentOS user namespace issue (this came up a lot on the Rendertron repo and other Chrome-related containers).

Willing to help make updates as needed.

@ebidel
Copy link
Contributor

ebidel commented Sep 6, 2018

FWIW, Google's OS team asks that official Google containers get published to http://gcr.io/ rather than dockerhub.

@justinribeiro
Copy link
Contributor

@ebidel Would that publish be specific to public google-containers on GCR (ala https://console.cloud.google.com/gcr/images/google-containers/GLOBAL)? Hard to find unless you know where to look; how do they feel about GCR > DockerHub mirrors? ;-)

GCR has the same concept in terms of tagging in terms of versioning, so that shouldn't be problematic (if that's a feature chosen for those images in terms of release channels).

@ebidel
Copy link
Contributor

ebidel commented Sep 6, 2018

@justinribeiro I don't see anything in the guidance that suggests we can mirror to Dockerhub. I brought up the gcr.io search+discoverability issues with them in the past and they told me others had mentioned the same. I think we should be in as many places as possible :)

@@ -0,0 +1,58 @@
FROM node:8-slim
Copy link
Member Author

Choose a reason for hiding this comment

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

Much like the headful folder has everything INSIDE the image self-contained.. let's do the same with the headless case. Create a headless folder and move this and the entrypoint.sh inside of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be easier to manage over time. That change would also require the docker_build.sh to be updated as well (since the path will have changed).

&& apt-get purge --auto-remove -y curl \
&& rm -rf /src/*.deb

ADD https://github.com/Yelp/dumb-init/releases/download/v1.2.1/dumb-init_1.2.1_amd64 /usr/local/bin/dumb-init
Copy link
Contributor

Choose a reason for hiding this comment

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

Get rid of this here too.

"serve:headful": "./docker_run.sh --headful",
"chrome:version": "docker run -it --rm --cap-add=SYS_ADMIN lighthouse_docker https://example.com --fast --quiet --output=json | node -e \"let f = ''; process.stdin.on('data', d => f += d); process.stdin.on('close', () => console.log(JSON.parse(f).userAgent));\""
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline

@brendankenny
Copy link
Member

this is unmaintained right now and has no owner. Let's close and reopen when there's a sufficient need for something like this in Lighthouse core (vs existing external sources).

@brendankenny brendankenny deleted the docker branch January 30, 2019 01:26
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.

None yet

7 participants