Run command #33
Run command #33
Conversation
pkg/commands/commands.go
Outdated
| type DockerCommand interface { | ||
| ExecuteCommand() error | ||
| // The config file has an "author" field, should return information about the command | ||
| Author() string |
There was a problem hiding this comment.
Will this ever differ between commands? Or will it always be something like "kbuild"?
There was a problem hiding this comment.
I was going to start each one with "kbuild" and then put in some extra info about the command -- but I'm not sure how necessary that it, they could also all just be "kbuild". WDYT?
There was a problem hiding this comment.
Ahh, this is for the history section? I see a few different fields used in there:
{
"created": "1970-01-01T00:00:00Z",
"author": "Bazel",
"created_by": "bazel build ..."
},
{
"created": "2018-01-09T20:15:50.874240981Z",
"created_by": "/bin/sh -c #(nop) ARG DEBIAN_FRONTEND=noninteractive",
"empty_layer": true
},
{
"created": "2018-01-09T20:18:05.709512027Z",
"created_by": "|1 DEBIAN_FRONTEND=noninteractive /bin/sh -c apt-get update -y && apt-get install -y -q --no-install-recommends apt-utils autoconf build-essential ca-certificates cmake curl git file imagemagick libcurl3 libcurl3-gnutls libcurl4-openssl-dev libffi-dev libgdbm-dev libgit2-dev libgmp-dev libicu-dev libmagickwand-dev libmysqlclient-dev libncurses5-dev libpq-dev libqdbm-dev libreadline6-dev libsqlite3-dev libssl-dev libxml2-dev libxslt-dev libyaml-dev libz-dev systemtap"
},It looks like author isn't actually required, but created_by might be? The command metadata itself probably belongs in created_by, and author can be fixed as "kbuild".
WDYT?
pkg/commands/commands.go
Outdated
| case *instructions.RunCommand: | ||
| return &RunCommand{cmd: c} | ||
| } | ||
| logrus.Errorf("%s is not a supported command.", cmd.Name()) |
There was a problem hiding this comment.
This should return an error instead of logging one.
pkg/commands/run.go
Outdated
| func (r *RunCommand) ExecuteCommand() error { | ||
| var newCommand []string | ||
| if r.cmd.PrependShell { | ||
| // This is the default shell on Linux |
There was a problem hiding this comment.
Can you add a TODO to make this support the "SHELL" directive?
pkg/util/tar_util.go
Outdated
| @@ -32,19 +36,46 @@ func AddToTar(p string, i os.FileInfo, w *tar.Writer) error { | |||
| return err | |||
| } | |||
| } | |||
|
|
|||
| hardlink := false | |||
| if sys := i.Sys(); sys != nil { | |||
There was a problem hiding this comment.
Could you pull the hardlink management out to a helper function?
executor/cmd/root.go
Outdated
| if snapshotFiles == nil { | ||
| contents, err = snapshotter.TakeSnapshot() | ||
| } else { | ||
| contents, err = snapshotter.TakeSnapshotOfFiles(snapshotFiles) |
There was a problem hiding this comment.
It's a fairly common practice to have a single function for things like this that takes a nil parameter to determine whether to check all files or no files or a set of files.
pkg/commands/commands.go
Outdated
| ) | ||
|
|
||
| type DockerCommand interface { | ||
| ExecuteCommand() error |
There was a problem hiding this comment.
Do you think this will need to take a pointer to the image config eventually? for things like "ENV", etc.
There was a problem hiding this comment.
Yah I've been trying to figure out the best way to update the config, WDYT about adding an UpdateConfig() function to the interface, which would take a pointer to the config?
There was a problem hiding this comment.
Hmm, then I guess the main loop would look like:
for cmd in cmds:
cmd.UpdateConfig(&cfg)
cmd.ExecuteCommand()
How would that work for things like ENV? Would the "UpdateConfig" method update the env in the config, then ExecuteCommand would reapply that change to the command's environment?
I guess it could make sense to split it out into two methods, but it would be weird if most commands only implement one of them.
There was a problem hiding this comment.
Yah that's probably how it would work for ENV, but you're right most commands would only need one of them. Let's pass it in to ExecuteCommand() then
priyawadhwa
added
the
area/dockerfile-command
Implements the run command, with integration tests. Also includes change to AddToTar to properly save hard links in appended layers.