begin: added docker2aci support for both local and remote images

[cgonyeo]

acbuild begin can now be passed a remote docker image, via
docker://image/name, or a local docker image, via a file path and the
new --docker-image-name flag.

Currently importing local docker files is not completely functional, putting this up for feedback anyway.

./bin/acbuild begin ./alpine.docker --docker-image-name alpine
begin: error getting ImageID: app "library/alpine" not found

[chancez]

To me, this indicates Begin() should start taking a single parameter, which is a struct containing config values. Lots of potentially empty positional arguments will only continue to grow most likely as use-cases develop.

[cgonyeo]

I was actually thinking of exposing all of the helper functions for Begin (beginWithRemoteImage, beginWithEmptyImage, ...) to the world, removing the Begin function, and having the github.com/appc/acbuild/acbuild package contain the logic for which one to call.

Think that would make more sense?

[chancez]

Potentially. Just making a note that providing "zero values" as positional arguments usually indicates that as the function grows in scope, it's parameter list will get very long, with lots more zero values being provided, so a struct does will in that scenario.

If you're breaking it apart and having callers call the appropriate Begin*() function then it's probably unneeded.

[jonboulle]

Shameless cross post, I'd really like to see rkt/rkt#795 (comment) happen and am wondering if we can use this as an excuse to drive it so we can just share code.

[cgonyeo]

bump on this, I'd like to see this finished and then if the rkt fetch plugins land and look like they'd work here I can tear it out again later.

I also just refactored all of lib/begin.go, so that each individual import method has it's own globally exposed function, and acbuild/begin.go now determines which to call.

func (a *ACBuild) BeginFromDirectory(start string) (err error)
func (a *ACBuild) BeginFromLocalDockerImage(start string, dockerImageName string) (err error)
func (a *ACBuild) BeginFromLocalImage(start string) (err error)
func (a *ACBuild) BeginFromRemoteDockerImage(start string, insecure bool) (err error)
func (a *ACBuild) BeginFromRemoteImage(start string, insecure bool) (err error)
func (a *ACBuild) BeginWithEmptyACI() (err error)

For some reason though beginning from local docker files doesn't appear to work for me:

derek@rokot ~/go/src/github.com/appc/acbuild> sudo ./bin/acbuild begin --docker-image-name alpine alpine.dck
begin: error getting ImageID: repositories file not found

[jonboulle]

IMO de-dupe all the boilerplate around this line by having:
type renderFunc func() (string, error)
func (a *ACBuild) beginFromDocker(rf renderFunc) (err error) { ... }

func (a *ACBuild) BeginFromLocalDockerImage(start string, dockerImageName string) (err error) { 
    return a.beginFromDocker(func() (string, error) { 
        return docker2aci.ConvertFile(dockerImageName, start, true, a.ContextPath, "") 
    }) 
}

etc

[jonboulle]

Maybe this is OK but I am a bit worried about the UX, it is a total mishmash. I almost wonder if we could instead encourage people to use the docker2aci CLI tool in tandem with acbuild instead of baking this in. I dunno.

For some reason though beginning from local docker files doesn't appear to work for me:

that seems like a blocker for exposing that..?

[cgonyeo]

It's broken for me too, and it's definitely a blocker. Wasn't able to dig into why it's broken yet.

On Dec 25, 2015, at 15:19, Jonathan Boulle [email protected] wrote:

Maybe this is OK but I am a bit worried about the UX, it is a total mishmash. I almost wonder if we could instead encourage people to use the docker2aci CLI tool in tandem with acbuild instead of baking this in. I dunno.

For some reason though beginning from local docker files doesn't appear to work for me:

that seems like a blocker for exposing that..?

―
Reply to this email directly or view it on GitHub.

[chancez]

Given that starting with a docker image is really the only reasonable way
(i.e. Not a hassle) to start building an ACI i think baking docker2aci into
acbuild is a must.

The ecosystem is already so built around docker and base images are a huge
part of it. People are not going to switch all at once either and until we
can easily create base images and host them as an ACI I think this is a
show stopper to adoption. Just my 2c.
On Fri, Dec 25, 2015 at 3:21 PM Derek Gonyeo [email protected]
wrote:

It's broken for me too, and it's definitely a blocker. Wasn't able to dig
into why it's broken yet.

On Dec 25, 2015, at 15:19, Jonathan Boulle [email protected]
wrote:

Maybe this is OK but I am a bit worried about the UX, it is a total
mishmash. I almost wonder if we could instead encourage people to use the
docker2aci CLI tool in tandem with acbuild instead of baking this in. I
dunno.

For some reason though beginning from local docker files doesn't appear
to work for me:

that seems like a blocker for exposing that..?

―
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#138 (comment).

[cgonyeo]

Revisited this. I dropped support for loading in docker images from disk to side step that issue for now, and everything appears to be working.

I think baking docker2aci support into acbuild is worthwhile over just telling users to use docker2aci, as it lowers the barrier for people to be able to get up and running building ACIs.

I also think the UX right now around beginning from stuff is fine, you call begin and hand it a local or remote thing (differentiated by starting with a ., /, or ~ to signify local), and acbuild figures out what file you're starting a build from.

Begin from nothing:

acbuild begin

Begin from a rootfs tar file:

acbuild begin ./rootfs.tar.gz

Begin from a local ACI:

acbuild begin ./myapp.aci

Begin from a remote ACI:

acbuild begin quay.io/coreos/alpine-sh

Begin from a remote docker image:

acbuild begin docker://alpine

[jonboulle]

Seems OK. This is with latest upstream docker2aci?

[cgonyeo]

Yup, v0.11.1.

[jonboulle]

LGTM for now.

[alban]

For the record, I see this is in acbuild v0.3.1.

[cgonyeo]

Yeah I screwed up the release's changelog (the time between me writing it and it being merged was longer than I expected). Since it's an enhancement, doesn't impact people who don't know about it, and doesn't show up in the help pages, I figured I'd just include it in the next release's changelog.