Introduce "generate" command #1002

isutton · 2019-10-31T14:58:01Z

This PR create a new s2i top level command, generate.

The choice to create this command instead of just implementing the same functionality in the build --as-dockerfile flow, is that the semantics of builder image would change between build and build --as-dockerfile; the former would reflect changes made to local images, where the latter would ignore those. This means s2i build my-image . and s2i build --as-dockerfile my-image . | buildah commands could produce different results.

This new command expects the given builder image name to be available on a remote registry, and is inspected through the infrastructure provided by containers/image.

Once OCI support is implemented in s2i, this command could benefit and provide transparent access to local images as well, since it would delegate the inspection task to the container runtime itself and even deprecate build --as-dockerfile.

This PR requires some dependencies to be updated, since it depends on containers/image, and both depended (and still depend) on Docker.

In cli.go the gen-dockerfile command is registered.

In dockerfile.go, the CreateDockerfile() method was modified to inherit both scripts-url and destination labels.

gen-dockerfile.go is a new file, where the command is implemented.

In go.mod, dependency to containers/image was added, together with replacements for indirect dependencies based on containers/image go.mod file.

openshift-ci-robot · 2019-10-31T14:58:18Z

Hi @isutton. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bparees · 2019-10-31T15:00:10Z

and even deprecate build --as-dockerfile.

i would definitely want to see --as-dockerfile deprecated when this new command is introduced, to avoid user confusion about which to use and differences in behavior between the two.

otaviof · 2019-10-31T15:02:23Z

Naming wise I suggest calling this new sub-command generate instead, to follow the names of the other existing sub-commands.

isutton · 2019-10-31T15:13:19Z

and even deprecate build --as-dockerfile.

i would definitely want to see --as-dockerfile deprecated when this new command is introduced, to avoid user confusion about which to use and differences in behavior between the two.

I'm not sure about doing both at the same time, but I would definitely release a minor to introduce the command, and a major to remove the --as-dockerfile flag in the case this is the direction we want to move forward.

sbose78 · 2019-10-31T15:33:25Z

Thanks @isutton and @otaviof for this, and thank you @bparees for taking a quick look.

Even though I find s2i build --as-dockerfile super-confusing because it doesn't actually build, I feel there's a need to fix image inspection for it #973 instead of having a new command.

My worry is:

s2i build : inspects image labels, builds images
s2i build --as-dockerfile: doesn't inspect image labels, doesn't build image, generates dockerfile
s2i gen-dockerfile inspects registry images image labels , doesn't build images, generates dockerfile

I would love to remove (2) entirely to avoid this. Or move (3) into (2).

@siamaksade , thoughts?

bparees · 2019-10-31T15:36:31Z

s2i build : inspects image labels, builds images

s2i build --as-dockerfile: doesn't inspect image labels, doesn't build image, generates dockerfile

s2i gen-dockerfile inspects registry images image labels , doesn't build images, generates dockerfile

I would love to remove (2) entirely to avoid this. Or move (3) into (2).

why not get rid of (2) entirely to avoid this?

bparees · 2019-10-31T15:36:48Z

(long term, after a deprecation period)

sbose78 · 2019-10-31T15:50:19Z

👍 to removing (2) . I think we should do it as part of a version 1.x release?

siamaksade · 2019-10-31T16:04:30Z

@bparees @sbose78 I agree with deprecation and removal following its grace period

adambkaplan · 2019-10-31T17:55:32Z

+1 to removal.

Now that we are switching to go modules, v2 may come sooner than we think. Any refactor that removes or alters an exported method/constant requires a major version bump.

https://github.com/golang/go/wiki/Modules#semantic-import-versioning

sbose78 · 2019-10-31T18:28:46Z

I was hoping v2 would look a lot more different ( a lot more breaking changes ;) ), but I'm good with getting to v2 sooner, and maybe postpone bigger changes to a future version.

isutton · 2019-11-08T09:16:42Z

The past couple of days @otaviof and I have been enabling buildah in s2i from start to end (#1003), and since this effort is being quite successful I'd like to propose a strategy for aligning that PR and this one.

As stated previously, the gen-dockerfile command was proposed because the same functionality present in s2i build --as-dockerfile:

Changed the semantics of the builder image, where without --as-dockerfile it would consult both local and remote builder images, but with it would consider only remote builder images. This alone makes the functionality incoherent and error prone for the end user.
Doesn't correctly represent the same actions s2i would do for the given input parameters, meaning images built by s2i and images built by a Dockerfile produced by s2i are not equivalent in some scenarios, making it harder for both the end user and s2i or builder image developers to correctly assess whether the right results are being yielded.

There are a couple of points I'm still unsure:

In the case Buildah is a first class citizen in s2i, I would expect that generating a Dockerfile might be not as important as before, since s2i build would work in OCP 4 with its Docker socket restrictions.
To produce a Dockerfile equivalent to the process s2i applies in its build process, the majority of build arguments and switches are required in this command to properly build the Dockerfile, since both commands would share the build process. I personally dislike this fact, and IMO it points back in the direction of --as-dockerfile in the developer experience front, since the result of s2i build would be equivalent to s2i build --as-dockerfile | buildah bud -f -.

My take on this is to finalize #1003, since it will enable s2i to build an application image without requiring a Docker socket, making it suitable to run in Tekton, Travis CI and other build pipelines. Additionally, it also gives us the foundation to produce an accurate Dockerfile based on the existing s2i build workflow.

sbose78 · 2019-11-08T13:38:37Z

https://github.com/openshift/builder does use the Dockerfile generation flow and then does a lot of other things. So, I wouldn't want to comment on the future of Dockerfile generation.

Dockerfile generation is handy because it lets us separate the concern of 'define strategy' v/s 'build the image using a strategy'.

So, irrespective of #1003 , we would need this to be in ASAP.

bparees · 2019-11-08T18:45:13Z

Dockerfile generation is handy because it lets us separate the concern of 'define strategy' v/s 'build the image using a strategy'.

+1. i think there is a lot of value in avoiding tying s2i to runtime libraries both on the inspection/generation side as well as the image construction side.

sbose78 · 2019-11-08T20:29:11Z

/ok-to-test

sbose78 · 2019-11-08T20:36:09Z

/retest

isutton · 2019-11-09T16:50:28Z

/retest

isutton · 2019-11-09T17:01:24Z

/retest

bparees · 2019-11-11T14:52:53Z

/test all

bparees · 2019-11-11T14:52:58Z

/refresh

adambkaplan · 2019-12-04T20:00:16Z

/retest

isutton · 2019-12-05T22:13:44Z

Test is currently not compiling with the following:

# github.com/openshift/source-to-image/test/integration/docker [github.com/openshift/source-to-image/test/integration/docker.test]
test/integration/docker/integration_test.go:154:6: undefined: client.IsErrImageNotFound

Note that I believe our CI vms use golang 1.12

Fixed in cbcc36a.

isutton · 2019-12-05T22:24:15Z

@isutton Are there any tests for this new s2i generate command?

I considered all the important logic was already being tested, and the logic that creates the Dockerfile was already being tested, so I didn't do it at the time.

Will look at that.

All generate does is either encoded in very straightforward and simple code, or is already tested through either SBO's or vendored libraries unit tests, so tests specific to the generate command have not been implemented.

isutton · 2019-12-05T22:27:01Z

Is there anything I can do to move this PR and finalize this piece of work?

adambkaplan

@isutton General logic here looks good. I think we can refactor the getImageLabels function in a follow-up PR, since openshift/builder implements its own logic to set image labels.

I'm worried a bit about the go mod bumps. From what I can gather all we need is containers/image/v5 and its dependencies.

Finally, these commits need to be squashed. I'd like to see the following:

A clean commit with the dependencies bumped in go.mod and go.sum
A bump(*) commit with the vendor updates.
A commit with the new functionality

adambkaplan · 2019-12-06T18:30:20Z

go.mod

+	github.com/containerd/fifo v0.0.0-20190816180239-bda0ff6ed73c // indirect
+	github.com/containers/image/v5 v5.0.0
+	github.com/coreos/etcd v3.3.17+incompatible // indirect
+	github.com/docker/distribution v2.7.1-0.20190205005809-0d3efadf0154+incompatible


this should be ok - docker/distribution on openshift/builder is 2.7.1

adambkaplan · 2019-12-06T18:32:51Z

go.mod

-	github.com/docker/distribution v2.6.0-rc.1.0.20170726174610-edc3ab29cdff+incompatible
-	github.com/docker/docker v0.0.0-20190404075923-dbe4a30928d4
+	github.com/containerd/fifo v0.0.0-20190816180239-bda0ff6ed73c // indirect
+	github.com/containers/image/v5 v5.0.0


this is ok - containers/image uses strict semvers so we won't have transitive dep conflicts in openshift/builder

adambkaplan · 2019-12-06T18:38:18Z

pkg/cmd/cli/cmd/generate.go

+		Short: "Generate a Dockerfile based on the provided builder image",
+		Example: `
+# Generate a Dockerfile from a remote builder image:
+$ s2i generate docker://quay.io/redhat-developer/app-binding-operator Dockerfile.gen


I'd use one of our canonical s2i images as an example - docker.io/centos/nodejs-10-centos7

@isutton Could you please make the above change?

@isutton please update the example here.

Also a bit of a nit - is it worth switching the order around? Thoughts on the following semantics?

$ s2i generate DOCKERFILE [IMAGE]

or

$ s2i generate DOCKERFILE --from=[IMAGE]

From what I understood, IMAGE is always required in this use case since there's no information regarding that; additionally, the protocol currently must be informed by the user to refer to the image since containers/image is responsible for retrieving its contents and inspecting its labels.

I believe the following example is closer to what we want, where DOCKERFILE could have the Dockerfile value by default:

$ s2i generate IMAGE [DOCKERFILE]

During implementation, I opted for DOCKERFILE to be mandatory and not having defaults to avoid unwillingly Dockerfile overwrites.

The requested example modification has been made.

Right, IMAGE is always required since it is the s2i builder image. I think Adam is OK with that.

I do like s2i generate DOCKERFILE --from=quay.io/builder/image

instead of

s2i generate quay.io/builder/image Dockerfile.gen

From what I understood, IMAGE is always required in this use case since there's no information regarding that;

Correct

During implementation, I opted for DOCKERFILE to be mandatory and not having defaults to avoid unwillingly Dockerfile overwrites.

I agree with this choice for now. We can always opt to make it optional later; I have other ideas on how to arrange the arguments to this command that are worth discussing in a follow-up PR.

pkg/cmd/cli/cmd/generate.go

adambkaplan · 2019-12-06T18:42:12Z

pkg/cmd/cli/cmd/generate.go

+			cfg.BuilderImage = cmd.Flags().Arg(0)
+			cfg.AsDockerfile = cmd.Flags().Arg(1)
+
+			ctx := context.TODO()


context.Background() is acceptable here, since this is effectively the main entrypoint to the command.

Done in 7eb02e7.

adambkaplan · 2019-12-06T18:43:14Z

go.mod

-	github.com/moby/buildkit => github.com/dmcgowan/buildkit v0.0.0-20170731200553-da2b9dc7dab9
+	github.com/Sirupsen/logrus => github.com/sirupsen/logrus v1.2.0
+	github.com/containerd/containerd => github.com/containerd/containerd v1.3.0
+	github.com/docker/docker => github.com/docker/docker v0.0.0-20180522102801-da99009bbb11


Docker has been downgraded here - why?

To use the same Docker version containers/image is using:

https://github.com/containers/image/blob/v5.0.0/go.mod#L13

adambkaplan · 2019-12-06T18:45:16Z

go.mod

+	github.com/Sirupsen/logrus => github.com/sirupsen/logrus v1.2.0
+	github.com/containerd/containerd => github.com/containerd/containerd v1.3.0
+	github.com/docker/docker => github.com/docker/docker v0.0.0-20180522102801-da99009bbb11
+	golang.org/x/crypto => golang.org/x/crypto v0.0.0-20180904163835-0709b304e793


golang.org/x/crypto has been added in the replace stanza, but is not referenced in the require stanza. Why is this necessary?

After I did my first changes, I noticed that make test didn't work. After I dug into it, I've noticed that buildkit and swarmkit should be updated as well, and this lead to all those dependencies

Tests use buildkit and swarmkit, which also had to be bumped and adjusted (see 9f3ead4 for reference).

isutton · 2019-12-08T22:49:22Z

@isutton General logic here looks good. I think we can refactor the getImageLabels function in a follow-up PR, since openshift/builder implements its own logic to set image labels.

Sounds fine.

I'm worried a bit about the go mod bumps. From what I can gather all we need is containers/image/v5 and its dependencies.

containers/image/v5 adds a new Docker version as a transitive dependency, being reflected on the replace section in go.mod (some dependencies have been added from Docker's dependencies and other libraries as well).

Finally, these commits need to be squashed. I'd like to see the following:
1. A clean commit with the dependencies bumped in `go.mod` and `go.sum`

2. A `bump(*)` commit with the vendor updates.

3. A commit with the new functionality

Will do.

adambkaplan · 2019-12-17T21:22:25Z

@isutton note that we recently merged a go.mod bump that addressed some of the issues with buildkit.

sbose78 · 2020-01-15T20:47:57Z

I think this is now ready?

isutton · 2020-01-20T07:22:19Z

I've updated the branch according to the project's guidelines.

/cc @adambkaplan @sbose78

sbose78 · 2020-01-23T13:20:38Z

@adambkaplan Should we take this in?

adambkaplan

In general this looks good - only have the remaining nit on the example. @sbose78 I'd like you and perhaps @otaviof to weigh in on my comment regarding the command line argument order. Given our prior difficulties with go.mod, I need to verify the dep changes work with golang 1.13.

@isutton thanks so much for the effort you've put in thus far. I think we're almost there.

adambkaplan · 2020-01-23T14:11:26Z

pkg/cmd/cli/cmd/generate.go

+		Short: "Generate a Dockerfile based on the provided builder image",
+		Example: `
+# Generate a Dockerfile from a remote builder image:
+$ s2i generate docker://quay.io/redhat-developer/app-binding-operator Dockerfile.gen


@isutton please update the example here.

adambkaplan · 2020-01-23T14:15:03Z

pkg/cmd/cli/cmd/generate.go

+		Short: "Generate a Dockerfile based on the provided builder image",
+		Example: `
+# Generate a Dockerfile from a remote builder image:
+$ s2i generate docker://quay.io/redhat-developer/app-binding-operator Dockerfile.gen


Also a bit of a nit - is it worth switching the order around? Thoughts on the following semantics?

$ s2i generate DOCKERFILE [IMAGE]

or

$ s2i generate DOCKERFILE --from=[IMAGE]

This change adds the generate command to s2i, where it allows the user to produce a Dockerfile able to be used by any build system supporting the format, such as buildah and possibly others.

adambkaplan · 2020-01-23T17:09:18Z

/approve

/lgtm

Will follow up on my "other ideas" I have for the cli args.

openshift-ci-robot · 2020-01-23T17:22:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, isutton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [adambkaplan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2019

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 31, 2019

openshift-ci-robot requested review from adambkaplan and gabemontero October 31, 2019 15:00

isutton mentioned this pull request Nov 8, 2019

Honor builder image labels when --as-dockerfile is enabled #999

Closed

isutton marked this pull request as ready for review November 8, 2019 14:37

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2019

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2019

adambkaplan requested changes Dec 6, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2019

bparees mentioned this pull request Jan 6, 2020

Can we get this to work with Buildah/podman rather then being Docker Centric #966

Closed

isutton force-pushed the isutton/gen-dockerfile branch from 7eb02e7 to eaa5d08 Compare January 9, 2020 13:41

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2020

isutton force-pushed the isutton/gen-dockerfile branch from eaa5d08 to 677a104 Compare January 10, 2020 09:54

Add containers/image v5

f15db29

isutton force-pushed the isutton/gen-dockerfile branch from 677a104 to c8e4429 Compare January 10, 2020 11:14

bump(*): containers/image dependencies

b4ed329

isutton force-pushed the isutton/gen-dockerfile branch from c8e4429 to 554280f Compare January 10, 2020 11:32

openshift-ci-robot requested review from adambkaplan and sbose78 January 20, 2020 07:22

adambkaplan reviewed Jan 23, 2020

View reviewed changes

Add generate command

f8dcf74

This change adds the generate command to s2i, where it allows the user to produce a Dockerfile able to be used by any build system supporting the format, such as buildah and possibly others.

isutton force-pushed the isutton/gen-dockerfile branch from 554280f to f8dcf74 Compare January 23, 2020 15:28

openshift-ci-robot assigned adambkaplan Jan 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020

openshift-merge-robot merged commit 3311f0e into openshift:master Jan 23, 2020

Introduce "generate" command #1002

Introduce "generate" command #1002

Conversation

isutton commented Oct 31, 2019 • edited Loading

openshift-ci-robot commented Oct 31, 2019

bparees commented Oct 31, 2019

otaviof commented Oct 31, 2019

isutton commented Oct 31, 2019

sbose78 commented Oct 31, 2019

bparees commented Oct 31, 2019

bparees commented Oct 31, 2019

sbose78 commented Oct 31, 2019 • edited Loading

siamaksade commented Oct 31, 2019

adambkaplan commented Oct 31, 2019

sbose78 commented Oct 31, 2019

isutton commented Nov 8, 2019 • edited Loading

sbose78 commented Nov 8, 2019

bparees commented Nov 8, 2019

sbose78 commented Nov 8, 2019

sbose78 commented Nov 8, 2019

isutton commented Nov 9, 2019

isutton commented Nov 9, 2019

bparees commented Nov 11, 2019

bparees commented Nov 11, 2019

adambkaplan commented Dec 4, 2019

isutton commented Dec 5, 2019

isutton commented Dec 5, 2019

isutton commented Dec 5, 2019

adambkaplan 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

isutton Jan 23, 2020 • 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

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

isutton commented Dec 8, 2019

adambkaplan commented Dec 17, 2019

sbose78 commented Jan 15, 2020

isutton commented Jan 20, 2020

sbose78 commented Jan 23, 2020

adambkaplan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambkaplan commented Jan 23, 2020

openshift-ci-robot commented Jan 23, 2020

isutton commented Oct 31, 2019 •

edited

Loading

sbose78 commented Oct 31, 2019 •

edited

Loading

isutton commented Nov 8, 2019 •

edited

Loading

isutton Jan 23, 2020 •

edited

Loading