-
Notifications
You must be signed in to change notification settings - Fork 54
Custom dockerfiles and build contexts #352
base: master
Are you sure you want to change the base?
Conversation
@@ -39,6 +36,7 @@ type Component struct { | |||
name string | |||
Image string | |||
Dockerfile string | |||
contextDir string |
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.
Any particular reason why this is not exposed as the rest of the fields here?
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.
it is just that I don't see why it should be exposed... Any particular reason any of these fields should be exposed ? :)
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.
At some point in the future we want to vendor out the Duffle build packages for other tools - and those tools might want to to something different with the build context.
pkg/builder/docker/builder.go
Outdated
@@ -68,10 +66,23 @@ func (dc Component) Digest() string { | |||
|
|||
// NewComponent returns a new Docker component based on the manifest | |||
func NewComponent(c *manifest.Component, cli *command.DockerCli) *Component { | |||
var ( | |||
contextDir string | |||
dockerfile = "Dockerfile" |
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.
Could we add this as a constant side by side with the Dockerignore file?
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.
we should yes
This looks great - there is one thing that might cause some confusion - consider the following component:
The config file ( |
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.
This looks great - however, there are a couple of small nits, plus the comment about paths that are relative to the application/ component directory, on which I'd like to have a discussion.
Thanks!
The rational here is:
Does it make sense ? or should we find something less confusing ? |
Regarding the relative paths - I'm ok with merging this as is, with the mention that we should document this (once we have a doc for building, that is..). So once the conversations above get resolved, I'll LGTM this and we can merge. Thanks for the contribution! |
So we're just a few small changes away from being able to merge this, right? |
This adds the possibility to specify a custom build context and Dockerfile for docker components. This makes it easier for adding duffle compatibility in existing projects. E.G.: We can now add a simple duffle.json file in the existing docker/app code base that builds the invocation image from the same Build context as before, but with a different Dockerfile Signed-off-by: Simon Ferquel <[email protected]>
rebased, fixed and pushed. PTAL @radu-matei @technosophos |
This adds the possibility to specify a custom build context and
Dockerfile for docker components. This makes it easier for adding duffle
compatibility in existing projects.
E.G.: We can now add a simple duffle.json file in the existing
docker/app code base that builds the invocation image from the same
Build context as before, but with a different Dockerfile
duffle.json at the root of my docker-app workspace: