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

site: use gcloud CLI to build Docker image #2479

Merged
merged 1 commit into from
Apr 22, 2019
Merged

Conversation

Conduitry
Copy link
Member

A few changes here:

  • the Makefile now uses gcloud builds submit to build the image (remotely) rather than locally with Docker and then pushing it
  • there's now a .dockerignore which whitelists only the files needed by the Sapper app
  • there's a .gcloudignore that just points to that .dockerignore (the reason for having both is so that the old docker build will continue to work nicely if so desired)
  • the Dockerfile now only copies the package[-lock].json files to the first container before installing deps, and then copies the installed packages and every (non-ignored) file in the directory to the second and final container (this is to avoid a little bit of extra copying of the app itself to the first container, which isn't really necessary)
  • installation of npm packages is now done with npm ci

@Conduitry Conduitry requested a review from lukeed April 22, 2019 06:06
@codecov-io
Copy link

codecov-io commented Apr 22, 2019

Codecov Report

Merging #2479 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2479   +/-   ##
=======================================
  Coverage   91.83%   91.83%           
=======================================
  Files           1        1           
  Lines          49       49           
=======================================
  Hits           45       45           
  Misses          4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8832f0...8866e5a. Read the comment docs.

@Conduitry
Copy link
Member Author

The upshot of all of this is:

  • docker is no longer required to be installed locally to deploy
  • stuff in e.g. __sapper__/dev is no longer getting copied into the image - __sapper__/build is the only thing in __sapper__ that's included now

/__sapper__/*
!/__sapper__/build
!/static
!/content
Copy link
Member

Choose a reason for hiding this comment

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

Could we make it this instead? I think it has same result

**
!/Dockerfile
!/package.json
!/package-lock.json
!/__sapper__/build/**
!/content
!/static

Ignore absolutely everything except these...

Copy link
Member

Choose a reason for hiding this comment

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

Built locally and it was the same

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't work

Creating temporary tarball archive of 3 file(s) totalling 235.3 KiB before compression. - only package.json, package-lock.json, and Dockerfile are included.

It's got something to do with not being able to include __sapper__/build/... if all of __sapper__ was already excluded.

When I was playing with GCR for the first time a couple of weeks ago, I spent a while wading through SO answers about gitignore trying to get this to work. I'm pretty sure there's not a tidier way to specifically include these files.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right! It works for docker alone, but gcloudignore is a bit different.

@Rich-Harris Rich-Harris merged commit 8e1fa57 into master Apr 22, 2019
@Rich-Harris Rich-Harris deleted the gcloud-builds branch April 22, 2019 10:07
@Rich-Harris
Copy link
Member

this is wonderful and i just about understand it

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