Gather build step timing information and output using oc describe #13307

coreydaley · 2017-03-08T16:36:21Z

Gathers build step timing information for significant step of the build process and makes it available to the 'oc describe' command.
Dependent on openshift/source-to-image#694

coreydaley · 2017-03-08T16:36:56Z

(5) Separate out build substatus and times [builds]

coreydaley · 2017-03-13T15:48:08Z

@openshift/devex ptal

This PR is dependent on code from pull request openshift/source-to-image#694 being updated in vendor/github.com/openshift/source-to-image , so if you clone this and test it, you'll need to also clone my new code in source-to-image on the add_timing_info branch and copy it into the vendor dir.
Example output from 'oc describe builds':

Steps:			4
 - FetchSource:		1s
 - DockerPullImage:	2s
 - DockerBuild:		1m16s
 - ExecPostCommitHook:	0s

gabemontero · 2017-03-13T18:37:04Z

@coreydaley - Took a glance ... struggled with a way to suggest a wrappering of the timestamp before and steps update after into a utility method, where we pass in the function being measured as a parameter into that method, but couldn't sort out a clean way to handle varying parameters and return types of the functions being measured. My guess is you already entertained that, but thought I would mention it just the same.

Otherwise, no suggestions, etc. occurred to me. For what it is worth, IGTM.

coreydaley · 2017-03-13T18:55:23Z

@gabemontero Thanks for taking a look. I did entertain your suggestions. See the conversation between @jim-minter and @bparees on openshift/source-to-image#694 for more info on why that wasn't the way this went.

csrwng · 2017-03-13T18:58:13Z

pkg/build/api/types.go

+type Steps []StepInfo
+
+// RecordInfo records details about each build step
+func (s *Steps) RecordInfo(stepName StepName, startTime unversioned.Time, stopTime unversioned.Time) {


@coreydaley We don't usually add methods to API types, and don't put them here in types.go. You could add a helper function in helper.go if needed.

@csrwng
@bparees had asked me to add it as a method onto the type in the source-to-image pull request, so that seemed like what should happen here also. Was just trying to be consistent between the code bases. Maybe Ben can weigh in here also.

@csrwng maybe i'm being too javay, but it seems pretty logical to have a method like this on the data structure itself rather than as a helper. For all the usual data encapsulation/abstraction reasons.

as for where such a definition belongs. personally i prefer to see struct methods defined in the same place as the struct (again, perhaps being too javay), though i can see how that might start to pollute types.go.

is there a reason why we have historically not added heper methods to api types? it's not like it's part of the public api or anything.

@bparees nothing comes to mind except convention (see types.go in kubernetes). @deads2k?

@bparees nothing comes to mind except convention (see types.go in kubernetes). @deads2k?

To my knowledge, convention. The structs in types.go are supposed to be pure value objects without any higher order logic on them. Doing it that way makes it possible to separate the type definitions packages from any logic performed on them.

@smarterclayton smacked my hand a few times for wanting to have helper methods attached. Go is nasty no matter what you do, so it didn't bother me much.

coreydaley · 2017-03-14T16:00:40Z

@bparees @csrwng @deads2k
The code has been refactored to remove the RecordInfo method and added a RecordStepInfo helper function

smarterclayton · 2017-03-15T20:17:57Z

pkg/build/api/v1/types.go

+	StepName StepName `json:"stepName,omitempty" protobuf:"bytes,1,opt,name=stepName"`
+
+	// StartTime identifies when this step started.
+	StartTime unversioned.Time `json:"startTime,omitempty" protobuf:"bytes,2,opt,name=startTime"`


Are clients supposed to assume that these don't overlap? What happens when they do overlap?

Correct, they should not overlap the way that we have wrapped the steps that occur. Maybe adding some tests that check for overlapping steps would be useful here.

smarterclayton · 2017-03-15T20:46:00Z

pkg/build/api/v1/types.go

@@ -176,8 +176,46 @@ type BuildStatus struct {

 	// output describes the Docker image the build has produced.
 	Output BuildStatusOutput `json:"output,omitempty" protobuf:"bytes,10,opt,name=output"`
+
+	// steps contains details about each build step.
+	Steps Steps `json:"steps,omitempty" protobuf:"bytes,11,opt,name=steps"`


At some point, jenkins needs to record status in a real object. When it does that, I assume it'll be stages, with steps in them. For this PR can you describe this in terms of terminology jenkins uses, and the pipeline plugin will use, so we can end up with a struct in Status that looks as much as possible like the destination will be?

here's the jenkins metadata we record into the build object. it has "stages" and "stageflownodes" within the stages, which appear to be the steps.

for both stages and stageflownodes/steps it provides:
startTimeMillis
durationMillis
pauseTimeMillis

openshift.io/jenkins-status-json: '{"_links":{"self":{"href":"https://jenkins-c1.router.default.svc.cluster.local/job/c1-sample-pipeline/1/wfapi/describe"}},"id":"1","name":"#1","status":"SUCCESS","startTimeMillis":1489611336514,"endTimeMillis":0,"durationMillis":0,"queueDurationMillis":97492,"pauseDurationMillis":0,"stages":[{"_links":{"self":{"href":"https://jenkins-c1.router.default.svc.cluster.local/job/c1-sample-pipeline/1/execution/node/6/wfapi/describe"}},"id":"6","name":"build","execNode":"","status":"SUCCESS","startTimeMillis":1489611434006,"durationMillis":42214,"pauseDurationMillis":0,"stageFlowNodes":[{"_links":{"self":{"href":"https://jenkins-c1.router.default.svc.cluster.local/job/c1-sample-pipeline/1/execution/node/7/wfapi/describe"}},"id":"7","name":"Trigger OpenShift Build","execNode":"","status":"SUCCESS","startTimeMillis":1489611434138,"durationMillis":42074,"pauseDurationMillis":0,"parentNodes":["6"]}]},{"_links":{"self":{"href":"https://jenkins-c1.router.default.svc.cluster.local/job/c1-sample-pipeline/1/execution/node/11/wfapi/describe"}},"id":"11","name":"deploy","execNode":"","status":"SUCCESS","startTimeMillis":1489611476231,"durationMillis":20327,"pauseDurationMillis":0,"stageFlowNodes":[{"_links":{"self":{"href":"https://jenkins-c1.router.default.svc.cluster.local/job/c1-sample-pipeline/1/execution/node/12/wfapi/describe"}},"id":"12","name":"Trigger OpenShift Deployment","execNode":"","status":"SUCCESS","startTimeMillis":1489611476253,"durationMillis":20299,"pauseDurationMillis":0,"parentNodes":["11"]}]}]}'

So if we want to mirror the jenkins api, we should add the following struct to our build objects:

Stage[] { name startTime duration Steps[] { name startTime duration }

@smarterclayton @jwforres tentative agreement?

seems reasonable, @spadgett

I can confirm the stage flow nodes are the steps.

The nodes also have an ID and a parent node. Parent node might be important if we want to tell if steps are parallel vs sequential (although I'd need to look at this more).

There is some doc on the JSON here:

https://github.com/jenkinsci/pipeline-stage-view-plugin/tree/master/rest-api

smarterclayton · 2017-03-17T14:56:15Z

I'm ok with being consistent. Be sure to call the field durationMilliseconds if you do so

…

On Wed, Mar 15, 2017 at 7:31 PM, Sam Padgett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/build/api/v1/types.go <#13307 (comment)>: > @@ -176,8 +176,46 @@ type BuildStatus struct { // output describes the Docker image the build has produced. Output BuildStatusOutput `json:"output,omitempty" protobuf:"bytes,10,opt,name=output"` + + // steps contains details about each build step. + Steps Steps `json:"steps,omitempty" protobuf:"bytes,11,opt,name=steps"` I can confirm the stage flow nodes are the steps. The nodes also have an ID and a parent node. Parent node might be important if we want to tell if steps are parallel vs sequential (although I'd need to look at this more). There is some doc on the JSON here: https://github.com/jenkinsci/pipeline-stage-view-plugin/ tree/master/rest-api — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_afbyFzaeefwwQuucZESJnr_IsXks5rmHTogaJpZM4MXAi5> .

coreydaley · 2017-03-17T15:21:05Z

@openshift/devex ptal
Sample describe command output:

Status:				Complete
Started:			Fri, 17 Mar 2017 10:59:23 EDT
Duration:			2m6s
  Fetch:			2s
    FetchSource:		2s
  Commit:			4s
    CommitContainer:		4s
  Assemble:			1m53s
    AssembleBuildScripts:	1m53s
  Post:				6s
    ExecPostCommitHook:		6s
  PushImages:			4s
    DockerPushImage:		4s

Suggestions for Stage/Step names & mappings are welcome!

bparees · 2017-03-17T18:13:52Z

pkg/build/api/types.go

+	StartTime unversioned.Time
+
+	// EndTime identifies when this stage ended.
+	EndTime unversioned.Time


drop endtime

bparees · 2017-03-17T18:15:16Z

pkg/build/api/types.go

+	StartTime unversioned.Time
+
+	// EndTime identifies when this step completed.
+	EndTime unversioned.Time


drop endtime

bparees · 2017-03-17T18:16:00Z

pkg/build/api/types.go

+	StepFetchSource StepName = "FetchSource"
+
+	// StepDockerPullImage pulls the docker image.
+	StepDockerPullImage StepName = "DockerPullImage"


should have different step names for the different images we pull (builder image, base image, input image, etc)

bparees · 2017-03-17T18:16:16Z

pkg/build/api/types.go

+// Valid StageNames
+const (
+	// StageFetch fetches the source. 
+	StageFetch StageName = "Fetch"


FetchInputs

bparees · 2017-03-17T18:17:10Z

pkg/build/api/types.go

+	StageBuild StageName = "Build"
+
+	// StagePost executes post steps.
+	StagePost StageName = "Post"


PostCommit ?

bparees · 2017-03-17T18:17:20Z

pkg/build/api/types.go

+	StagePost StageName = "Post"
+
+	// StagePushImages pushes the docker images.
+	StagePushImages StageName = "PushImages"


PushImage (there's only one)

coreydaley · 2017-03-20T16:36:57Z

@bparees
Updated per your suggestions, ptal

bparees

I think this is close enough that you should start working on tests.

bparees · 2017-03-21T18:05:07Z

@openshift/api-review I think the api is ready for another look. We've roughly mirrored the jenkins stage/step model of having start time + duration for each stage+step.

coreydaley · 2017-03-21T20:14:02Z

@bparees ptal
Added tests and bumped the vendored s2i

coreydaley · 2017-03-22T00:41:41Z

[test]

coreydaley · 2017-03-22T00:43:18Z

[testextended][extended:core(builds)]

smarterclayton · 2017-03-22T16:31:43Z

pkg/build/api/v1/types.go

+}
+
+// Stages is a slice of build stages that have been recorded.
+type Stages []StageInfo


Generally we don't do this unless there's a reason that has to do with protobuf serialization (null vs empty behavior), and that doesn't apply here. Recommend removing this alias type.

smarterclayton · 2017-03-22T16:32:28Z

pkg/build/api/v1/types.go

+	StartTime unversioned.Time `json:"startTime,omitempty" protobuf:"bytes,2,opt,name=startTime"`
+
+	// Duration identifies how long this stage ran.
+	Duration unversioned.Duration `json:"duration,omitempty" protobuf:"bytes,3,opt,name=duration"`


Duration is not allowed in the public API. You must use DurationMilliseconds

And an int64

we're using time.Duration in the BuildStatus, is not not acceptable?

Duration time.Duration json:"duration,omitempty" protobuf:"varint,7,opt,name=duration,casttype=time.Duration"

and @smarterclayton are you saying the value must be stored as milliseconds?

Yes, the field must be:

DurationMilliseconds int64 `json:"durationMilliseconds"`

and when you store it you need to do the conversion to milliseconds.

smarterclayton · 2017-03-22T16:55:48Z

pkg/build/api/v1/types.go

+	StepPullInputImage StepName = "PullInputImage"
+
+	// StepDockerPushImage pushes the docker image to the registry.
+	StepPushDockerImage StepName = "PushDockerImage"


PushImage instead of PushDockerImage. Only reference Docker directly if it is relevant to the inputs (i.e. Docker strategy is compatible with docker build and so it's appropriate) - otherwise use the generic Image.

smarterclayton · 2017-03-22T16:56:17Z

pkg/build/api/v1/types.go

+// Valid StepNames
+const (
+	// StepExecPostCommitHook executes the buildconfigs post commit hook.
+	StepExecPostCommitHook StepName = "ExecPostCommitHook"


Rather than Exec, why not RunPostCommitHook? Reads better at least to me.

smarterclayton · 2017-03-22T16:56:35Z

pkg/build/api/v1/types.go

+// StageInfo contains details about a build stage.
+type StageInfo struct {
+	// StageName is the identifier for each build stage.
+	StageName StageName `json:"stageName,omitempty" protobuf:"bytes,1,opt,name=stageName"`


Why not just Name? Will a Stage ever have a different Name?

we have constants defined like StageFetchInputs StageName = "FetchInputs",
so "Name" would be pretty generic there?

change the field name, not the type name.

smarterclayton · 2017-03-22T16:57:18Z

pkg/build/api/v1/types.go

+// StepInfo contains details about a build step.
+type StepInfo struct {
+	// StepName is the identifier for each build step.
+	StepName StepName `json:"stepName,omitempty" protobuf:"bytes,1,opt,name=stepName"`


Name instead of StepName unless we can make an argument that there will ever be a more basic "name" for a step that isn't this field. Basically build.status.steps[0].name reads better than build.status.steps[0].stepName.

We have constants defined for StepName, so using just Name seems to generic there and would overlap the StageName

he's talking about the field name, not the type name.

jim-minter · 2017-03-28T13:50:29Z

pkg/build/api/helpers.go

+		if stageVal.Name == stageName {
+			stages[stageKey].DurationMilliseconds = endTime.Rfc3339Copy().Time.Sub(stages[stageKey].StartTime.Rfc3339Copy().Time).Nanoseconds() / int64(time.Millisecond)
+			if len(stages[stageKey].Steps) == 0 {
+				stages[stageKey].Steps = make([]StepInfo, 0)


And again...

We are only displaying using second-level precision, I was using that so we don't get something like 2.4738495747s in the display.

Corrected in next push

jim-minter · 2017-03-28T13:57:28Z

pkg/build/api/helpers.go

+		stages = make([]StageInfo, 0)
+	}
+
+	// If the stage already exists in the slice, update the endTime and DurationMilliseconds, and append the new step.


Comment needs an update.

Thanks, will push an update

jim-minter · 2017-03-28T14:11:06Z

pkg/build/api/helpers.go

+}
+
+// MergeStageAndStepInfo merges one Stages slice into another.
+func MergeStageAndStepInfo(stages []StageInfo, stagesToMerge []StageInfo) ([]StageInfo, int64) {


I think this function is quite brittle - if for some reason in the future this is called with two []StageInfos which each contain a StageInfo with an identical name, but different StartTimes, I think bad things could happen.
Is it possible to get rid of this function entirely by passing a *StageInfo argument into functions like fetchSource, and not increasing the number of return values? I think that would be cleaner if feasible.

I'm not sure i love carrying a StageInfo object around through all our build calls (as opposed to letting the calls return the stages they created and merging them in a common place) but i agree there's a risk of breakage here.

Perhaps we could compromise on having this function throw an error if the merge includes the same stage name twice w/ different start times? (if we are using this properly, i don't think we have that scenario)

alternatively we could always use the earliest start time for the stage if the stage appears twice, which also seems like reasonable behavior.

The way this works is that Stages are like categories, and Steps are like items in those categories. The first Step that is added sets the start time for the Stage, and each step that is added extends the duration to that Step's stop time. So the question would seem to be, what if a second Step with the same name is added? Should we replace the first one? Go ahead and add this second Step to the Stage? or throw an error?

We shouldn't ever get a duplicate step, but if we did, it would not hurt anything, it would just dump it into the Step slice under the correct Stage and keep on chugging along.

I don't think that's @jim-minter's concern. His concern is w/ regard to the behavior when you have duplicate stage names. (but duplicate step names within a stage could be a problem too, certainly, depending how those are being merged)

jim-minter · 2017-03-28T14:12:38Z

pkg/build/api/helpers.go

+	for _, stage := range stagesToMerge {
+		for _, step := range stage.Steps {
+			stages = RecordStageAndStepInfo(stages, stage.Name, step.Name, step.StartTime, unversioned.NewTime(step.StartTime.Add(time.Duration(step.DurationMilliseconds)*time.Millisecond)))
+			subStepDuration = subStepDuration + step.DurationMilliseconds


I think the fact that you're returning subStepDuration is indicative of an encapsulation problem. I think that any code that uses the returned subStepDuration should be pulled out to a helper.

i think there's a more fundamental issue in thinking we even need this information, i commented on the relevant code below.

Corrected in next push

jim-minter · 2017-03-28T14:20:18Z

pkg/build/builder/docker.go

@@ -66,7 +68,14 @@ func (d *DockerBuilder) Build() error {
 	if err != nil {
 		return err
 	}
-	sourceInfo, err := fetchSource(d.dockerClient, buildDir, d.build, initialURLCheckTimeout, os.Stdin, d.gitClient)
+	startTime := unversioned.Now()


@bparees I still think that we should be using something like a helper function and closure, for readability and maintainability, to avoid blow-up of 1 line to 8 at measurement points.

well i think this block is more complicated than it should be. it should just need to be:

getStartTime
runStepLogic
merge(?)
recordStep/Stage

As it stands, it's 7 lines and i think at least 3 of these lines are unnecessary(variable declaration, storing stopTime temporarily, and the logic related to subStepDuration), and most invocations wouldn't have the merge step (and the closure wouldn't help with the merge step, i don't think)

In any case, I'm still not convinced creating closures is going to result in more readability (or reduced linecounts), since it means creating wrapper functions around the blocks of code we want to measure, blocks that otherwise serve no purpose, in exchange for a slightly more awkward callstack and return path.

bparees · 2017-03-28T17:03:44Z

pkg/build/builder/docker.go

+	stopTime := unversioned.Now()
+	var subStepDuration int64
+	d.build.Status.Stages, subStepDuration = api.MergeStageAndStepInfo(d.build.Status.Stages, stages)
+	stepDuration := int64(stopTime.Time.Sub(startTime.Time)) - subStepDuration


at a minimum this needs some comments but fundamentally it seems like we're doing more manipulation than i'd expect...what's going on here?

it seems like you're treating "fetchSource()" as a step, but fetchSource() itself also has steps. i'd expect one or the other (and i'd expect it to be the latter).

Corrected in next push

bparees · 2017-03-28T17:06:09Z

pkg/build/builder/sti.go

+	var subStepDuration int64
+	s.build.Status.Stages, subStepDuration = api.MergeStageAndStepInfo(s.build.Status.Stages, stages)
+	stepDuration := int64(stopTime.Time.Sub(startTime.Time)) - subStepDuration
+	s.build.Status.Stages = api.RecordStageAndStepInfo(s.build.Status.Stages, api.StageFetchInputs, api.StepFetchSource, startTime, unversioned.NewTime(startTime.Add(time.Duration(stepDuration))))


same issue here.

Corrected in next push

bparees · 2017-03-28T17:34:53Z

pkg/build/builder/docker.go

+	startTime := unversioned.Now()
+	sourceInfo, stages, err := fetchSource(d.dockerClient, buildDir, d.build, initialURLCheckTimeout, os.Stdin, d.gitClient)
+	stopTime := unversioned.Now()
+	var subStepDuration int64


you don't need to declare this, you can just use := below (assuming this value even sticks around, but per my other comments i hope it doesn't)

Corrected in next push

coreydaley · 2017-04-11T20:31:01Z

@bparees ptal

bparees · 2017-04-11T21:29:37Z

test/extended/builds/build_timing.go

+		verifyStages(br.Build.Status.Stages, expectedBuildStages)
+
+	})
+	g.It("should fail if a duplicate step is added", func() {


this should be a unit test on the timing package.

updated in next push

coreydaley · 2017-04-12T15:48:53Z

@bparees updated per your suggestions, ptal

coreydaley · 2017-04-13T13:01:01Z

test flake #13739

coreydaley · 2017-04-13T20:16:50Z

test flake #11114

coreydaley · 2017-04-14T15:20:29Z

@bparees ptal, looks like it was just a couple of test flakes (as noted above)

coreydaley · 2017-04-17T03:01:27Z

test flake #13694

bparees · 2017-04-17T20:33:25Z

pkg/build/api/helpers.go

+}
+
+// AppendStageAndStepInfo appends the step info from one stages slice into another.
+func AppendStageAndStepInfo(stages []StageInfo, stagesToMerge []StageInfo) ([]StageInfo, error) {


is stages=append(stages,otherstages) not sufficient for your use case? (do you actually have a use case where "stages" and "otherstages" have an overlap in stage names and you need to merge things at a step level?)

Yes, it is possible

i'm not sure "it is possible" answers my question.

is the code currently doing so?

if not, do you have an explicit scenario in mind where it would?

Both the origin code and the source-to-image code record steps in the StagePullImages stage, so those would need to be merged together. There is also the potential as we record more fine grained details that there will be more steps recorded for each stage, again needing to be merged instead of just appended.

bparees · 2017-04-17T20:43:56Z

pkg/build/builder/sti.go

+				return err
+			}
+		}
+	}


why isn't this using your merge function?

The stages/steps returned from the s2i code is the wrong type. I guess I could just cast everything in the merge function.

bparees · 2017-04-17T20:44:34Z

pkg/build/builder/sti.go

@@ -283,8 +297,17 @@ func (s *S2IBuilder) Build() error {
 		handleBuildStatusUpdate(s.build, s.client, nil)
 		return err


shouldn't you add the stage info up here, before returning?

bparees · 2017-04-17T20:45:19Z

pkg/build/builder/sti.go

@@ -293,6 +316,11 @@ func (s *S2IBuilder) Build() error {
 		return err


same here...add the stage info before returning. I still care how long my postcommithook took to run, even if it failed.

bparees · 2017-04-17T20:50:32Z

pkg/build/builder/docker.go

 			if err = pullImage(d.dockerClient, imageName, pullAuthConfig); err != nil {
 				d.build.Status.Phase = api.BuildPhaseFailed
 				d.build.Status.Reason = api.StatusReasonPullBuilderImageFailed
 				d.build.Status.Message = api.StatusMessagePullBuilderImageFailed
 				handleBuildStatusUpdate(d.build, d.client, nil)
 				return fmt.Errorf("failed to pull image: %v", err)
 			}
+			if d.build.Status.Stages, err = api.RecordStageAndStepInfo(d.build.Status.Stages, api.StagePullImages, api.StepPullBaseImage, startTime, unversioned.Now()); err != nil {


you've got the context object, why not consistently call timing.RecordNewStep everywhere? It's a bit confusing having two mechanisms for recording new steps being used in different places.

bparees · 2017-04-17T20:54:37Z

test/extended/builds/build_timing.go

+		expectedDurations, ok := expectedStages[string(stage.Name)]
+		if !ok {
+			fmt.Fprintf(g.GinkgoWriter, "Unexpected stage %v was encountered", stage.Name)
+			o.Expect(ok).To(o.BeTrue())


when calling gomega from within a helper function, use ExpectWithOffset so the error reporting shows you who the actual caller.

bparees · 2017-04-17T20:55:02Z

test/extended/builds/build_timing.go

+		if expectedDurations[0] != "" {
+			expectedMinDuration, _ := time.ParseDuration(expectedDurations[0])
+			if stage.DurationMilliseconds < expectedMinDuration.Nanoseconds()/int64(time.Millisecond) {
+				fmt.Fprintf(g.GinkgoWriter, "Stage %v ran for %v, expected greater than %v", stage.Name, stage.DurationMilliseconds, expectedMinDuration)


should this be a failure?

bparees · 2017-04-17T20:55:09Z

test/extended/builds/build_timing.go

+		}
+		expectedMaxDuration, _ := time.ParseDuration(expectedDurations[1])
+		if stage.DurationMilliseconds > expectedMaxDuration.Nanoseconds()/int64(time.Millisecond) {
+			fmt.Fprintf(g.GinkgoWriter, "Stage %v ran for %v, expected less than %v", stage.Name, stage.DurationMilliseconds, expectedMaxDuration)


should this be a failure?

coreydaley · 2017-04-21T14:25:46Z

@bparees ptal, updated per your suggestions

bparees · 2017-04-21T16:57:11Z

pkg/build/builder/sti.go

+	defer func() {
+		s.build.Status.Stages = api.AppendStageAndStepInfo(s.build.Status.Stages, timing.GetStages(ctx))
+		handleBuildStatusUpdate(s.build, s.client, nil)
+


delete this newline.

openshift-bot · 2017-04-24T22:45:35Z

Evaluated for origin testextended up to 915246a

openshift-bot · 2017-04-25T00:29:40Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/222/) (Base Commit: 184b859) (Extended Tests: core(builds))

openshift-bot · 2017-04-25T01:21:32Z

Evaluated for origin test up to 915246a

openshift-bot · 2017-04-25T02:41:46Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/912/) (Base Commit: 092bd57)

coreydaley · 2017-04-25T13:39:12Z

@bparees ptal

bparees · 2017-04-25T21:55:21Z

[merge]

smarterclayton · 2017-04-25T23:47:19Z

Queue is being held for rebase

bparees · 2017-04-25T23:59:43Z

Queue is being held for rebase

i know..I assume you guys are using some alternate tag for merging and I can just leave this tag here for when you re-enable the queue for the rest of us?

openshift-bot · 2017-04-26T20:29:30Z

Evaluated for origin merge up to 915246a

openshift-bot · 2017-04-27T10:05:41Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/486/) (Base Commit: 33052f6) (Image: devenv-rhel7_6183)

coreydaley changed the title ~~(WIP) add build substatus~~ add build substatus Mar 13, 2017

coreydaley changed the title ~~add build substatus~~ Add build timing information Mar 13, 2017

coreydaley changed the title ~~Add build timing information~~ Gather build step timing information and output using oc describe Mar 13, 2017

coreydaley mentioned this pull request Mar 13, 2017

Gather build step timing information openshift/source-to-image#694

Merged

csrwng reviewed Mar 13, 2017

View reviewed changes

smarterclayton added the needs-api-review label Mar 15, 2017

smarterclayton reviewed Mar 15, 2017

View reviewed changes

bparees requested changes Mar 17, 2017

View reviewed changes

bparees requested changes Mar 20, 2017

View reviewed changes

smarterclayton reviewed Mar 22, 2017

View reviewed changes

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

coreydaley removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2017

jim-minter reviewed Mar 28, 2017

View reviewed changes

jim-minter suggested changes Mar 28, 2017

View reviewed changes

bparees reviewed Mar 28, 2017

View reviewed changes

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

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

bparees reviewed Apr 11, 2017

View reviewed changes

bparees requested changes Apr 17, 2017

View reviewed changes

bparees reviewed Apr 21, 2017

View reviewed changes

add build substatus

915246a

openshift-bot merged commit 10c7a97 into openshift:master Apr 27, 2017

		@@ -283,8 +297,17 @@ func (s *S2IBuilder) Build() error {
		handleBuildStatusUpdate(s.build, s.client, nil)
		return err

		@@ -293,6 +316,11 @@ func (s *S2IBuilder) Build() error {
		return err

Gather build step timing information and output using oc describe #13307

Gather build step timing information and output using oc describe #13307

Conversation

coreydaley commented Mar 8, 2017 • edited Loading

coreydaley commented Mar 8, 2017

coreydaley commented Mar 13, 2017 • edited Loading

gabemontero commented Mar 13, 2017

coreydaley commented Mar 13, 2017

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

coreydaley commented Mar 14, 2017 • 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

smarterclayton commented Mar 17, 2017 via email

coreydaley commented Mar 17, 2017 • 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

coreydaley commented Mar 20, 2017

bparees left a comment

Choose a reason for hiding this comment

bparees commented Mar 21, 2017

coreydaley commented Mar 21, 2017

coreydaley commented Mar 22, 2017

coreydaley commented Mar 22, 2017

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

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

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

coreydaley commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Apr 12, 2017

coreydaley commented Apr 13, 2017

coreydaley commented Apr 13, 2017

coreydaley commented Apr 14, 2017

coreydaley commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Mar 8, 2017 •

edited

Loading

coreydaley commented Mar 13, 2017 •

edited

Loading

coreydaley commented Mar 14, 2017 •

edited

Loading

coreydaley commented Mar 17, 2017 •

edited

Loading