feat: cache dockerfile images through warmer #2499
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
can anyone review this PR? |
| ``` | ||
|
|
||
| `--image` can be specified for any number of desired images. This command will | ||
| `--image` can be specified for any number of desired images. `--dockerfile` can | ||
| be specified for the path of dockerfile for cache.These command will combined to |
There was a problem hiding this comment.
nit: can you also add in the docs here that an optional --build-arg flag is available as well w/ this change
| @@ -157,3 +175,41 @@ func (w *Warmer) Warm(image string, opts *config.WarmerOptions) (v1.Hash, error) | |||
|
|
|||
| return digest, nil | |||
| } | |||
|
|
|||
| func ParseDockerfile(opts *config.WarmerOptions) ([]string, error) { | |||
There was a problem hiding this comment.
Can you add some unit tests around ParseDockerfile w/ tests cases such as:
- missing Dockerfile
- invalid Dockerfile
- single stage Dockerfile
- multi-stage Dockerfile
cmd/warmer/cmd/root.go
Outdated
| @@ -87,6 +95,8 @@ func addKanikoOptionsFlags() { | |||
| RootCmd.PersistentFlags().VarP(&opts.RegistriesCertificates, "registry-certificate", "", "Use the provided certificate for TLS communication with the given registry. Expected format is 'my.registry.url=/path/to/the/server/certificate'.") | |||
| RootCmd.PersistentFlags().VarP(&opts.RegistryMirrors, "registry-mirror", "", "Registry mirror to use as pull-through cache instead of docker.io. Set it repeatedly for multiple mirrors.") | |||
| RootCmd.PersistentFlags().StringVarP(&opts.CustomPlatform, "customPlatform", "", "", "Specify the build platform if different from the current host") | |||
| RootCmd.PersistentFlags().StringVarP(&opts.DockerfilePath, "dockerfile", "d", "Dockerfile", "Path to the dockerfile to be built.") | |||
| RootCmd.PersistentFlags().VarP(&opts.BuildArgs, "build-arg", "", "This flag allows you to pass in ARG values at build time for construct Base image name. Set it repeatedly for multiple values.") | |||
There was a problem hiding this comment.
nit: can you update the help text to add that this flag should be used with the dockerfile flag
cmd/warmer/cmd/root.go
Outdated
| @@ -87,6 +95,8 @@ func addKanikoOptionsFlags() { | |||
| RootCmd.PersistentFlags().VarP(&opts.RegistriesCertificates, "registry-certificate", "", "Use the provided certificate for TLS communication with the given registry. Expected format is 'my.registry.url=/path/to/the/server/certificate'.") | |||
| RootCmd.PersistentFlags().VarP(&opts.RegistryMirrors, "registry-mirror", "", "Registry mirror to use as pull-through cache instead of docker.io. Set it repeatedly for multiple mirrors.") | |||
| RootCmd.PersistentFlags().StringVarP(&opts.CustomPlatform, "customPlatform", "", "", "Specify the build platform if different from the current host") | |||
| RootCmd.PersistentFlags().StringVarP(&opts.DockerfilePath, "dockerfile", "d", "Dockerfile", "Path to the dockerfile to be built.") | |||
There was a problem hiding this comment.
nit: might make sense to add some more information in the help text here, this was my attempt:
s/Path to the dockerfile to be built./Path to the dockerfile to be cached. The kaniko warmer will parse and write out each stage's base image layers to the cache-dir. Using the same dockerfile path as what you plan to build in the kaniko executor is the expected usage
Sorry for the delay here, thanks for the PR @alexezio ! Just looked through the PR here, added some comments. Overall it's looking good, just tried it locally and the parser + cache layers worked as expected for me! |
1. Updated help text for the --build-arg flag to indicate it should be used with the dockerfile flag. 2. Updated the documentation to include the optional --build-arg flag. 3. Added unit tests for `ParseDockerfile`, covering scenarios for missing Dockerfile, invalid Dockerfile, single stage Dockerfile, multi-stage Dockerfile and Args Dockerfile
There was a problem hiding this comment.
Thank you for your time and the valuable feedback you've provided on this PR.
I've addressed all the points you raised. Specifically, I revised the help text, updated the documentation, and added unit tests covering all scenarios we discussed.
Could you please take another look at the changes I've made? @aaron-prindle
* feat: cache dockerfile images through warmer * Fix logical error in conditional statement * Addressed review feedback 1. Updated help text for the --build-arg flag to indicate it should be used with the dockerfile flag. 2. Updated the documentation to include the optional --build-arg flag. 3. Added unit tests for `ParseDockerfile`, covering scenarios for missing Dockerfile, invalid Dockerfile, single stage Dockerfile, multi-stage Dockerfile and Args Dockerfile --------- Co-authored-by: 连奔驰 <benchi.lian@thoughtworks.com>
Description
This PR introduces a new feature to cache Dockerfile images through Warmer. This feature is aimed at addressing the scenario where users expect to cache images from their Dockerfiles during CI/CD builds instead of relying on administrators to maintain the cache of base images.
Best Approach: I have considered implementing the logic to cache images in the executor, but it would require significant effort. We can implement this feature as a warming task before image building from the CI/CD perspective.
Future Work: In cases where both images and dockerfiles are specified, we can add deduplication logic.
Code Logic: When the dockerfile parameter is specified, the warmer program will read the dockerfile and parse the images to download, and will also resolve the args parameter to adapt to variable image names.
Reviewer Notes
Release Notes