[New Feature] Ability to build dockerfile located inside sub-directories #102

dduportal · 2018-05-10T18:29:27Z

Hello dear maintainers! Thank you for this piece of software.

This Pull Request aims at implementing #83 .

It introduces the following changes:

The keyword context can be used for each step, in order to run the build of this step inside a sub-folder.
- The example directory had been updated to provide an "acceptance test" and to help end users.
- The naming "context" is following the docker build convention to be coherent: https://docs.docker.com/engine/reference/commandline/build/#extended-description
Ignoring the files with the extension .generated. These are the Dockerfiles generated by habitus and should not be tracked.
The test suite has been fixed by adding the go get . It is currently failing on the cloud66/master branc (both test and crosscompile) with the following error message:

vendor/github.com/docker/docker/pkg/fileutils/fileutils.go:13:2: cannot find package "github.com/sirupsen/logrus" in any of:
	/usr/local/go/src/github.com/cloud66/habitus/vendor/github.com/sirupsen/logrus (vendor tree)
	/usr/local/go/src/vendor/github.com/sirupsen/logrus
	/usr/local/go/src/github.com/sirupsen/logrus (from $GOROOT)
	/go/src/github.com/sirupsen/logrus (from $GOPATH)

=> As I understand, it sounds like that the vendored github.com/docker/docker dependency is not resolving its own dependency transitively. I don't know anything about Golang's dependency management so I did it "blindly" to make it work.

Do not hesitate to comment the code, but please give me guidance for improvmeent if needed: I learnt Golang today and used this feature as a first real case (and as an excuse to learn).

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

dduportal · 2018-05-10T18:43:21Z

PR for the Documentation: #103

Documenting the 'context' keyword as part of PR #102

mumoshu · 2018-05-15T07:54:37Z

examples/context/sub/Dockerfile

@@ -1,6 +1,10 @@
 #use the golang base image
 FROM golang:1.7
-MAINTAINER Daniël van Gils
+LABEL MAINTAINER="Daniël van Gils"


Oh! I didn't know that it was deprecated almost one and a half years ago. Good catch.

moby/moby#25466

I am sorry, I should have added a comment to explain this. Happy to help :)

mumoshu · 2018-05-15T07:58:01Z

.gitignore

@@ -34,3 +34,6 @@ habitus_linux_386
 habitus_linux_amd64
 habitus_linux_arm
 launch.json
+
+# Habitus-Generated Dockerfiles
+*.generated


mumoshu · 2018-05-15T08:06:03Z

Dockerfile.tester

@@ -5,10 +5,11 @@ MAINTAINER Daniël van Gils
 #get all the go testing stuff
 RUN go get github.com/onsi/ginkgo/ginkgo
 RUN go get github.com/onsi/gomega
+RUN go get github.com/sirupsen/logrus


Aha! I was wondering why go couldn't find the vendored Sirupsen/logrus... It turns out the author renamed his github username from capitalized to not.

sirupsen/logrus#570

So, good catch. Probably we need both sirupsen/logrus either vendored or installed sysem-wide to make it work. Makes sense.

mumoshu · 2018-05-15T08:08:04Z

build/manifest.go

@@ -45,6 +45,7 @@ type Step struct {
 	Name              string
 	Label             string
 	Dockerfile        string
+  Context           string


Would you mind running find . -path ./vendor -prune -type f -o -name '*.go' -exec gofmt -w {} + and see if it fixes this indentation issue?

Oh right, I was not aware of the gofmt command, cool!
It is done.

mumoshu

Thank you so much for your contribution.

Code LGTM! Only minor comment about code format. I can fix the siruspen/logrus issue on my own, but appreciate it if you could run dep ensure -add github.com/sirupsen/logrus to see if it fixes the issue without the Dockerfile change.

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

dduportal · 2018-05-16T12:35:11Z

Hello @mumoshu !
Thank you for the review you did and your feedbacks!

I have added the 2 following changes to the PR:

The gofmt command had been ran. I was not aware of such a golang functionnality, it is very useful! The issue with indentation should be gone now.
Your assumption about sirupsen/logrus dependency resolved using "dep" is right: the change on the Dockerfile are not needed with this. So I did the following:
- Adding a "RUN" instruction to install the command dep in the main Dockerfile (it is not available into the official golang:1.8 Docker image).
- Removing the RUN go get .../sirupsen/logrus instructions
- Updating the Gopkg.* files after they have been changed by the "dep" command.

I am sure the dependency management could be improved (by removing all the "go get" instructions on the Dockerfiles, and by automatizing the dep command at build time). But I would like to make this PR an "atomic" change, and track the dependency solving on another PR if possible.

khash · 2018-05-16T18:34:40Z

Thank you @dduportal and @mumoshu

This should go out next week on Tuesday.

dduportal · 2018-05-21T15:49:08Z

Hello @khash , thanks a lot! Let us know when it will be released :)

khash · 2018-05-22T08:34:18Z

@dduportal @mumoshu v1.0.3 is now released! Thank you very much for the help and your contributions!

khash · 2018-05-22T08:36:38Z

cc @Kasia66 for Cloud 66 changelog.

dduportal added 3 commits May 10, 2018 20:17

Git-ignoring the Dockerfile generated by habitus

cf42a41

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Fixing Test suite by adding the sirupsen/logrus dependency

13f9a1d

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Implementing the context keyword

09dbbda

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

This was referenced May 10, 2018

[Feature Request] Ability to build dockerfile located inside sub-directories #83

Closed

Documenting the 'context' keyword as part of PR #102 #103

Merged

khash added a commit that referenced this pull request May 14, 2018

Merge pull request #103 from dduportal/gh-pages

4ee1cf6

Documenting the 'context' keyword as part of PR #102

mumoshu reviewed May 15, 2018

View reviewed changes

dduportal added 2 commits May 16, 2018 09:50

Apply gofmt on all go source files

2720150

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Using Golang dep command to fix sirupsen/logrus dependency

982e04f

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

khash approved these changes May 16, 2018

View reviewed changes

khash merged commit 266319f into cloud66-oss:master May 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Feature] Ability to build dockerfile located inside sub-directories #102

[New Feature] Ability to build dockerfile located inside sub-directories #102

dduportal commented May 10, 2018 •

edited

Loading

dduportal commented May 10, 2018

mumoshu May 15, 2018

dduportal May 16, 2018

mumoshu May 15, 2018

mumoshu May 15, 2018 •

edited

Loading

mumoshu May 15, 2018

dduportal May 16, 2018

mumoshu left a comment

dduportal commented May 16, 2018

khash commented May 16, 2018

dduportal commented May 21, 2018

khash commented May 22, 2018

khash commented May 22, 2018

[New Feature] Ability to build dockerfile located inside sub-directories #102

[New Feature] Ability to build dockerfile located inside sub-directories #102

Conversation

dduportal commented May 10, 2018 • edited Loading

dduportal commented May 10, 2018

mumoshu May 15, 2018

Choose a reason for hiding this comment

dduportal May 16, 2018

Choose a reason for hiding this comment

mumoshu May 15, 2018

Choose a reason for hiding this comment

mumoshu May 15, 2018 • edited Loading

Choose a reason for hiding this comment

mumoshu May 15, 2018

Choose a reason for hiding this comment

dduportal May 16, 2018

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

dduportal commented May 16, 2018

khash commented May 16, 2018

dduportal commented May 21, 2018

khash commented May 22, 2018

khash commented May 22, 2018

dduportal commented May 10, 2018 •

edited

Loading

mumoshu May 15, 2018 •

edited

Loading