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

Jump to bottom

Improve cacheability of Docker layers and add ca-certs to scratch image #2842

Merged
merged 2 commits into from
Nov 11, 2024
Merged

Improve cacheability of Docker layers and add ca-certs to scratch image #2842

merged 2 commits into from
Nov 11, 2024

Conversation

marcofranssen
Copy link
Contributor

No description provided.

@marcofranssen marcofranssen requested a review from a team as a code owner October 29, 2024 08:13
@maintainer-s-little-helper
Copy link

Commit 7f3f19e does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commit 7f3f19e does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@marcofranssen marcofranssen requested review from a team as code owners October 29, 2024 09:07
@marcofranssen marcofranssen requested a review from brlbil October 29, 2024 09:07
@maintainer-s-little-helper
Copy link

Commit 018b43f does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

marcofranssen
marcofranssen commented Oct 29, 2024
View reviewed changes
.github/workflows/images.yaml
@@ -25,8 +25,10 @@ jobs:
strategy:
matrix:
include:
- name: cilium-cli
- name: cilium-cli-ci
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way we can leverage the target in docker build. as well don't need to append -ci in various places.

@marcofranssen marcofranssen mentioned this pull request Oct 29, 2024
Open
@marcofranssen marcofranssen mentioned this pull request Oct 29, 2024
Open
joestringer
joestringer approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
derailed
derailed reviewed Nov 7, 2024
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@marcofranssen Nice work Marco! I This sounds great!
It would be nice to add here some benchmarks in terms of before/after build times and resulting image sizes.

@marcofranssen
Copy link
Contributor Author

Rebased the PR to fix the conflict on the recently bumped Go builder image. Would be great if we can merge this PR to prevent future merge conflicts and rework on my side.

@marcofranssen
Copy link
Contributor Author

marcofranssen commented Nov 11, 2024 •
edited
Loading

@marcofranssen Nice work Marco! I This sounds great! It would be nice to add here some benchmarks in terms of before/after build times and resulting image sizes.

Don't really have time to do that, as that would require me to do changes to binary and such to have a representative benchmark for both old and new implementation. But if you understand docker image layering you will understand there are fewer layers to rebuild as the ones that are most likely to change are the last layers, therefore allowing to reuse the layers before that are most likely not to change.

brlbil
brlbil approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@brlbil brlbil left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

michi-covalent
michi-covalent approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@michi-covalent michi-covalent left a comment •
edited
Loading

Choose a reason for hiding this comment

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

testing the workflow file change before merging 🚀🙏 #2861

edit: tests passed here => #2861 ✅ merging.

@michi-covalent michi-covalent merged commit 38a2044 into cilium:main Nov 11, 2024
9 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Dec 6, 2024
Merged
@github-actions github-actions bot mentioned this pull request Dec 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michi-covalent michi-covalent michi-covalent approved these changes

@brlbil brlbil brlbil approved these changes

@joestringer joestringer joestringer approved these changes

@derailed derailed Awaiting requested review from derailed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marcofranssen @derailed @joestringer @brlbil @michi-covalent