add taskrun controller implementation #86

aaron-prindle · 2018-10-02T18:54:58Z

this pr implements a simple taskrun controller that creates a build and updates the taskrun status to reflect the build status. currently there are no tests around the controllers and this piece is not fully tested.

bobcatfish

EXCITING!!

my main piece of feedback is id like to see this go in without most of the status checking, maybe just create when needed, delete when needed (or just create), and deal with status/conditions in the next iteration

currently there are no tests around the controllers and this piece is not fully tested.

once we have #75 where @tejal29 is writing some initial unit tests we can add them for this logic too

bobcatfish · 2018-10-02T19:41:32Z

pkg/apis/pipeline/v1alpha1/task_types.go

-	Outputs   *Outputs  `json:"outputs,omitempty"`
-	BuildSpec BuildSpec `json:"buildSpec"`
+	Outputs   *Outputs                 `json:"outputs,omitempty"`
+	BuildSpec *buildv1alpha1.BuildSpec `json:"buildSpec"`


switching to the build crd type will make our code easier

also lets delete type BuildSpec struct in the same file

bobcatfish · 2018-10-02T19:42:22Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

+	// AbortedState means prow killed the job early (new commit pushed, perhaps).
+	AbortedState = "aborted"
+	// ErrorState means the job could not schedule (bad config, perhaps).
+	ErrorState = "error"


can we use the types we already defined instead of replacing with the prow ones? from what i can see here it doesn't seem like the prow conditions are following the k8s api conventions but what we had before did (and so does most of knative)

As discussed this morning with @tejal29 we could completely remove most of the condition logic here and replace it with something like:

Does it need to exist? Then create it

Does it need to be deleted? Then delete it (or even leave this for a later iteration)

p.s. I think in the long term we should use https://github.com/knative/pkg/blob/master/apis/duck/v1alpha1/conditions_types.go but due to a lack of docs im not exactly sure how to just dive in there (maybe the docs exist and i just can't find them @mattmoor ?)

+1 agree with @bobcatfish. Lets add them if we need those and end up using these.

Here is a code example from serving https://github.com/knative/serving/blob/76500a6917ce4aec522651677dd9bac677c6034f/pkg/reconciler/v1alpha1/revision/revision.go#L265

https://docs.google.com/document/d/16j8C91jML4fQRQPhnHihNJUJDcbvW0RM1YAX2REHgyY/edit#heading=h.n8a530nnrb

@pivotal-nader-ziada thanks for thereconcileBuild links!

I think our plan for Task Status is to adopt the same status conditions as Build and sync the Tasks status to be the same as the Build's status

bobcatfish · 2018-10-02T19:44:57Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	switch {
+	case tr.DeletionTimestamp == nil:
+		wantBuild = true
+	}


i think an if statement would be clearer here? or do we already know we need other cases?

+1 for if
is there a possibility to merge the multiple switch cases info one switch with the code actually inside the case? would be easier to read if possible

if the taskRun is deleted, what is the case you need to create anything? should we just assume its done?

bobcatfish · 2018-10-02T19:45:56Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+
+	// Get related task for taskrun
+	t, err := c.PipelineClientSet.PipelineV1alpha1().Tasks(tr.Namespace).Get(
+		tr.Spec.TaskRef.Name, metav1.GetOptions{})


i think this is only used in the wantBuild && !haveBuild: case, maybe we could move this logic there? (maybe even in a separate function, so that this function is only about understanding the cases?

bobcatfish · 2018-10-02T19:46:21Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	b, err := c.getBuild(namespace, name)
+	switch {
+	case errors.IsNotFound(err):
+		// Do not have a build


this should probably be an error if we expect the build to already exist right?

yes, should either return an error or create a build if it needs it

if the error is that a build is not found, that means that we need to create a build (haveBuild = false which is it's default value) which is a passthrough here. In the case that it is any other error (the next switch case) we return an error. The passthrough is required to catch any other kind of error in the next statement

I'll collapse this into one statement

bobcatfish · 2018-10-02T19:47:00Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+
+	var haveBuild bool
+
+	b, err := c.getBuild(namespace, name)


can we add a comment about how we're using the same name for Build and TaskRun

bobcatfish · 2018-10-02T19:47:47Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	case errors.IsNotFound(err):
+		// Do not have a build
+	case err != nil:
+		return fmt.Errorf("get build: %v", err)


maybe some more detail in this error message, e.g. expected build %s to exist for taskRun %s but failed to retrieve it: %s

bobcatfish · 2018-10-02T19:48:27Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	switch {
+	case !wantBuild:
+		if !haveBuild {
+			fmt.Printf("Deleted %s", key)


should this be done via the logger? (also maybe more info in the log message)

same thing for other fmt.Printfs in this review

changed, good catch!

bobcatfish · 2018-10-02T19:48:51Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		}
+		switch or := metav1.GetControllerOf(b); {
+		case or == nil, or.APIVersion != groupVersionKind.GroupVersion().String(), or.Kind != groupVersionKind.Kind:
+			return nil // Not controlled by this


what is or? im kind of confused by this case, maybe more comments?

or i guess is ownerRef

or is ownerref which is what is being checked. i've changed the name to be ownerref

bobcatfish · 2018-10-02T20:03:13Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	descSucceeded        = "succeeded"
+	descFailed           = "failed"
+	descUnknown          = "unknown status"
+	descMissingCondition = "missing end condition"


as far as i can tell, these pretty much mirror the statuses, so i don't feel like they add much and we can remove them. in the recommended api conventions, I think the examples we have (which use reason, type and message) are pretty good:

(We can leave most of this for the next iteration tho, but in that case I'd rather leave it out of this PR if we can)

tejal29 · 2018-10-02T21:28:43Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

+	// AbortedState means prow killed the job early (new commit pushed, perhaps).
+	AbortedState = "aborted"
+	// ErrorState means the job could not schedule (bad config, perhaps).
+	ErrorState = "error"


+1 agree with @bobcatfish. Lets add them if we need those and end up using these.

tejal29 · 2018-10-02T21:34:59Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	groupVersionKind = schema.GroupVersionKind{
+		Group:   v1alpha1.SchemeGroupVersion.Group,
+		Version: v1alpha1.SchemeGroupVersion.Version,
+		Kind:    "TaskRun",


add constant.

changed groupVersionKind to be a const

tejal29 · 2018-10-02T21:37:04Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		case or == nil, or.APIVersion != groupVersionKind.GroupVersion().String(), or.Kind != groupVersionKind.Kind:
+			return nil // Not controlled by this
+		}
+		fmt.Printf("Delete builds/%s", key)


use logger here and elsewhere.

tejal29 · 2018-10-02T21:49:39Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		}
+		switch or := metav1.GetControllerOf(b); {
+		case or == nil, or.APIVersion != groupVersionKind.GroupVersion().String(), or.Kind != groupVersionKind.Kind:
+			return nil // Not controlled by this


or i guess is ownerRef

tejal29 · 2018-10-02T21:54:51Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	}
+
+	// Ensure taskrun status is correct
+	haveState := tr.Status.State


the ntr is not used anywhere?
Can we just move lines 219:232 to a function, and use that in L237 in reconcile ?

this is an error on my part, ntr should be used in updateStatus. I will need to move some things around for this

I will refactor lines 219:232 to a function

bobcatfish · 2018-10-02T23:21:43Z

(plz add link to #59 in commit message)

bobcatfish · 2018-10-02T23:22:36Z

For this to work, would a person need to deploy any Build CRD types to their cluster as well? If so plz update DEVELOPMENT.md re. how to do this

nader-ziada · 2018-10-03T13:33:04Z

pkg/apis/pipeline/v1alpha1/task_types.go

-	Outputs   *Outputs  `json:"outputs,omitempty"`
-	BuildSpec BuildSpec `json:"buildSpec"`
+	Outputs   *Outputs                 `json:"outputs,omitempty"`
+	BuildSpec *buildv1alpha1.BuildSpec `json:"buildSpec"`


switching to the build crd type will make our code easier

also lets delete type BuildSpec struct in the same file

nader-ziada · 2018-10-03T13:55:41Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

+	// AbortedState means prow killed the job early (new commit pushed, perhaps).
+	AbortedState = "aborted"
+	// ErrorState means the job could not schedule (bad config, perhaps).
+	ErrorState = "error"


Here is a code example from serving https://github.com/knative/serving/blob/76500a6917ce4aec522651677dd9bac677c6034f/pkg/reconciler/v1alpha1/revision/revision.go#L265

nader-ziada · 2018-10-03T13:56:19Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	switch {
+	case tr.DeletionTimestamp == nil:
+		wantBuild = true
+	}


+1 for if
is there a possibility to merge the multiple switch cases info one switch with the code actually inside the case? would be easier to read if possible

nader-ziada · 2018-10-03T14:00:01Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	b, err := c.getBuild(namespace, name)
+	switch {
+	case errors.IsNotFound(err):
+		// Do not have a build


yes, should either return an error or create a build if it needs it

nader-ziada · 2018-10-03T14:02:08Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		ntr.Status.State = wantState
+		ntr.Status.Description = wantMsg
+	}
+
 	// Reconcile this copy of the task run and then write back any status
 	// updates regardless of whether the reconciliation errored out.
 	err = c.reconcile(ctx, tr)


I was expecting the actual logic to check/create the build to be in this private func, see example here: https://github.com/knative/serving/blob/76500a6917ce4aec522651677dd9bac677c6034f/pkg/reconciler/v1alpha1/revision/revision.go#L314:22
otherwise consider if we need this func?

the way that @pivotal-nader-ziada 's example code explicitly expresses phases, then delegates work to them seems pretty slick!

yes, I realize now that I put this in the wrong place (Reconcile vs reconcile). I will move everything to the correct reconcile function, thanks for catching this!

nader-ziada · 2018-10-03T14:04:02Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+			},
+		},
+		Spec: *t.Spec.BuildSpec,
+	}, nil


you have to check if the BuildSpec has a template and apply that template.
Applying the template includes:

checking type of template

getting appropriate template object using lister

apply the template (use ApplyTemplate() from Build?)

I have currently omitted this change as I believe the Build controller will do this for us:
https://github.com/knative/build/blob/e69c21ed74a05d83d68fc15bacbc458807be8288/pkg/controller/build/controller.go#L338

I could definitely be misunderstanding something though, let me know if you still believe we need to call ApplyTemplate()

nader-ziada · 2018-10-03T14:08:52Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

@@ -136,6 +159,79 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
 	// Don't modify the informer's copy.
 	tr := original.DeepCopy()

+	// build creation and status update adapted from prow: https://github.com/kubernetes/test-infra/blob/8faa040989cf8552683dcdd4400505ca62a3c408/prow/cmd/build/controller.go#L207


There is a tracker defined in the reconciler (meant to track build status) that I don't see used

I think I understand now, a tracker is used to monitor child object (in this case a Build) and when the tracker sees a change on the tracked object, it runs reconcile? I have added a tracker.Track statement to reconcile, thanks for pointing out the tracker!

@pivotal-nader-ziada
could you clarify how you thought the tracker would be used, from the comment:

// TODO use build tracker to fetch build

it seems that it might be possible to fetch a build with a tracker. My understanding is that a tracker is an event watcher(tracker) that runs reconcile when whatever it is tracking changes. Can you shed some light here? Thanks!

You are correct Aaron, the tracker is to watch the build. Sorry for the confusing comment

bobcatfish · 2018-10-04T23:45:00Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	}
+	return &buildv1alpha1.Build{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      t.Name,


probably want this to be the name of the taskrun, since one task could have many task runs

bobcatfish · 2018-10-04T23:45:14Z

pkg/apis/pipeline/v1alpha1/taskrun_types.go

 )

 // TaskRunSpec defines the desired state of TaskRun
 type TaskRunSpec struct {
 	TaskRef TaskRef     `json:"taskRef"`
 	Trigger TaskTrigger `json:"trigger"`
-	// +optional
+	// +optionalm


This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually.

aaron-prindle · 2018-10-05T00:19:52Z

all feedback has been addressed

This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually.

This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually. Also had to change some optional resources to be pointers, because json marshalling will still require optional fields to be present if they are not nullable (https://stackoverflow.com/questions/18088294/how-to-not-marshal-an-empty-struct-into-json-with-go)

dprotaso · 2018-10-05T12:26:19Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	switch {
+	case err != nil && !errors.IsNotFound(err):
+		return fmt.Errorf("retrieving build %s for taskRun %s: %v", tr.Name, tr.Name, err)
+	case b.DeletionTimestamp == nil:


I believe this will panic when the error != nil is true & errors.IsNotFound(err) is true

b, err := c.getBuild(tr.Namespace, tr.Name) switch { // if there is an error that is NOT "build not found" case err != nil && !errors.IsNotFound(err):

this case is for any error that is not "build not found". if "build is not found", haveBuild will remain false from initialization which is desired. let me know if i am still missing something or misunderstanding, thanks!

the b.DeletionTimestamp will panic if the build is not found and is nil

ah of course, I will change it back to the original form

nader-ziada · 2018-10-05T13:30:55Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	descMissingCondition = "missing end condition"
+)
+
+const (


This should be var instead of const. Its making the code not compile and thats why the tests in the PR are failing

i've changed this back to var

nader-ziada · 2018-10-05T14:16:26Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+			return fmt.Errorf("delete build: %v", err)
+		}
+		return nil
+	case tr.Status.Conditions[0].Status != corev1.ConditionUnknown:


This will panic if the array is empty which what happened when I tried to run it

fixed now, thanks!

nader-ziada · 2018-10-05T14:17:35Z

@aaron-prindle can you add at least one test to make sure the code runs with no issues, we can add more tests later

This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually.

This is a WIP of adding an integration test to cover #59, which is a simple TaskRun which has no inputs or outputs but just runs a container that echoes helloworld. At this point we have the logic to: - Create the Task and TaskRun - Wait for the TaskRun to have a condition (TODO: actually check the conditions, waiting to rebase onto #86) - Get the Build associated (not yet created, will be via #86) - Get the Pod for the build - TODO: get the logs for that Pod (we've been looking at https://github.com/knative/build/blob/e8c2cb6eb5cb09d9737ca9e6da4a1c68af3247b2/pkg/logs/logs.go#L36:6 for inspiration) This also changes namespaces to be test specific, so each test can create a namespace and tear it down, without needing to keep track of resources created and delete them individually. Co-authored-by: Aaron Prindle <aprindle@google.com>

aaron-prindle · 2018-10-06T00:57:05Z

all changes made, changed taskrun.go controller to no longer support delete trigger (mirroring pipelinerun). this simplifies the controller logic. also added taskrun_test.go which mirrors pipelinerun_test.go

bobcatfish · 2018-10-08T17:36:40Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	descSucceeded        = "succeeded"
+	descFailed           = "failed"
+	descUnknown          = "unknown status"
+	descMissingCondition = "missing end condition"


i think these can be deleted, doesn't seem like they are used anymore

bobcatfish · 2018-10-08T18:25:52Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+)
+
+var (
+	groupVersionKind = schema.GroupVersionKind{


this is misc cleanup but we should change schema to scheme maybe? kinda weird that we are using both schema and scheme

I'm not sure what is suggested here, the lib's default name is schema. Would you want schema to be aliased to something else?

nope my bad, i misunderstood and thought we had made a package called schema

bobcatfish · 2018-10-08T18:26:48Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		Name:       tr.Name,
+	}
+	if err := c.tracker.Track(buildRef, tr); err != nil {
+		logger.Errorf("build %s for taskrun %s: %v", buildRef, tr.Name, err)


could the error say something about what it was trying to do? e.g. something like failed to create tracker for build %s for taskrun %s: %v

bobcatfish · 2018-10-08T18:27:24Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+		// make build obj from task buildspec
+		logger.Infof("make build: %s", tr.Name)
+		if b, err = c.makeBuild(t, tr); err != nil {
+			return fmt.Errorf("make build: %v", err)


failed to create a build for taskrun %s: %v

bobcatfish · 2018-10-08T18:37:06Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+
+func (c *Reconciler) getBuild(namespace, name string) (*buildv1alpha1.Build, error) {
+	return c.buildLister.Builds(namespace).Get(name)
+}


i dont think we need these wrapper methods, this might have been based on a previous iteration of the pipelinerun controller (for mocking), check out the latest greatness https://github.com/knative/build-pipeline/blob/master/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go#L69

bobcatfish · 2018-10-08T21:59:45Z

One big find here was that the logs in the pods went away too quickly for us to read them, even when trying to read them IMMEDIATELY after the Build completed, so we need a better solution here (@aaron-prindle you or I should make a task in the october milestone for this)

This pr implements a simple TaskRun controller that creates a knative/build Build and updates the TaskRun status to reflect the Build status. We delegate to the knative/build controller to do the work of actually fulfillign the Build itself - meaning we have a hard dependency on knative/build. The integration test doesn't actually assert on the logs output by the bulid step because the pods disappear immediately after completion, so we need a better solution here (e.g. writing to a PVC in the test) - in the long run we need to implement better log support (#107). Remaining work for #59 is to improve unit test coverage, and add some docs on running + debugging.

bobcatfish · 2018-10-08T23:21:31Z

/lgtm
/meow space

knative-prow-robot · 2018-10-08T23:21:32Z

@bobcatfish:

In response to this:

/lgtm
/meow space

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.

bobcatfish · 2018-10-08T23:22:02Z

wut

/approve

knative-prow-robot · 2018-10-08T23:22:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaron-prindle, bobcatfish

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

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

We don't yet have validation implemented for our Runs, but as of #86 we are using the same Conditions as Knative Build: just `Succeeded` (where unknown means running, true means it finished successfully and false means it finished but failed)

knative-prow-robot requested review from imjasonh and tejal29 October 2, 2018 18:55

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

bobcatfish reviewed Oct 2, 2018

View reviewed changes

tejal29 reviewed Oct 2, 2018

View reviewed changes

nader-ziada suggested changes Oct 3, 2018

View reviewed changes

bobcatfish reviewed Oct 4, 2018

View reviewed changes

bobcatfish mentioned this pull request Oct 5, 2018

Add initial helloworld integration test #98

Merged

dprotaso reviewed Oct 5, 2018

View reviewed changes

nader-ziada suggested changes Oct 5, 2018

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 6, 2018

bobcatfish reviewed Oct 8, 2018

View reviewed changes

knative-prow-robot assigned bobcatfish Oct 8, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2018

knative-prow-robot merged commit c6fe81c into tektoncd:master Oct 8, 2018

aaron-prindle mentioned this pull request Oct 9, 2018

Implement working "hello world" Task + TaskRun #59

Closed

bobcatfish mentioned this pull request Oct 9, 2018

Only have one set of examples and test that they are valid #109

Merged

add taskrun controller implementation #86

add taskrun controller implementation #86

Conversation

aaron-prindle commented Oct 2, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Oct 3, 2018 • edited by aaron-prindle 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

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by tejal29 Loading

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

nader-ziada Oct 3, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 2, 2018 • edited by aaron-prindle 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

tejal29 Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejal29 Oct 2, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

tejal29 Oct 2, 2018 • edited by aaron-prindle 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

bobcatfish commented Oct 2, 2018

bobcatfish commented Oct 2, 2018

nader-ziada Oct 3, 2018 • edited by aaron-prindle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Oct 3, 2018 • edited by aaron-prindle 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

aaron-prindle Oct 3, 2018 • 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

aaron-prindle commented Oct 5, 2018

Choose a reason for hiding this comment

aaron-prindle Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada Oct 3, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by tejal29

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

nader-ziada Oct 3, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

bobcatfish Oct 2, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 2, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 2, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 2, 2018 •

edited by aaron-prindle

Loading

tejal29 Oct 2, 2018 •

edited by aaron-prindle

Loading

nader-ziada Oct 3, 2018 •

edited by aaron-prindle

Loading

nader-ziada Oct 3, 2018 •

edited by aaron-prindle

Loading

aaron-prindle Oct 3, 2018 •

edited

Loading

aaron-prindle Oct 5, 2018 •

edited

Loading