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

Update Dockerfile #5926

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PentesterPriyanshu
Copy link

@PentesterPriyanshu PentesterPriyanshu commented Oct 28, 2024

these changes help make the build faster by optimizing Docker's layer caching Setting TARGETARCH=amd64 as a default ensures that the build does not fai

Summary of Changes

  • Set a default architecture fallback (TARGETARCH=amd64).
  • Improved caching mechanism for Go modules by separating the go mod download step from the build process.## Background
    The previous Dockerfiles used an outdated Alpine version (3.13.5), which may lack important security updates and features. The updates aim to enhance performance, ensure compatibility with the latest dependencies, and improve overall security.

Why are the changes needed?

  • The base image has been updated to alpine:latest to ensure we are using the most recent stable version with the latest security patches and improvements.
  • Improved handling of architecture-specific builds for better robustness and maintainability.
  • Removed unnecessary commands and streamlined the installation process to make the builds faster and more efficient.

What changes were proposed in this pull request?

Summary of Changes

  • Updated base image to alpine:latest.
  • Enhanced architecture handling in the Dockerfile.
  • Cleaned up unnecessary dependencies and added missing ones (e.g., wget, bash).
  • Consolidated installation scripts for Helm and Flytectl to ensure they work seamlessly with the new Alpine version.

How was this patch tested?

Testing Environment

  • OS: Ubuntu 24
  • Docker version: 20.10.14

Setup process

  1. Built the Docker images using the updated Dockerfiles.
  2. Ran the containers to verify that all services started correctly without errors.
  3. Executed functional tests on the Flyte application to ensure everything operates as expected.

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • Documentation changes are reflected in the project docs.
  • All new and existing tests passed.
  • All commits are signed-off.

Docs link

these changes help make the build faster by optimizing Docker's layer caching
Setting TARGETARCH=amd64 as a default ensures that the build does not fai 

Signed-off-by: Priyanshu Prajapati <[email protected]>
Copy link

welcome bot commented Oct 28, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@PentesterPriyanshu
Copy link
Author

@davidmirror-ops can you check the pr and merge it

Signed-off-by: Priyanshu Prajapati <[email protected]>
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.66%. Comparing base (13b3d82) to head (18de665).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5926      +/-   ##
==========================================
- Coverage   36.84%   36.66%   -0.19%     
==========================================
  Files        1309     1241      -68     
  Lines      130967   127548    -3419     
==========================================
- Hits        48252    46761    -1491     
+ Misses      78531    76763    -1768     
+ Partials     4184     4024     -160     
Flag Coverage Δ
unittests-datacatalog ?
unittests-flyteadmin 54.12% <ø> (ø)
unittests-flytecopilot ?
unittests-flytectl 62.40% <ø> (ø)
unittests-flyteidl 6.92% <ø> (ø)
unittests-flyteplugins 53.64% <ø> (ø)
unittests-flytepropeller 43.00% <ø> (ø)
unittests-flytestdlib 55.41% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wild-endeavor
Copy link
Contributor

@eapolinario is this dockerfile still used?

Should we go through the bundled dockerfile sometime and see if the base images there need to be updated?

Comment on lines 5 to 7
# Set architecture with a default fallback
ARG TARGETARCH=amd64
ENV TARGETARCH "${TARGETARCH}"
Copy link
Member

Choose a reason for hiding this comment

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

this should not change, right?

Copy link
Contributor

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Improved caching mechanism for Go modules by separating the go mod download step from the build process.

You mention the above in the PR description, but I don't see this change in the PR. Can you clarify?

FROM --platform=${BUILDPLATFORM} mgoltzsche/podman:minimal AS builder

ARG TARGETARCH
# Set architecture with a default fallback
ARG TARGETARCH=amd64
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs mention that TARGETARCH is one of the pre-defined arguments in buildkit contexts, so no need to define this.

Copy link
Author

Choose a reason for hiding this comment

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

check it now

@@ -1,4 +1,5 @@
FROM alpine:3.13.5 AS base
FROM alpine:latest AS base
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use an explicit version, e.g. 3.20.3.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I agree with @eapolinario

Signed-off-by: Priyanshu Prajapati <[email protected]>
Signed-off-by: Priyanshu Prajapati <[email protected]>
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