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.

Sign up for GitHub

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

build: Mutli-stage build and slim image for websocket container #21954

Merged
merged 6 commits into from
Nov 4, 2022
Merged

build: Mutli-stage build and slim image for websocket container #21954

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
14082d2
Mutli-stage build for websocket image
Yann-J Oct 27, 2022
48e600d
Tweak npm install cmd
Yann-J Oct 27, 2022
6491631
Merge branch 'master' into websocket-slim-image
Yann-J Oct 31, 2022
e51356d
Build and publish websocket image in CI
Yann-J Oct 31, 2022
7b334ee
Use npm ci instead of npm install
Yann-J Nov 3, 2022
b2a50bb
Sync lock file
Yann-J Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/docker_build_push.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ docker build --target lean \
--label "build_actor=${GITHUB_ACTOR}" \
.

#
# Build the "websocket" image
#
docker build \
-t "${REPO_NAME}-websocket:${SHA}" \
-t "${REPO_NAME}-websocket:${REFSPEC}" \
-t "${REPO_NAME}-websocket:${LATEST_TAG}" \
--label "sha=${SHA}" \
--label "built_at=$(date)" \
--label "target=lean" \
--label "build_actor=${GITHUB_ACTOR}" \
superset-websocket

#
# Build the dev image
#
Expand Down
25 changes: 20 additions & 5 deletions superset-websocket/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,28 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
FROM node:16
FROM node:16-alpine as build

WORKDIR /home/superset-websocket

COPY . .
COPY . ./

RUN npm ci
RUN npm run build
RUN npm ci && \
npm run build

CMD ["npm", "start"]

FROM node:16-alpine

ENV NODE_ENV=production
WORKDIR /home/superset-websocket

COPY --from=build /home/superset-websocket/dist ./dist
COPY package*.json ./

# Only install production dependencies
RUN npm install --omit=dev
Copy link

Choose a reason for hiding this comment

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

Not sure this is required since you run npm ci and npm run build from build container. I think you should copy node_modules directory as well to the runtime container instead of running npm install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because one of the objectives is to avoid having the devDependencies in the production image to keep it slim

Copy link
Member

Choose a reason for hiding this comment

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

Yea, not sure about this one either. npm install can make the builds non-deterministic as it can do patch version bumps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it shouldn't because we're also committing package-lock.json (although I see it's outdated and not in sync with package.json? some people tend to think it shouldn't be committed, but that's a misconception) - but more importantly this has little to do with whether the build is single or multi-stage - versions will be resolved at build time in both cases.

Note that I didn't invent this... multi-stage builds with a prd-only npm install in the final image are a well established pattern for nodejs apps.

Copy link
Member

Choose a reason for hiding this comment

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

npm ci leverages the contents of the .lock file to make a "clean" install (https://docs.npmjs.com/cli/v8/commands/npm-ci#description)

Agree that multi-stage builds are good :). I just think that we should be doing an npm ci there, or just copy node_modules from the base

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 think I get it, you mean just use npm ci rather than npm install, but still ignoring devDependencies (which in fact seems to be the default when NODE_ENV=production)?

Yeah, that seems to make sense, as long as we agree on not dragging the devDependencies (which in the build image, would be installed by npm ci since the build image doesn't have NODE_ENV=production - which is a good thing since the build image obviously needs those). Those don't just make the image fatter, they also increase the security footprint / exposure of the image...

I've just pushed the change, and also re-synced up the lock file.


# Don't run as root!
USER node

CMD [ "npm", "start" ]