Store built image digest in the build status #12407

mmilata · 2017-01-06T14:59:47Z

Trello card: https://trello.com/c/SAvLJLLl/1036-5-provide-the-built-image-reference-as-part-of-the-build-status-builds

@legionus @miminar I'm extracting the digest from the stream of jsons docker client generates during pushing, is this approach fine with you guys? I've asked Docker if they consider it part of the API: moby/moby#29865.

@bparees PTAL

mmilata · 2017-01-06T15:01:30Z

[test][testextended][extended:core(builds)]

bparees · 2017-01-06T16:11:24Z

lgtm, @openshift/api-review please approve the new digest field.

smarterclayton · 2017-01-06T16:42:15Z

pkg/build/api/types.go

+	// outputDockerImageDigest is the digest of the built Docker image. The
+	// digest uniquely identifies the image in the registry to which it was
+	// pushed.
+	OutputDockerImageDigest string


Put this in a struct that parallels spec output - probably status.output.to.imageDigest where output is a struct, to is a pointer to a struct, and to is only set when an image was output.

I've changed the structs as requested, PTAL if it's what you wanted.

smarterclayton · 2017-01-06T16:43:14Z

Also add at to the field description that the field may not be set the push completes but the build encounters an unexpected failure. That is, the field is set "best effort".

…

On Fri, Jan 6, 2017 at 11:21 AM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test FAILURE ( https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12686/) (Base Commit: 9e682de <9e682de> ) — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#12407 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_h85JVOyFSe9z1kicWAdxPueD3rks5rPmoHgaJpZM4LcyRW> .

mmilata · 2017-01-12T15:03:43Z

Rebased, though there's still a bug in the digest extraction code that makes the tests fail that I'm working on.

miminar · 2017-01-18T07:19:04Z

api/protobuf-spec/github_com_openshift_origin_pkg_build_api_v1.proto

+message BuildStatusOutputTo {
+  // imageDigest is the digest of the built Docker image. The digest uniquely
+  // identifies the image in the registry to which it was pushed. Please note
+  // that this field may not always set even if the push completes


What causes the digest not to be set?

miminar

Few remarks. LGTM overall.

miminar · 2017-01-18T07:20:24Z

api/protobuf-spec/github_com_openshift_origin_pkg_build_api_v1.proto

+message BuildStatusOutputTo {
+  // imageDigest is the digest of the built Docker image. The digest uniquely
+  // identifies the image in the registry to which it was pushed. Please note
+  // that this field may not always set even if the push completes


What causes the digest not to be set?

miminar · 2017-01-18T07:26:23Z

pkg/build/builder/docker.go

 			return reportPushFailure(err, authPresent, pushAuthConfig)
 		}
+		if digest != "" {


len(digest) > 0

Just curious, is it a matter of consistency or is there a practical difference?

I'd say it's just about consistency.

miminar · 2017-01-18T07:28:05Z

pkg/build/builder/dockerutil.go

@@ -73,28 +77,33 @@ func pullImage(client DockerClient, name string, authConfig docker.AuthConfigura
 // - Docker registry is down temporarily or permanently
 // - other image is being pushed to the registry
 // If any other scenario the push will fail, without retries.


Document the newly added return value.

miminar · 2017-01-18T07:41:35Z

pkg/build/builder/dockerutil.go

+	for {
+		// save the not yet parsed input so we can restore it in case it
+		// contains part of valid JSON
+		savedBuf, err := ioutil.ReadAll(dec.Buffered())


If the err is ignored intentionally, use _.

miminar · 2017-01-18T07:51:14Z

pkg/build/builder/dockerutil.go

+	}
+}
+
+// simpleProgressWriter is an io.Writer which consumes a stream of json


s/simpleProgressWriter/newSimpleWriter/

miminar · 2017-01-18T07:52:12Z

pkg/build/builder/dockerutil.go

+// with RawJSONStream=false.
+func newSimpleWriter(output io.Writer) io.Writer {
+	return newPushWriter(func(line progressLine) error {
+		if line.Stream != "" {


len(line.Stream) > 0 and the same below

miminar · 2017-01-18T08:05:08Z

pkg/build/builder/dockerutil_test.go

+		decoded := []progressLine{}
+		w := newPushWriter(func(line progressLine) error {
+			decoded = append(decoded, line)
+			if line.Error != "" {


len(line.Error) > 0 and the same below

miminar · 2017-01-18T08:16:54Z

test/extended/builds/digest.go

+				br, err := exutil.StartBuildAndWait(oc, "test")
+
+				g.By("checking that the resulting image has a digest")
+				o.Expect(br.Build.Status.Output.To.ImageDigest).To(o.HavePrefix("sha256:"))


Can you also fetch the created image object by istag and match its name against the digest?

miminar · 2017-01-18T08:20:42Z

test/extended/builds/digest.go

+		})
+
+		g.Describe("started with log level >5", func() {
+			g.It(fmt.Sprintf("should save the image digest when finished"), func() {


All the It blocks differ in just few values. Could you make it a top-level parametrised function and call it from each sub-Describe?

Huh, I even searched the ginkgo docs to see if it supports parametrized tests, missing the obvious solution completely /facepalm.

mmilata · 2017-01-18T15:17:40Z

@miminar thank you for the review. I've addressed the issues you found and pushed updated branch.

bparees · 2017-01-18T16:06:52Z

@openshift/api-review needs api approval to land for 3.5.

mmilata · 2017-01-18T18:32:39Z

Flake #12341, #12479.

miminar

LGTM, thanks! Few amendments to string comparisons would make it perfect.

miminar · 2017-01-19T11:58:32Z

pkg/build/builder/docker.go

 			return reportPushFailure(err, authPresent, pushAuthConfig)
 		}
+		if digest != "" {


miminar · 2017-01-19T12:01:33Z

pkg/build/builder/sti.go

 			return reportPushFailure(err, authPresent, pushAuthConfig)
 		}
+		if digest != "" {


Fixed, thanks:)

git show | grep -c '^[+].*[!=]= ""' 0

mmilata · 2017-01-19T16:47:31Z

Flake #12479. [testextended][extended:core(builds)]

smarterclayton · 2017-01-19T17:05:07Z

API approved.

bparees · 2017-01-19T19:16:14Z

extended test failure is a known flake but it's also in an area of code related to what's being changed here, so i'd like to see a clean run.

[testextended][extended:core(failure reason)]

bparees · 2017-01-19T21:09:16Z

seems like a consistent failure that needs to be investigated.

openshift-bot · 2017-01-20T12:45:36Z

Evaluated for origin test up to 0ea756b

openshift-bot · 2017-01-20T12:45:46Z

Evaluated for origin testextended up to 0ea756b

openshift-bot · 2017-01-20T14:13:31Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13078/) (Base Commit: 1e269a3)

mmilata · 2017-01-20T14:32:30Z

Flake #9414. Trying to reproduce the failing extended tests locally.

openshift-bot · 2017-01-20T14:37:40Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1006/) (Base Commit: 1e269a3) (Extended Tests: core(failure reason), core(builds))

bparees · 2017-01-20T14:41:13Z

flake #9414
[merge]

openshift-bot · 2017-01-20T14:45:33Z

Evaluated for origin merge up to 0ea756b

openshift-bot · 2017-01-20T17:37:37Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13096/) (Base Commit: 8146e11) (Image: devenv-rhel7_5736)

bparees added the needs-api-review label Jan 6, 2017

bparees self-assigned this Jan 6, 2017

smarterclayton reviewed Jan 6, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2017

mmilata force-pushed the store-digest branch from 9babf9c to 4460c7d Compare January 12, 2017 14:58

mmilata changed the title ~~Store built image digest in the build status~~ [do not merge] Store built image digest in the build status Jan 12, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2017

mmilata force-pushed the store-digest branch 3 times, most recently from 46a6c41 to 3078317 Compare January 17, 2017 16:02

mmilata mentioned this pull request Jan 17, 2017

imageProgressWriter doesn't propagate docker errors under certain conditions openshift/imagebuilder#32

Closed

mmilata force-pushed the store-digest branch from 3078317 to 3f54f76 Compare January 17, 2017 18:43

mmilata changed the title ~~[do not merge] Store built image digest in the build status~~ Store built image digest in the build status Jan 17, 2017

miminar reviewed Jan 18, 2017

View reviewed changes

miminar suggested changes Jan 18, 2017

View reviewed changes

mmilata force-pushed the store-digest branch 3 times, most recently from 339508e to 2630ae2 Compare January 18, 2017 15:06

mmilata force-pushed the store-digest branch from 2630ae2 to 0b43c21 Compare January 18, 2017 15:25

miminar approved these changes Jan 19, 2017

View reviewed changes

mmilata force-pushed the store-digest branch from 0b43c21 to ad9dc0a Compare January 19, 2017 13:05

mmilata mentioned this pull request Jan 19, 2017

Built image digest is stored in the build status openshift/openshift-docs#3533

Closed

smarterclayton added api-approved and removed needs-api-review labels Jan 19, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2017

Store built image digest in the build status

0ea756b

mmilata force-pushed the store-digest branch from ad9dc0a to 0ea756b Compare January 20, 2017 12:37

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2017

openshift-bot merged commit 636ecf5 into openshift:master Jan 20, 2017

adellape mentioned this pull request Feb 17, 2017

Built image digest stored in build status openshift/openshift-docs#3772

Merged

Store built image digest in the build status #12407

Store built image digest in the build status #12407

Conversation

mmilata commented Jan 6, 2017

mmilata commented Jan 6, 2017

bparees commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jan 6, 2017 via email

mmilata commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar 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

Choose a reason for hiding this comment

mmilata commented Jan 18, 2017

bparees commented Jan 18, 2017

mmilata commented Jan 18, 2017

miminar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilata Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

mmilata commented Jan 19, 2017

smarterclayton commented Jan 19, 2017

bparees commented Jan 19, 2017

bparees commented Jan 19, 2017

openshift-bot commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

mmilata commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

bparees commented Jan 20, 2017

openshift-bot commented Jan 20, 2017

openshift-bot commented Jan 20, 2017 • edited Loading

mmilata Jan 19, 2017 •

edited

Loading

openshift-bot commented Jan 20, 2017 •

edited

Loading