Add Pipeline-level timeouts #355

abayer · 2018-12-27T20:44:00Z

This allows configuring Pipeline timeouts that will be enforced for their TaskRuns appropriately as well, and also replicates Build's timeout behavior on TaskRun, since that didn't actually get moved over here earlier.

Fixes #222

abayer · 2018-12-27T20:44:36Z

If someone with the appropriate privileges could label this as work in progress, I'd appreciate it. =)

vdemeester · 2018-12-28T09:49:06Z

@abayer if you put WIP in the title (prefixing the rest), it should automatically do that 😉

vdemeester

Few comments, didn't dig "too" deep as it's still wip 👼
/hold

vdemeester · 2018-12-28T13:23:23Z

pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go

@@ -17,12 +17,15 @@ limitations under the License.
 package v1alpha1

 import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


Should be separate import

"testing" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

vdemeester · 2018-12-28T13:24:50Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

@@ -238,15 +238,16 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er

 	if rprt != nil {
 		c.Logger.Infof("Creating a new TaskRun object %s", rprt.TaskRunName)
-		rprt.TaskRun, err = c.createTaskRun(c.Logger, rprt.ResolvedTaskResources.TaskName, rprt.TaskRunName, pr, rprt.PipelineTask, serviceAccount)
+		rprt.TaskRun, err = c.createTaskRun(c.Logger, rprt.ResolvedTaskResources.TaskName, rprt.TaskRunName, pr, rprt.PipelineTask, serviceAccount, p.Spec.Timeout)


nit: we really need to refactor this createTaskRun method 👼
(but it's not the PR's role to do it 👼 )

vdemeester · 2018-12-28T13:26:19Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -44,6 +42,8 @@ import (
 	coreinformers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/cache"
+	"reflect"


Same as before for the imports 👼

vdemeester · 2018-12-28T13:27:58Z

test/builder/pipeline.go

@@ -97,6 +98,13 @@ func PipelineTask(name, taskName string, ops ...PipelineTaskOp) PipelineSpecOp {
 	}
 }

+// PipelineTimeout sets the timeout to the PipelineSpec.
+func PipelineTimeout(duration *metav1.Duration) PipelineSpecOp {


I wonder why PipelineTimeout takes a metav1.Duration but PipelineRunStartTime task a time.Time — aka why not using time.Duration on PipelineTimeout ?

No actual good reason - just the hodgepodge of things I derived from. i.e., Build's timeout is a metav1.Duration, so I assumed I should do the same thing here.

shashwathi

Great PR and awesome work. I have some minor feedback with code but here is my concern in terms of design

If user configures timeout in pipelinerun, then all corresponding taskruns created should include parent object timeout
This PR is doing this (👍 ) but logic seems a bit complex
if pipelinerun creates a taskrun then taskrun timeout duration should be set to (pipelinerun start time + timeout duration) - current time.
You could set this time to Taskspec.Timeout and do not need new spec variable. Logic is already present in taskrun to check for this timeout.
This PR introduces additional timeout in taskrun spec and its confusing for a user who is building just taskrun yaml. Why would I use one over another? and why do we need 2 timeouts ?

shashwathi · 2019-01-07T20:55:51Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -106,6 +107,12 @@ type PipelineRunStatus struct {
 	// In #107 should be updated to hold the location logs have been uploaded to
 	// +optional
 	Results *Results `json:"results,omitempty"`
+	// StartTime is the time the build is actually started.


Typo: I think you meant "PipelineRun" here

shashwathi · 2019-01-07T20:56:01Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

+	// StartTime is the time the build is actually started.
+	// +optional
+	StartTime *metav1.Time `json:"startTime,omitempty"`
+	// CompletionTime is the time the build completed.


shashwathi · 2019-01-07T21:23:53Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

@@ -51,6 +51,9 @@ type TaskRunSpec struct {
 	TaskRef *TaskRef `json:"taskRef,omitempty"`
 	//+optional
 	TaskSpec *TaskSpec `json:"taskSpec,omitempty"`
+	// PipelineRunTimeout is the time at which the entire PipelineRun times out. Defaults to nil.
+	// +optional
+	PipelineRunTimeout *metav1.Time `json:"pipelineRunTimeout,omitempty"`


I am not certain about the name of this variable to be "PipelineRunTimeout". TaskRun should not be referencing pipelinerun is names. Why not timeout ?

Yeah, nuked all that from orbit.

shashwathi · 2019-01-07T21:46:49Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	var after *duckv1alpha1.Condition
+	before := tr.Status.GetCondition(duckv1alpha1.ConditionSucceeded)
+
+	if !tr.Status.StartTime.IsZero() && tr.Spec.PipelineRunTimeout != nil {


I liked the pattern in pipelinerun controller where InitializeConditions function took care of initializing StartTime as well. I would recommend doing same here.

nader-ziada

There is already a timeout in the taskrun from the TaskSpec. Currently there are two ways to get the task spec for the taskrun, either directly from the taskSpec field or from the Task referenced from the taskRef field (taskSpec and taskRef are mutually exclusive). So I don't think its necessary to define another field on the task level

The pipeline timeout should also override the task(s) timeouts, but take into account the actual time used in a task, for example if task one has a timeout of 5 seconds but actually is completed in 2 seconds, these 3 seconds should be added to the following task while respecting the pipeline timeout

abayer · 2019-01-18T17:58:03Z

I'm going to pick this back up next week - hopefully #381 gets merged soon so I can redo some aspects of this on top of it. =)

abayer · 2019-01-22T21:42:19Z

#381 is merged, I've rebased this, and tomorrow I'll be working on responding to feedback. =)

abayer · 2019-01-22T21:59:43Z

...and yes, the tests currently hang. Will fix tomorrow.

bobcatfish

Really coming along!! I left a smattering of feedback, nothing too serious

Main thoughts:

How does it work after a PipelineRun times out, how do we know any TaskRuns it created will also time out?
Could you add some docs as well (maybe in using.md?) on how this will work

❤️

bobcatfish · 2019-01-22T22:29:42Z

pkg/apis/pipeline/v1alpha1/pipeline_types.go

-	Generation int64          `json:"generation,omitempty"`
+	Tasks []PipelineTask `json:"tasks"`
+	// Time after which the build times out. Defaults to never.
+	// Specified build timeout should be less than 24h.


is there a specific reason why less than 24h? (e.g. something we can link to) otherwise i wonder if we really should set an upper bound at all 🤔 people might want to do some crazy things

(ps. maybe update the comment here to say "pipeline" instead of "build"?)

Good question - I just assumed we'd want a max, but I'll yank it out for now.

bobcatfish · 2019-01-22T22:30:46Z

pkg/apis/pipeline/v1alpha1/pipeline_validation.go

+
+	if ps.Timeout != nil {
+		// timeout should be a valid duration of between 0 and 24 hours.
+		maxTimeout := time.Duration(24 * time.Hour)


i think declaring this at the top of the file as a var is consistent with how we declare other values like this

could even be a constant 👼

I've yanked this out anyway. =)

bobcatfish · 2019-01-22T22:32:28Z

pkg/apis/pipeline/v1alpha1/taskrun_validation.go

+		} else if ts.PipelineRunTimeout.Time.Before(time.Now()) {
+			return apis.ErrInvalidValue(fmt.Sprintf("Time from now until timeout %s should be >= 0", ts.PipelineRunTimeout.Sub(time.Now()).Truncate(time.Second).String()), "spec.pipelineruntimeout")
+		}
+	}


could this logic be shared with the pipeline logic? e.g. moved into a module they can both call (i think it's the same)

So after removing PipelineRunTimeout, this goes away too, so hey. =)

bobcatfish · 2019-01-22T22:33:39Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

+	// If a timeout is specified for the PipelineRun, set the timeout for the task to be the time from now until the
+	// PipelineRun timeout would occur.
+	return &metav1.Time{Time: pr.Status.StartTime.Add(pipelineTimeout.Duration)}
+}


i like that this function doesn't depend on the reconciler at all! and b/c of that, we could easily move it into a separate module, and even have a unit test or two for it :)

May even be attached to PipelineRun struct 👼

aaaaaand I've gotten rid of this and just moved the equivalent logic within createTaskRun, but I can extract it again if you'd like?

bobcatfish · 2019-01-22T22:34:27Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go

+			tb.PipelineRunServiceAccount("test-sa"),
+		),
+		tb.PipelineRunStatus(
+			tb.PipelineRunStartTime(time.Now().AddDate(0, 0, -1))),


bobcatfish · 2019-01-22T22:36:17Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

+		if !startTime.IsZero() && pipelineTimeout != nil {
+			timeout := pipelineTimeout.Duration
+			runtime := time.Since(startTime.Time)
+			if runtime > timeout {


this logic to determine if a timeout has occurred seems to me like a good candidate for moving into a separate function with some tests (e.g. hasTimedOut)

bobcatfish · 2019-01-22T22:36:56Z

pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go

+					Status:  corev1.ConditionFalse,
+					Reason:  ReasonTimedOut,
+					Message: timeoutMsg,
+				}


after this happens, does anything need to be done to make sure the taskruns that the pipelinerun spawned will also time out?

Hum yeah I think we need to cancel the TaskRuns there 👼

The TaskRuns should themselves time out now.

@abayer oh right, TaskRuns will also have the timeout field, and thus should time out on their own in the same time more or less, right ?

But I don't see the code for that though 🤔 Am I missing something ?

@vdemeester See the changes tocreateTaskRun in https://github.com/knative/build-pipeline/pull/355/files#diff-225ed18b17b573cba99d592dea1da1a0R308

@abayer nevermind, I was looking in the diff for support for Timeout on Task.… but it's already supported (because of build) 😅

bobcatfish · 2019-01-22T22:39:09Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

 		if err != nil {
 			c.Recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunCreationFailed", "Failed to create TaskRun %q: %v", rprt.TaskRunName, err)
 			return fmt.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %s", rprt.TaskRunName, rprt.PipelineTask.Name, pr.Name, err)
 		}
 	}

 	before := pr.Status.GetCondition(duckv1alpha1.ConditionSucceeded)
-	after := resources.GetPipelineConditionStatus(pr.Name, pipelineState, c.Logger)
+	after := resources.GetPipelineConditionStatus(pr.Name, pipelineState, c.Logger, pr.Status.StartTime, p.Spec.Timeout)


i have this sneaking suspicion like it'd be a bit cleaner if we checked for the timeout after this call, instead of inside it, since it seems like the logic needs different inputs (i.e. start time and timeout)

but i dont feel too strongly, also seems weird to determine the condition should be X and then decide afterward it should actually be Y

Yeah, this needs more thought.

bobcatfish · 2019-01-22T22:39:40Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	before := tr.Status.GetCondition(duckv1alpha1.ConditionSucceeded)
+
+	if !tr.Status.StartTime.IsZero() && tr.Spec.PipelineRunTimeout != nil {
+		if time.Now().After(tr.Spec.PipelineRunTimeout.Time) {


could both controller share the timeout logic?

It's been yanked out here and is just using Build's timeout.

bobcatfish · 2019-01-22T22:40:50Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go


-	before := tr.Status.GetCondition(duckv1alpha1.ConditionSucceeded)
+	// TODO: Skipping all things pod/build-related if we've timed out, but this probably should be better.
+	if after == nil {


i feel like after == nil is a bit hard to understand, i wonder if there's some way this could be something like:

if hasTimedOut() { // do timeout stuff } else { // do all this pod stuff }

or even like

if hasTimedOut() { // set some status stuff // return } do pod stuff

And this has all been yanked. =)

abayer · 2019-01-23T13:59:29Z

@pivotal-nader-ziada @shashwathi - Ah, I hadn't realized it was possible to use TaskRunSpec.TaskSpec directly. I guess I'd need to change the TaskRun creation to populate the TaskSpec rather than use TaskRef - perhaps just when there's a PipelineRun timeout in effect? It also looks like that would result in the taskName used in the reconciler being the TaskRun name, not the Task name...which, I think, makes sense, since I'm assuming this capability is here in the first place to allow for creating a TaskRun without a pre-existing Task to go with it. (which also makes me wonder if we might want to have a similar thing for PipelineRun, but I'll get back to that another time!)

abayer · 2019-01-23T17:00:33Z

/hold

abayer · 2019-01-23T17:03:52Z

...ok, I have no idea how to get rid of the hold label. =)

shashwathi · 2019-01-23T17:07:33Z

/hold cancel

abayer · 2019-01-23T17:08:19Z

Ah-ha! Thanks, @shashwathi!

bobcatfish · 2019-01-28T14:40:03Z

docs/using.md

+spec:
+  timeout: 5m
+```
+


bobcatfish · 2019-01-28T14:45:04Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -528,3 +542,37 @@ func makeLabels(s *v1alpha1.TaskRun) map[string]string {
 func isDone(status *v1alpha1.TaskRunStatus) bool {
 	return !status.GetCondition(duckv1alpha1.ConditionSucceeded).IsUnknown()
 }
+
+func (c *Reconciler) checkTimeout(tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec) (bool, error) {


if you pass the Delete function into here, you can move this function into a separate package and have unit tests for it without involving the reconciler :D (for example see ResolveTaskResources)

Well, mostly done. Finishing now.

I take that back - I'm not sure where to move this function to. =)

aw well, we can move it later :)

i would have maybe made a new package in https://github.com/knative/build-pipeline/tree/master/pkg/reconciler/v1alpha1/taskrun called timeout or something

nader-ziada · 2019-01-29T12:38:28Z

/test pull-knative-build-pipeline-integration-tests

Fixes tektoncd#222

abayer · 2019-01-29T17:39:36Z

Ok, fixed TestTaskRunTimeout after rebasing - I think everything should pass now...

knative-metrics-robot · 2019-01-29T17:39:49Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	91.7%	92.2%	0.5
pkg/apis/pipeline/v1alpha1/taskrun_types.go	63.6%	53.8%	-9.8
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	81.3%	82.6%	1.3
pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go	93.0%	88.5%	-4.5
pkg/reconciler/v1alpha1/taskrun/taskrun.go	72.3%	73.0%	0.8
test/builder/pipeline.go	94.7%	95.0%	0.3
test/builder/task.go	99.4%	98.2%	-1.2

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

abayer · 2019-01-29T20:18:52Z

@pivotal-nader-ziada @shashwathi @bobcatfish @tejal29 @vdemeester @imjasonh - sorry to ping you all directly, but I just wanted to see if there's anything else I need to work on here or if this is good to go? Thanks!

vdemeester

Code wise LGTM 👍
Related to knative/build#543 and slack discussion on handling timeout "out of resync", I wonder if we should go with that for the time being and refactor it once we find a better solution.

cc @imjasonh

abayer · 2019-01-30T11:55:13Z

@vdemeester Yeah, that’s my thinking - the bulk of this won’t change in that scenario, just the encapsulated checkTimeout logic, so it shouldn’t be hard to refactor in the future.

vdemeester

/lgtm

imjasonh · 2019-01-30T20:21:30Z

/approve

🎉

knative-prow-robot · 2019-01-30T20:21:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, ImJasonH, vdemeester

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~~ [ImJasonH]

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

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

We were passing the original request where the body had been drained to the first interceptor meaning that the body to the first interceptor in the chain was always empty. Further, we were using the `GetBody` which can sometimes be empty leading to a panic. fixes tektoncd#355 Signed-off-by: Dibyo Mukherjee <[email protected]>

knative-prow-robot requested review from nader-ziada and srinivashegde86 December 27, 2018 20:44

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 27, 2018

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2018

vdemeester reviewed Dec 28, 2018

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 28, 2018

abayer changed the title ~~Add Pipeline-level timeouts~~ WIP: Add Pipeline-level timeouts Dec 31, 2018

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 31, 2018

abayer mentioned this pull request Jan 7, 2019

Add possibility for a client to cancel a build… knative/build#510

Merged

shashwathi reviewed Jan 7, 2019

View reviewed changes

nader-ziada reviewed Jan 8, 2019

View reviewed changes

abayer force-pushed the add-timeout-to-pipeline branch from ab4a51c to 5acbad6 Compare January 22, 2019 21:41

bobcatfish reviewed Jan 22, 2019

View reviewed changes

abayer force-pushed the add-timeout-to-pipeline branch from a632f7a to b945146 Compare January 23, 2019 15:41

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 23, 2019

abayer changed the title ~~WIP: Add Pipeline-level timeouts~~ Add Pipeline-level timeouts Jan 23, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2019

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2019

bobcatfish reviewed Jan 28, 2019

View reviewed changes

docs/using.md

spec:

timeout: 5m

```

Copy link

Collaborator

bobcatfish Jan 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 nice!

bobcatfish reviewed Jan 28, 2019

View reviewed changes

abayer force-pushed the add-timeout-to-pipeline branch from 9b23a4b to 7a87112 Compare January 28, 2019 15:26

Add Pipeline-level timeouts, implement Task-level timeouts

ce606dd

Fixes tektoncd#222

abayer force-pushed the add-timeout-to-pipeline branch from 08c6aaf to ce606dd Compare January 29, 2019 17:38

abayer mentioned this pull request Jan 29, 2019

Design and implement retries #221

Closed

abayer added a commit to abayer/jx that referenced this pull request Jan 29, 2019

Add timeout support

a499fa9

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

abayer mentioned this pull request Jan 29, 2019

Add timeout support abayer/jx#4

Merged

vdemeester reviewed Jan 30, 2019

View reviewed changes

vdemeester approved these changes Jan 30, 2019

View reviewed changes

knative-prow-robot assigned vdemeester Jan 30, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019

vdemeester mentioned this pull request Jan 30, 2019

Handle build/taskrun timeout outside of resync #456

Closed

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019

knative-prow-robot merged commit 23a2b91 into tektoncd:master Jan 30, 2019

abayer added a commit to abayer/jx that referenced this pull request Jan 31, 2019

Add timeout support

fa0c817

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

abayer added a commit to abayer/jx that referenced this pull request Feb 1, 2019

Add timeout support

12f901e

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

abayer added a commit to abayer/jx that referenced this pull request Feb 4, 2019

Add timeout support

8795538

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

abayer added a commit to abayer/jx that referenced this pull request Feb 6, 2019

Add timeout support

8dec05f

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

joseblas pushed a commit to abayer/jx that referenced this pull request Feb 7, 2019

Add timeout support

0de22bc

Waiting on tektoncd/pipeline#355 to be merged to add top-level timeout.

mchmarny unassigned vdemeester and tejal29 Mar 7, 2019

Add Pipeline-level timeouts #355

Add Pipeline-level timeouts #355

Conversation

abayer commented Dec 27, 2018 • edited Loading

abayer commented Dec 27, 2018

vdemeester commented Dec 28, 2018

vdemeester 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

shashwathi 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

nader-ziada left a comment

Choose a reason for hiding this comment

abayer commented Jan 18, 2019

abayer commented Jan 22, 2019

abayer commented Jan 22, 2019

bobcatfish 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

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

abayer commented Jan 23, 2019

abayer commented Jan 23, 2019

abayer commented Jan 23, 2019

shashwathi commented Jan 23, 2019

abayer commented Jan 23, 2019

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

nader-ziada commented Jan 29, 2019

abayer commented Jan 29, 2019

knative-metrics-robot commented Jan 29, 2019

abayer commented Jan 29, 2019

vdemeester left a comment

Choose a reason for hiding this comment

abayer commented Jan 30, 2019

vdemeester left a comment

Choose a reason for hiding this comment

imjasonh commented Jan 30, 2019

knative-prow-robot commented Jan 30, 2019

abayer commented Dec 27, 2018 •

edited

Loading