-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix: cargo install ignoring Cargo.lock #1047
fix: cargo install ignoring Cargo.lock #1047
Conversation
Signed-off-by: Fabrizio Sestito <[email protected]>
@@ -14,7 +14,7 @@ WORKDIR /usr/src/policy-server | |||
COPY ./ ./ | |||
|
|||
RUN cargo install cargo-auditable | |||
RUN cargo auditable install --target aarch64-unknown-linux-musl --path . | |||
RUN cargo auditable install --locked --target aarch64-unknown-linux-musl --path . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to run build --release
instead of install
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is fine to do that. We just need to copy the binary later, in the final image, from a different place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine replacing the install
with build --release
as long as this doesn't make the Dockerfile more complicated (this is a multi-arch image, the build artifact would be located under two different paths, based on the architecture)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
=======================================
Coverage 63.58% 63.58%
=======================================
Files 17 17
Lines 1071 1071
=======================================
Hits 681 681
Misses 390 390
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'm fine with the current PR, but I'm also open to use the build --release
command as stated on my comment
@@ -14,7 +14,7 @@ WORKDIR /usr/src/policy-server | |||
COPY ./ ./ | |||
|
|||
RUN cargo install cargo-auditable | |||
RUN cargo auditable install --target aarch64-unknown-linux-musl --path . | |||
RUN cargo auditable install --locked --target aarch64-unknown-linux-musl --path . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine replacing the install
with build --release
as long as this doesn't make the Dockerfile more complicated (this is a multi-arch image, the build artifact would be located under two different paths, based on the architecture)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch indeed!
Description
By default, the
Cargo.lock
file that is included with the package will be ignored bycargo install
.Since the Dockerfile uses
cargo auditable install
, there were scenarios where the build regeneratedCargo.lock
with updated dependencies.As a result, the CI passed while the Docker build failed (see [GitHub Actions run](https://github.com/kubewarden/policy-server/actions/runs/12902609277/job/35976498867)).
This PR updates the Dockerfile to add
--locked
to thecargo auditable install
instruction, fixing the issue.