-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Run command #33
Run command #33
Changes from 5 commits
6668fa0
cefb444
cd9467d
04cca43
75e7e47
55314ce
27b09f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Copyright 2018 Google, Inc. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
FROM gcr.io/google-appengine/debian9 | ||
RUN echo "hey" > /etc/foo | ||
RUN apt-get update && apt-get install -y \ | ||
bzr \ | ||
cvs \ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Copyright 2018 Google, Inc. All rights reserved. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
# Test to make sure the executor builds an image correctly | ||
# when no files are changed | ||
|
||
FROM gcr.io/google-appengine/debian9 | ||
RUN echo "hey" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
[ | ||
{ | ||
"Image1": "gcr.io/kbuild-test/docker-test-run:latest", | ||
"Image2": "gcr.io/kbuild-test/kbuild-test-run:latest", | ||
"DiffType": "File", | ||
"Diff": { | ||
"Adds": null, | ||
"Dels": null, | ||
"Mods": [ | ||
{ | ||
"Name": "/var/log/dpkg.log", | ||
"Size1": 57425, | ||
"Size2": 57425 | ||
}, | ||
{ | ||
"Name": "/var/log/apt/term.log", | ||
"Size1": 24400, | ||
"Size2": 24400 | ||
}, | ||
{ | ||
"Name": "/var/cache/ldconfig/aux-cache", | ||
"Size1": 8057, | ||
"Size2": 8057 | ||
}, | ||
{ | ||
"Name": "/var/log/apt/history.log", | ||
"Size1": 5089, | ||
"Size2": 5089 | ||
}, | ||
{ | ||
"Name": "/var/log/alternatives.log", | ||
"Size1": 2579, | ||
"Size2": 2579 | ||
}, | ||
{ | ||
"Name": "/usr/lib/python2.7/dist-packages/keyrings/__init__.pyc", | ||
"Size1": 140, | ||
"Size2": 140 | ||
}, | ||
{ | ||
"Name": "/usr/lib/python2.7/dist-packages/lazr/__init__.pyc", | ||
"Size1": 136, | ||
"Size2": 136 | ||
} | ||
] | ||
} | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
[ | ||
{ | ||
"Image1": "gcr.io/kbuild-test/docker-test-run-2:latest", | ||
"Image2": "gcr.io/kbuild-test/kbuild-test-run-2:latest", | ||
"DiffType": "File", | ||
"Diff": { | ||
"Adds": null, | ||
"Dels": null, | ||
"Mods": null | ||
} | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
Copyright 2018 Google LLC | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package commands | ||
|
||
import ( | ||
"github.com/docker/docker/builder/dockerfile/instructions" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
type DockerCommand interface { | ||
ExecuteCommand() error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, then I guess the main loop would look like:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
// The config file has an "author" field, should return information about the command | ||
Author() string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this ever differ between commands? Or will it always be something like "kbuild"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! |
||
// A list of files to snapshot, empty for metadata commands or nil if we don't know | ||
FilesToSnapshot() []string | ||
} | ||
|
||
func GetCommand(cmd instructions.Command) DockerCommand { | ||
switch c := cmd.(type) { | ||
case *instructions.RunCommand: | ||
return &RunCommand{cmd: c} | ||
} | ||
logrus.Errorf("%s is not a supported command.", cmd.Name()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should return an error instead of logging one. |
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
Copyright 2018 Google LLC | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package commands | ||
|
||
import ( | ||
"github.com/GoogleCloudPlatform/k8s-container-builder/pkg/constants" | ||
"github.com/docker/docker/builder/dockerfile/instructions" | ||
"github.com/sirupsen/logrus" | ||
"os" | ||
"os/exec" | ||
"strings" | ||
) | ||
|
||
type RunCommand struct { | ||
cmd *instructions.RunCommand | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a TODO to make this support the "SHELL" directive? |
||
shell := []string{"/bin/sh", "-c"} | ||
newCommand = append(shell, strings.Join(r.cmd.CmdLine, " ")) | ||
} else { | ||
newCommand = r.cmd.CmdLine | ||
} | ||
|
||
logrus.Infof("cmd: %s", newCommand[0]) | ||
logrus.Infof("args: %s", newCommand[1:]) | ||
|
||
cmd := exec.Command(newCommand[0], newCommand[1:]...) | ||
cmd.Stdout = os.Stdout | ||
return cmd.Run() | ||
} | ||
|
||
// FilesToSnapshot returns nil for this command because we don't know which files | ||
// have changed, so we snapshot the entire system. | ||
func (r *RunCommand) FilesToSnapshot() []string { | ||
return nil | ||
} | ||
|
||
// Author returns some information about the command for the image config | ||
func (r *RunCommand) Author() string { | ||
author := []string{constants.Author} | ||
cmdLine := strings.Join(r.cmd.CmdLine, " ") | ||
if r.cmd.PrependShell { | ||
shell := []string{"/bin/sh", "-c"} | ||
author = append(author, shell...) | ||
return strings.Join(author, " ") + " " + cmdLine | ||
} | ||
author = append(author, cmdLine) | ||
return strings.Join(author, " ") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,4 +27,6 @@ const ( | |
WorkspaceDir = "/workspace" | ||
|
||
WhitelistPath = "/proc/self/mountinfo" | ||
|
||
Author = "kbuild" | ||
) |
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'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.