-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
allowing docker to use cached base image in build #110
allowing docker to use cached base image in build #110
Conversation
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.
Thanks! added some minor comments, but one resolved will be good to merge
plugin.go
Outdated
args = append(args, "--pull=false") | ||
} else { | ||
args = append(args, "--pull=true") | ||
} |
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 this could be condensed to a single line:
if build.Pull {
args = append(args, "--pull=true")
}
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.
also make sure to run gofmt
on your changes. This will ensure the codebase conforms to the official Go style guidelines (in this case tags for indentation)
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.
Doooh.. now I feel silly! I knew there was a way of doing this without negating the option, thanks!
plugin.go
Outdated
@@ -47,6 +47,7 @@ type ( | |||
Tags []string // Docker build tags | |||
Args []string // Docker build args | |||
Squash bool // Docker build squash | |||
Cache bool // Docker build without pulling |
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.
recommend change to Pull
main.go
Outdated
@@ -172,6 +177,7 @@ func run(c *cli.Context) error { | |||
Tags: c.StringSlice("tags"), | |||
Args: c.StringSlice("args"), | |||
Squash: c.Bool("squash"), | |||
Cache: c.Bool("cache"), |
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.
recommend change to:
c.BoolT("pull-image")
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.
also note we need to use c.BoolT
here
main.go
Outdated
cli.BoolTFlag{ | ||
Name: "cache", | ||
Usage: "don't attempt to re-build layers of the image that already exist", | ||
EnvVar: "PLUGIN_USE_CACHE", |
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.
recommend change to:
cli.BoolTFlag{
Name: "pull_image",
Usage: "pull a newer version of the image on build",
EnvVar: "PLUGIN_PULL_IMAGE",
}
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.
note that we cannot use pull
here. This is a reserved keywork in the yaml and will not get passed to the plugin. So instead we use pull_image
.gitignore
Outdated
@@ -26,3 +26,6 @@ _testmain.go | |||
|
|||
coverage.out | |||
drone-docker | |||
|
|||
# IDE/Editor related files | |||
**.swp |
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.
can you revert this file? You can add editor specific files to .git/info/exclude
I should also mention that mounting a volume should not be required, and may result in unexpected behavior and certain settings not working because you are bypassing the docker-in-docker daemon that is executing inside this plugin container. This plugin is meant to give the ability to build and publish images without compromising the host machine. So if you are mounting a docker volume, I would recommend not using this plugin and instead running docker commands directly: pipeline:
publish:
- image: plugins/docker
+ image: docker
commands:
- docker build .
- docker tag ...
- ...
volumes:
- /var/run/docker.sock:/var/run/docker.sock If you want to use the docker plugin with layer caching, the recommended solution is to use one of the drone caching plugins. These plugins give you the ability to cache files and folders and restore them at the start of your build. pipeline:
restore_cache:
image: <cache plugin>
restore: true
mount: /drone/docker
publish:
image: plugins/docker
repo: foo/bar
storage_path: /drone/docker # volume shared by all containers in the pipeline
rebuild_cache:
image: <cache plugin>
restore: true
mount: /drone/docker In the above example we make sure the docker storage path is in the shared volume so that is accessible by all steps in the pipeline. The first step (restore) will restore the docker storage path from the cache. The last step will update it. This is the recommended way to persist state between builds without having to mount a volume (although there is a cache plugin that does this too, if you want a local cache) |
fcb93cf
to
8551bcc
Compare
@bradrydzewski I've made the changes - didn't know about this gofmt tool, nice! It might be worth mentioning, I had a lot of trouble running my own version of this plugin on my own drone server, because it is hardcoded to run as privileged in the actual drone agent - it might be worth documenting this somewhere! I also came across an issue where I tried to make the |
@bradrydzewski I've been trying to use the above caching method with this plugin, and I can't seem to get it to work nicely - how can you avoid the cache growing in size with old images continually? There's no obvious way of cleaning the docker storage of everything except an image? The only obvious solution that comes to my mind is not using any drone caching plugin and instead pulling down a previous version of the image from the registry on this image's startup. This feels a little excessive for a plugin though - I may just write my own commands onto a docker-in-docker container to get the caching strategy I need. |
maybe add a
That is fine too. This is not intended to be a one-size-fits-all plugin. It works well for most people that use it, but certainly not everyone. I therefore highly encourage forking and / or creating alternate versions of plugins tailored to fit specific requirements and use cases. |
@bradrydzewski, what cache plugin should I use for |
This fixes the last comment in issue #34 - and adds a flag to allow not pulling the base image down on every build.
I'm not 100% sure that the name I've given to the option is good, but I couldn't get the EnvVar options to work with a bool flag that was true-by-default correctly :/. This being my first go code ever, I'm happy to take any suggestions/help regarding this!
example usage:
Needs to be run as trusted for the volume obviously!