-
Notifications
You must be signed in to change notification settings - Fork 678
feat: add support for DockerfileInline #4305
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
base: main
Are you sure you want to change the base?
feat: add support for DockerfileInline #4305
Conversation
I was a bit confused about where to place this logic: writing the inline file to a temporary path and then modifying the build command to build using that file. It seems like |
@apostasie, can I get a cursory review of the approach? |
Hey @tushar5526 You do need to make sure that dockerfile and dockerfileinline cannot be specified together though: |
It looks like |
055fe8e
to
8880063
Compare
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.
Please squash and sign off the commits.
(make sure that the Signed-off-by: NAME <EMAIL>
line with your real name is included in the commit message)
8880063
to
5651e76
Compare
Done, thanks! |
CI is failing
|
5651e76
to
976b5ca
Compare
I think its good to go now. |
pkg/composer/serviceparser/build.go
Outdated
} | ||
b.BuildArgs = append(b.BuildArgs, "-f="+tmpFile.Name()) | ||
} | ||
|
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.
The tmp file has to be removed on completion of the build?
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.
Hmm, at the moment, it does not clear it up. We are creating this file at the "service parser" phase, the actual build command just gets back the modified BuildArgs
. I will take a look and see if I can clear up the tmp
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.
It seems like I can store the dockerfile_inline
value in the Build
struct, and then during the build phase, I can create the temporary file, use it for the build, and then clear it up.
type Build struct {
Force bool // force build even if already present
BuildArgs []string // {"-t", "example.com/foo", "--target", "foo", "/path/to/ctx"}
// something like follows
DockerfileInline string
// TODO: call BuildKit API directly without executing `nerdctl build`
}
Do you have any objections to putting it there?
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.
No objection
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.
Done.
199d1be
to
cd54169
Compare
As nerdctl currently uses "nerdctl build" to build the service images, write the inline file to a temporary file and use "-f" to specify the temporary dockerfile. Signed-off-by: Tushar Gupta <[email protected]>
cd54169
to
fe8ffa5
Compare
Fixes #4108
As nerdctl currently uses "nerdctl build" to build the service images, write the inline file to a temporary file and use "-f" to specify the temporary dockerfile.
TODO: Add tests if the approach looks fine.