Skip to content

feat: add embedded web image build option#210

Open
HernandoR wants to merge 2 commits into
GoogleCloudPlatform:mainfrom
HernandoR:lz/pr/build-with-embaded-image
Open

feat: add embedded web image build option#210
HernandoR wants to merge 2 commits into
GoogleCloudPlatform:mainfrom
HernandoR:lz/pr/build-with-embaded-image

Conversation

@HernandoR
Copy link
Copy Markdown
Contributor

Fixes #<issue_number_goes_here>

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to documentation are included in the PR

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the --embed-web flag to the image build system, enabling the embedding of web client assets into the scion binary for local Docker and Podman builds. The changes include updates to the build scripts, documentation, and the scion-base Dockerfile to handle conditional frontend compilation and Go build tags. Feedback was provided to optimize the Dockerfile by separating npm dependency installation from the source code copy to improve layer caching and ensuring that frontend file changes do not unnecessarily invalidate Go build caches when the embedding feature is disabled.

Comment on lines +40 to +47
COPY web/package.json web/package-lock.json web/vite.config.ts web/tsconfig.json web/tsconfig.client.json web/index.html ./web/
COPY web/scripts/ ./web/scripts/
COPY web/public/ ./web/public/
COPY web/src/ ./web/src/

RUN if [ "${EMBED_WEB}" = "true" ]; then \
cd web && npm ci && npm run build; \
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current structure of the web asset build process has two significant efficiency issues:

  1. Layer Caching for Dependencies: By copying all web source files (including src/) before running npm ci, any change to a source file will invalidate the cache for the dependency installation step. npm ci is typically slow and should only be re-run when package.json or package-lock.json changes.
  2. Go Build Invalidation: Because the Go build step (line 52) follows the web asset COPY and RUN commands, any change to a frontend file (e.g., a CSS or TS file) will invalidate the cache for the Go build, even if EMBED_WEB is set to false. This causes unnecessary full rebuilds of the scion and sciontool binaries during local development.

To optimize this, split the dependency installation from the build step. For a more robust fix to the Go build invalidation, consider using a separate build stage for the web assets and copying the results back into the main builder stage only if EMBED_WEB is true.

COPY web/package.json web/package-lock.json ./web/
RUN if [ "${EMBED_WEB}" = "true" ]; then \
        cd web && npm ci; \
    fi

COPY web/vite.config.ts web/tsconfig.json web/tsconfig.client.json web/index.html ./web/
COPY web/scripts/ ./web/scripts/
COPY web/public/ ./web/public/
COPY web/src/ ./web/src/

RUN if [ "${EMBED_WEB}" = "true" ]; then \
        cd web && npm run build; \
    fi

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.

1 participant