Skip to content

fix: let oci pull be concurrent#1

Open
thomas9911 wants to merge 2 commits intomainfrom
feat/make-pull-images-concurrent
Open

fix: let oci pull be concurrent#1
thomas9911 wants to merge 2 commits intomainfrom
feat/make-pull-images-concurrent

Conversation

@thomas9911
Copy link

No description provided.

Copy link

@Aditya1404Sal Aditya1404Sal left a comment

Choose a reason for hiding this comment

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

@thomas9911 this is looking good overall, nice use of cargo-chef 👍
I left two small suggestions:

move protoc into the base chef stage so build scripts are covered in both prepare and cook
&
limit the planner stage copy to Cargo.toml files so source changes don’t invalidate the dependency cache.

Both are minor, but it should help with CI stability and caching.

concurrent pulling logic LGTM.

RUN apk --no-cache add protoc protobuf protobuf-dev
USER nonroot
WORKDIR /src

Choose a reason for hiding this comment

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

Suggested change
RUN apk --no-cache add protoc protobuf protobuf-dev

Choose a reason for hiding this comment

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

To avoid CI breakage if any dependency build script requires protoc, I think it’s safer to install it in the chef stage.

COPY . .
FROM chef AS builder

RUN apk --no-cache add protoc protobuf protobuf-dev

Choose a reason for hiding this comment

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

Suggested change
RUN apk --no-cache add protoc protobuf protobuf-dev

COPY --parents ./crates/**/Cargo.toml ./
RUN cargo fetch
FROM chef AS planner
COPY --exclude=rust-toolchain.toml . .

Choose a reason for hiding this comment

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

Suggested change
COPY --exclude=rust-toolchain.toml . .
COPY Cargo.toml Cargo.lock ./
COPY crates/**/Cargo.toml crates/**/

Choose a reason for hiding this comment

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

planner stage only needs manifests, copying source here would unnecessarily invalidate dependency cache.

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.

3 participants