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

Docker image #48

Merged
merged 10 commits into from
Sep 30, 2020
Merged

Docker image #48

merged 10 commits into from
Sep 30, 2020

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Sep 18, 2020

This requires that your system already have the docker image docker.elastic.co/beats/heartbeat:8.0.0, which at the moment must be created off of this branch. To build it cd x-pack/heartbeat && env PLATFORMS=linux/amd64 mage package.

tar -xf node.tar.gz && \
mv node-v* node
ENV PATH="/usr/share/heartbeat/.node/node/bin:$PATH"
RUN npm i -g playwright
Copy link
Member

Choose a reason for hiding this comment

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

we might have to find a way to skip firefox, webkit installation. take up memory and time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vigneshshanmugam I tried installing the playwright-chromium package, which does exactly that, but it caused weird issues, and running our stuff still asked for playwright. Something to figure out before GA for sure.

Copy link
Member

Choose a reason for hiding this comment

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

Two options i see from this thread - microsoft/playwright#823

If we are going via playwright-chromium, then we have to use the playwright-core in our code to avoid downloading multiple times.

There is also a programmatic way of downloading browsers.

const {chromium} = require('playwright');

chromium. downloadBrowserIfNeeded()

We could do this in post-install script of our library

Copy link
Member

Choose a reason for hiding this comment

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

Lets solve in separate PR.

Dockerfile Outdated
ENV PATH="/usr/share/heartbeat/.node/node/bin:$PATH"
RUN npm i -g playwright
COPY elastic-synthetics*.tgz /opt/elastic-synthetics.tgz
RUN npm install -g /opt/elastic-synthetics.tgz
Copy link
Member

Choose a reason for hiding this comment

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

any specific reason for using packed file vs directly installing our package?

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively we can copy /src into the image and install the dependencies during the docker build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried doing exactly that, which I realize is more standard, but I hit some weird error during the build IIRC, this worked reliably

Copy link
Member

Choose a reason for hiding this comment

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

Think, we have to copy /dist not source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I remember what the problem is now. Running npm i /path/to/repo doesn't actually 'install' the package, it really just links it. For some reason on the docker image this wasn't working. Doing a 'proper' pack and install made it act like other node modules.

This probably has some other advantages in that when it comes to release time I assume we'll actually run the npm packed version instead of direct from source, so this makes it more similar. That said, IMHO both methods are fine.

@apmmachine
Copy link

apmmachine commented Sep 18, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #48 updated]

  • Start Time: 2020-09-30T18:40:21.223+0000

  • Duration: 9 min 44 sec

Test stats 🧪

Test Results
Failed 0
Passed 22
Skipped 0
Total 22

USER root
RUN yum -y update && \
yum -y install epel-release && \
yum -y localinstall --nogpgcheck https://download1.rpmfusion.org/free/el/rpmfusion-free-release-7.noarch.rpm && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this for ffmpeg, but I need to see if we really need this, since it's not on the official package list playwright recommends for centos.

@@ -0,0 +1,24 @@
FROM docker.elastic.co/beats/heartbeat:8.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be changed to version 7.10.0-SNAPSHOT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's make that change on the 7.x branch when we create it

@andrewvc andrewvc marked this pull request as ready for review September 30, 2020 13:39
heartbeat.monitors:
- type: browser
id: my-monitor
name: My Monitor
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 do it separate PR as well, let's add the path which is also supported?

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Looks good

@andrewvc andrewvc merged commit 57cfa0b into elastic:master Sep 30, 2020
@andrewvc andrewvc deleted the docker branch September 30, 2020 19:13
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.

5 participants