Feature/steps embeds a Dockerfile inside a image resource field #55

cescoferraro · 2016-11-03T20:06:07Z

Still needs to resolve env var inside the dobifile. But its working.
implements idea from #51

cescoferraro · 2016-11-03T20:07:54Z

tasks/image/build.go

@@ -106,3 +114,51 @@ func buildArgs(args map[string]string) []docker.BuildArg {
 	}
 	return out
 }
+
+func DobiDocker(dobifile []map[string]string, dockerfile string) (string, error) {


dnephin · 2016-11-04T15:54:11Z

config/image.go

@@ -35,6 +35,9 @@ type ImageConfig struct {
 	// Dockerfile The path to the ``Dockerfile`` used to build the image. This
 	// path is relative to the **context**.
 	Dockerfile string
+	// Dobifile is a in house version of a dockerfile
+	// type: mapping ``[Valid Docker command]: value``
+	Dobifile []map[string]string


I agree we need some other name for this, but I think dobifile is going to be confused with the dobi.yaml.

I'm thinking maybe content: is a better name, since it's the "content" of the Dockerfile.

dnephin · 2016-11-04T15:57:08Z

config/image.go

+		return fmt.Errorf("one of dockerfile, dobifile, context, or pull is required")
+	}
+	if c.Dockerfile != "" && len(c.Dobifile) != 0 {
+		return fmt.Errorf("Either use a dockerfile or a dobifile")


This isn't right, they could use pull: without either of these, and that would be valid.

dnephin · 2016-11-04T15:57:52Z

dobi.yaml

+      - RUN: ls
+      - RUN: mkdir {env.HOME:}
+      - RUN: env
+      - CMD: pwd


I'm thinking it might be better to make this a []string instead, so that people can copy/pate out of a Dockerfile much easier. What's the reason for making it a []map[string]string ?

I though it would be easier to validade if a key matches a existing Docker command and supply custom ones. Plus it keeps the yaml with nice colors, and a slice would still require the initial dash anyway, so it would never be that a straight foward copy/paste. Its your call

ok, let's not change it yet. I know there have been a couple other projects that have created builders with a yaml config. I'd like to look at them to see what they did. I'll link the projects here when I find them again.

dnephin · 2016-11-04T15:59:43Z

tasks/image/build.go

+	if err != nil {
+		return "", err
+	}
+	defer tempfile.Close()


hmm, the python client allows for a way to pass in a Dockerfile without it being a real file. It lets you just pass something like an io.Writer. I wonder if go-dockerclient supports that, or if we could get it added.

It would be really great to now have to write to a temp file.

Have not looked a lot into this, but I wonder the same thing.

dnephin

I like the idea of this feature.

I hadn't considered that we could make use of the dobifile templating itself for the Dockerfile, I think that's a great idea.

houndci-bot · 2016-11-04T17:38:03Z

tasks/image/build.go

+	return false
+}
+
+func (t *Task) WriteContentToBuffer(inputbuf bytes.Buffer) error{


exported method Task.WriteContentToBuffer should have comment or be unexported

btw, if you want to catch these before pushing, there are a couple options:

install http://pre-commit.com/ and run pre-commit install from the project directory. This will install pre-commit hooks for you and run static checks like this

or, run dobi lint which runs the same checks, but in a container.

I think you could also just add a pre-commit hook that ran dobi lint, I haven't tried that yet

cescoferraro

Docker itself call each line of a dockerfile Step, so our slice of Steps should be called Steps.

cescoferraro@desktop: ~/code/go/src/github.com/dnephin/dobi on feature/dobifile [!?]
$ docker build -t test .
Sending build context to Docker daemon 227.1 MB
Step 1 : FROM busybox

cescoferraro · 2016-11-04T17:20:44Z

tasks/image/build.go

+		tr.WriteHeader(&tar.Header{Name: "Dockerfile", Size: 10, ModTime: timee, AccessTime: timee, ChangeTime: timee})
+		for _, val := range t.config.Content {
+			for key, value := range val {
+				tr.Write([]byte(key + " " + value + "\n"))


There is a problem here. The bytes are too short. They are slicing FROM busybox to FROM busyb.

$ go run main.go dobi 2016/11/04 15:22:03 is content 2016/11/04 15:22:03 FROM busybox 2016/11/04 15:22:03 RUN ls 2016/11/04 15:22:03 RUN mkdir /home/cescoferraro 2016/11/04 15:22:03 RUN env 2016/11/04 15:22:03 CMD pwd Step 1 : FROM busyb Pulling repository docker.io/library/busyb [ERROR] Failed to execute task "dobi:build": Error: image library/busyb:latest not found

cescoferraro · 2016-11-04T17:27:46Z

tasks/image/build.go

+		inputbuf, _ := bytes.NewBuffer([]byte{}), bytes.NewBuffer(nil)
+		timee := time.Now()
+		tr := tar.NewWriter(inputbuf)
+		tr.WriteHeader(&tar.Header{Name: "Dockerfile", Size: 10, ModTime: timee, AccessTime: timee, ChangeTime: timee})


What is the ideal size we should? with 20 it seems to work now even with really big lines

cescoferraro · 2016-11-04T17:40:36Z

tasks/image/build.go

-				tr.Write([]byte(key + " " + value + "\n"))
-				log.Printf(key + " " + value + "\n")
-			}
+		inputbuf := bytes.NewBuffer(nil)


should probably return this from the writeContentToBuffer

cescoferraro · 2016-11-04T21:13:10Z

tasks/image/build.go

+	return inputbuf, nil
+}
+
+func (t *Task) runContainerFromDockerfile(ctx *context.ExecuteContext) error {


no the best name here. buildImageFromDockerfile sounds better

cescoferraro · 2016-11-04T21:13:39Z

tasks/image/build.go

+	return nil
+}
+
+func (t *Task) runContainerFromTarBall(ctx *context.ExecuteContext) error {


buildImageFromTar

cescoferraro · 2016-11-04T21:14:39Z

tasks/image/build.go

@@ -135,25 +104,69 @@ func buildArgs(args map[string]string) []docker.BuildArg {
 	return out
 }

-func isDobi(t *Task) (bool) {
+func (t *Task) hasContent() bool {


i dont think we need to check this t.config.Dockerfile == "Dockerfile"

cescoferraro · 2016-11-04T21:15:55Z

config/image.go

+	if len(c.Content) == 0 && c.Dockerfile == "" {
+		return fmt.Errorf("dockerfile or content have to be present")
+	}
+


I have simplified this a little bit. Have I missed anything?

Yes! A lot!

cescoferraro · 2016-11-05T10:36:46Z

config/image.go

 		c.Dockerfile = "Dockerfile"
-	case c.Context == "" && c.Dockerfile != "":
+	case c.Context == "":


This is completly useless. fsouza does it automatically. If you pass a empty string it tries to build a Dockerfile anyway.

dnephin · 2016-11-08T05:07:22Z

tasks/image/steps.go

+	"GLIDE":  "{{ .GLIDE }}",
+	"GO-GET": "{{ .GOGET }}",
+	"GO-BIN": "{{.GOBIN}}",
+}


I think it would be good to hold off on adding any custom steps for the first version of this. This can potentially grow quite a bit. We would need some way to allow users to define these without having to add patches to dobi.

dnephin · 2016-11-08T05:08:05Z

config/image.go

@@ -35,6 +35,9 @@ type ImageConfig struct {
 	// Dockerfile The path to the ``Dockerfile`` used to build the image. This
 	// path is relative to the **context**.
 	Dockerfile string
+	// Dobifile is a in house version of a dockerfile
+	// type: mapping ``[Valid Docker command]: value``
+	Steps []map[string]string


I agree, Steps sounds like a good name for this. The comment just needs to be updated.

"Steps Inline Dockerfile steps to use instead of a Dockerfile"

dnephin

Thanks, this is looking like it's on the right track.

This is already going to be a big change just using the existing steps, so I don't think we should add any custom ones yet.

dnephin · 2016-11-08T05:09:36Z

config/image.go

+			return fmt.Errorf("is pull is set, you cannot specifie a dockerfile or a content")
+		}
+	case len(c.Steps) == 0 && c.Dockerfile == "":
+		return fmt.Errorf("use either a dockerfile or set you commands in the content")


"use either a dockerfile or steps"

dnephin · 2016-11-08T05:11:06Z

config/image.go

+				return &conf, err
+			}
+		}
+	}


I'm still not sure about the []map[string]string. It's true you get syntax highlighting, but not everyone uses that for yaml. It seems like a strange structure to store what is really a []string.

We could always add syntax highlighting later by creating a custom highlighter for dobi files.

Call me crazy, but I like colors. It's your call.

Syntax highlighting is definitely a nice feature, but I think having the correct data structure is more important. We can always improve syntax highlighting later.

How are you planning to tell all editors to know about this?

dnephin · 2016-11-08T05:11:41Z

tasks/image/build.go

+	return out
+}
+
+func (t *Task) hasContent() bool {


I guess this should be hasSteps() now

dnephin · 2016-11-08T05:12:55Z

tasks/image/build.go

+	case true:
+		err = t.runContainerFromTarBall(ctx)
+	default:
+		err = t.runContainerFromDockerfile(ctx)


Ya I agree, these should be buildImageFrom...

dnephin · 2016-11-08T05:16:01Z

tasks/image/build.go

+		ModTime:    rightNow,
+		AccessTime: rightNow,
+		ChangeTime: rightNow,
+	}


I think this is the problem. The tarball needs to contain all the files in the Context directory, not just the Dockerfile.

We should try to re-use some code from docker/docker for this, so that it honors the .dockerignore file correctly.

The content size should come from the tarball size, not the size of the steps.

Can you point me to witch package 📦?

It should be in https://github.com/docker/docker/tree/master/builder

cescoferraro · 2016-11-22T15:42:20Z

This is still failing to write hidden files to the tarball

cescoferraro · 2017-04-04T00:33:38Z

@dnephin Can you take a look at this? Let me know if there is anything I improve upon.

dnephin · 2017-04-04T00:53:11Z

Yes, I'd like to get this merged, but I'm still not really liking the []map[string[string.

I'd like to use just a string, like this 8f0c5b3

I'm waiting for moby/moby#31236 to get merged, so that we can re-use a lot of the same code.

cescoferraro · 2017-04-04T00:59:35Z

Cool, I think I have already fixed this

dobi/config/image.go

Line 40 in bef9b45

Steps []string

I will take a look a the upstream PR!

dnephin · 2017-06-05T03:10:44Z

Thank you again for starting this work! This is a really cool feature.

I opened #94 which includes your commits (squashed). I updated it to use the code from docker/cli for reading a dockerfile from stdin, and reading the dockerignore file.

cescoferraro commented Nov 3, 2016

View reviewed changes

dnephin added area/config kind/feature labels Nov 3, 2016

dnephin reviewed Nov 4, 2016

View reviewed changes

cescoferraro mentioned this pull request Nov 4, 2016

Question: build container from an io.Writer fsouza/go-dockerclient#603

Closed

houndci-bot reviewed Nov 4, 2016

View reviewed changes

cescoferraro commented Nov 5, 2016

View reviewed changes

cescoferraro mentioned this pull request Nov 5, 2016

Job abstraction #58

Closed

dnephin reviewed Nov 8, 2016

View reviewed changes

dnephin added this to the 0.9 milestone Nov 9, 2016

cescoferraro mentioned this pull request Nov 10, 2016

WIP Feature/init #52

Closed

cescoferraro added 13 commits November 22, 2016 03:12

dockerfile inside dobi is called dobifile

c0efa2e

resolve variables inside dobifile

ed4a631

remove dobi file after image is build

42531df

dobifile now is called content

6254f8e

solves writing to a temporary file

58e2382

solves writing to a temporary file bug fix

22c9844

code clean up

2c2fc81

calculate content size before writing tar

59c7c00

Steps; better validation

42ca79d

custom commands

8f0b566

steps cleanup

40fa8ec

special chars bug; needs atention

bf60354

little spaces; + weird char

dc92c98

cescoferraro added 2 commits November 22, 2016 03:12

steps is a slice of string; name changes; writing context to tarball;

ad4db13

honoring dockerignore; adding context files

e070534

cescoferraro force-pushed the feature/dobifile branch from 27d7ecf to e070534 Compare November 22, 2016 05:15

cescoferraro added 2 commits November 22, 2016 03:35

test fix

5fdf448

test fix II.

f8b7bf1

cescoferraro added 2 commits November 22, 2016 13:51

handle hidden files better

0e76362

set proper file permissions

d238e59

cescoferraro changed the title ~~dockerfile inside dobi is called dobifile~~ Feature/steps embeeds a Dockerfile inside a image resource field Nov 25, 2016

cescoferraro changed the title ~~Feature/steps embeeds a Dockerfile inside a image resource field~~ Feature/steps embeds a Dockerfile inside a image resource field Nov 25, 2016

cescoferraro added 3 commits November 27, 2016 13:54

scan files concurrently

7981f91

channel of pointer bug fix

4ba2c70

cleanup

79cfc71

dnephin modified the milestones: 0.10, 0.9 Feb 1, 2017

Merge branch 'master' into feature/dobifile

bef9b45

dnephin closed this Jun 5, 2017

Feature/steps embeds a Dockerfile inside a image resource field #55

Feature/steps embeds a Dockerfile inside a image resource field #55

Conversation

cescoferraro commented Nov 3, 2016

Choose a reason for hiding this comment

dnephin Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescoferraro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescoferraro commented Nov 22, 2016

cescoferraro commented Apr 4, 2017

dnephin commented Apr 4, 2017

cescoferraro commented Apr 4, 2017

dnephin commented Jun 5, 2017

dnephin Nov 4, 2016 •

edited

Loading

dnephin Nov 4, 2016 •

edited

Loading