apb pkg public interface overhaul #773

eriknelson · 2018-02-18T22:54:02Z

Describe what this PR does and why we need it:
CC: @maleck13, this is the intended follow-up for #619. We identified the way we'd like the public interfaces of the apb pkg to move in light of trying to think more consciously about what the experience should be like consuming libapb. Namely:

There should be a suite of methods for callers to run APBs both asynchronously, and synchronously.
The async calls are non-blocking, and should push updates back to the caller over a StatusMessage channel.
The sync calls should block, and return with a result.

Changes proposed in this pull request:

Implement the above items. Introduces an Executor type that manages the APB lifecycle, and exposes the various methods for execution.
Update the broker calling code to use the new apb pkg correctly.
Job code has become virtually identical, so this removes the individual jobs favoring a more generic apbJob type, with concrete wrapper methods that call into the generic job.

Couple blockers before a merge:

It's obviously a pretty big change to how we're doing things, so I would like to get a lot of review attention and testing.
I deleted a lot of non-relevant tests and haven't added any to test the new code paths, so I think we need a strategy to make sure the new code is properly tested and to get that implemented. I mostly wanted to just prove out in the code that this was possible The new jobs routine is fully tested.

EDIT 02/20: I added white box testing that exercises the new job branches. I'm not too concerned with this now given a green make ci + make check.

TODO: I still need to implement the Sync variants of the apb actions. I'm seeing them simply call the async versions and block while watching the channel. The caller will know no better, and we're using the same codepaths. This is required, but the domain of another PR.

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #771

Introduces the Executor as well.

Binding -> Bind Unbinding -> Unbind

The New funcs really are not offering much.

jmrodri

I really like this, great improvement. I'll look more deeply at it again but so far. +1

jmrodri · 2018-02-19T01:40:18Z

pkg/apb/bind.go

+		metrics.ActionStarted("bind")
+		executionContext, err := e.executeApb(
+			"bind", instance.Spec, instance.Context, parameters)
+		defer runtime.Provider.DestroySandbox(


Do we do the defer AFTER we kickoff the executeApb? feels like we should do this before. Because the err we are looking at on line 53 is related to the execute not the defer.

I'm not clear on the best practice with defers. If I'm doing something and I know I'll need to call the relevant "finish" function at the end of it, it makes sense to me to defer the call immediately after, assuming the mechanics of defer apply. A good example of this is the metrics hooks in the apbJob, I just partner the Start with a defered finish right way. I'll take a closer look at this, thanks for raising it.

After looking at this, I think it doesn't actually matter in practice? Regardless of where the defer call is made, it will execute at the conclusion of the function. Optically, it might make sense to be made elsewhere, though.

I think that if the sandbox is created in the executeApb method then calling defer afterwards makes sense. It is how the code looks currently I believe?
Speaking of Optics and readability as we have a call to runtime.Provider.DestroySandbox would it be reasonable to extract the call to the CreateSandbox method from the executeApb method.

runtime.Provider.CreateSandbox() defer runtime.Provider.DestroySandbox() e.executeApb()

It doesn't matter, what I was saying was tying statements together.

go func() { e.start() metrics.ActionStarted("bind") defer runtime.Provider.DestroySandbox( executionContext.PodName, executionContext.Namespace, executionContext.Targets, clusterConfig.Namespace, clusterConfig.KeepNamespace, clusterConfig.KeepNamespaceOnError, ) executionContext, err := e.executeApb("bind", instance.Spec, instance.Context, parameters) if err != nil { log.Errorf("Problem executing apb [%s] bind", executionContext.PodName) e.finishWithError(err) return }

So the err != nil follows the executeApb so it is obvious that it is related and highly unlikely that someone will come and add another line of code that set err. As it stands now the defer statement separates the executeApb and the err check. If someone comes in later they could accidentally set err and we'll be eating an err.

~~The issue is that executionContext isn't in scope until the executeApb call finished. I'll update to keep the err check tied closely.~~

I think it's actually exactly where it needs to be, we definitely still want to destroy the sandbox even if an error occurs, but it also must be after executionContext is in scope. (unless we implement @maleck13's suggestion, which I do think is clearer and maybe makes sense in another PR).

jmrodri · 2018-02-19T01:51:01Z

pkg/apb/executor.go

+	return e.extractedCredentials
+}
+
+func (e *Executor) start() {


I would like to rename these so they are obvious. I kept having to lookup what it meant by start. I'd think something like sendStart maybe it's just me. I'll think about it.

They're really lifecycle hooks that need to be called from the action methods to alert the Executor to the current state of what's going on (and it abstracts the fact that messages are being sent). I originally named updateDescriptor to something like sendDescriptorMessage, but I felt like the latter was a less abstract name. I want to call high level hooks to alert the executor to triggers in the cycle without saying, "hey send this message", because it's better if the actions don't even care about that. They just say "hey, this thing happened, do what you would like with this info".

I'll also say that I tried to avoid imposing additional requirements on callers since I expect these to turn into the primary methods that people will use with libapb. The worst of it is to ask folks to instantiate an Executor with NewExecutor; I've personally seen us on this path for a while, so I'm happy to finally get this down on paper, so to speak.

I think the use of the word start on a type called Executor is a little confusing as it seems to suggest this is how you start execution.
Off the cuff, but what if you were to have a internal statusPublisher object that had these methods so that it would read as e.statusPublisher.start() also would give an opportunity for this to be an interface that could be overridden for tests or other uses if we exposed it via a constructor.

Do we think there is a better name for Executor? I'm still hesitant to name these something like "sendX" or other related messaging concept. They're internal lifecycle hooks intended to be called by the internal APB code, and do more than send a message over the buffer (not much today, but tomorrow they might).

also would give an opportunity for this to be an interface that could be overridden for tests or other uses if we exposed it via a constructor.

Definitely interested in this since decent test coverage is the biggest thing missing right now. I don't think callers should have to worry about injecting that via ctor though. A sensible default that can get overriden by test code I think makes sense, given this is an internal detail.

Definitely interested in this since decent test coverage is the biggest thing missing right now. I don't think callers should have to worry about injecting that via ctor though. A sensible default that can get overriden by test code I think makes sense, given this is an internal detail.

Yep agree no need to expose as long as it can be overridden in test code

shawn-hurley · 2018-02-19T03:30:43Z

pkg/apb/executor.go

-	context *Context,
-	p *Parameters) (ExecutionContext, error) {
+// Executor - Main struct used for running APBs. Manages their lifecycle.
+type Executor struct {


I would like to make it more clear that a single Executor for a single APB run. At least that is how I am reading this code, am I wrong?

Yeah that's exactly correct. What do you think the best way is to convey that?

I would say documentation for the end user. Code examples will help too. I think that nothing from the code will make it more obvious. At least I can't think of anything at the moment.

maleck13

This is a pretty big PR. I have added quite a few thoughts and comments. If some don't make sense feel free to ignore, can be difficult to understand the full thought process from an external perspective.

maleck13 · 2018-02-19T09:22:15Z

pkg/apb/bind.go

+		metrics.ActionStarted("bind")
+		executionContext, err := e.executeApb(
+			"bind", instance.Spec, instance.Context, parameters)
+		defer runtime.Provider.DestroySandbox(


I think that if the sandbox is created in the executeApb method then calling defer afterwards makes sense. It is how the code looks currently I believe?
Speaking of Optics and readability as we have a call to runtime.Provider.DestroySandbox would it be reasonable to extract the call to the CreateSandbox method from the executeApb method.

runtime.Provider.CreateSandbox() defer runtime.Provider.DestroySandbox() e.executeApb()

maleck13 · 2018-02-19T10:01:19Z

pkg/apb/executor.go

+}
+
+// GetPodName - Returns the name of the pod running the APB
+func (e *Executor) GetPodName() string {


nit: generally in go you don't use the Get https://golang.org/doc/effective_go.html#Getters . It's really a small thing but if this is to turn into an external lib that we hope will be consumed by go developers probably worth following the idioms

maleck13 · 2018-02-19T10:03:02Z

pkg/apb/executor.go

+	status.State = StateFailed
+	e.lastStatus = status
+	e.statusChan <- status
+	close(e.statusChan)


seeing the close in two different methods makes me a little nervous that we will potentially close the channel twice and cause a panic, maybe some form of protection against this would be useful

Very good point. Reading a bit, it looks like there isn't a safe way to check if a channel has been closed without reading from it, which feels ugly and indirect to me. Alternatively, we can lock the sensitive code and nil it out, then check if it's still present before closing.

Added some locks.

maleck13 · 2018-02-19T10:03:50Z

pkg/apb/executor.go

+	close(e.statusChan)
+}
+
+func (e *Executor) updateDescription(newDescription string) {


At some point we may want to be able to add the Dashboard URL, but probably a feature of its own rather than to do with this PR

maleck13 · 2018-02-19T10:40:15Z

pkg/apb/provision_or_update.go

-	stateUpdates chan<- JobState) (string, *ExtractedCredentials, error) {
-
+func (e *Executor) provisionOrUpdate(method executionMethod, instance *ServiceInstance) {
+	e.start()


Opinion: what if we were to have defer e.stop() and remove the close from the other funcs? The other funcs would then become sendError() and sendSuccess()
Another option here would be to return a function from start that closed the channel

stop := e.start() defer stop()

Also see comment below around the start method

maleck13 · 2018-02-19T10:55:51Z

pkg/apb/executor.go

+	return e.extractedCredentials
+}
+
+func (e *Executor) start() {


I think the use of the word start on a type called Executor is a little confusing as it seems to suggest this is how you start execution.
Off the cuff, but what if you were to have a internal statusPublisher object that had these methods so that it would read as e.statusPublisher.start() also would give an opportunity for this to be an interface that could be overridden for tests or other uses if we exposed it via a constructor.

maleck13 · 2018-02-19T11:07:10Z

pkg/broker/jobs.go

+	specID                 string
+	bindingID              *string
+	method                 apb.JobMethod
+	metricsJobStartHook    metricsHookFn


So potentially as part of the work I have outlined under the proposal for the changes to the work engine these would become StateObservers / Subscribers and not needed here

shawn-hurley · 2018-02-19T04:04:50Z

pkg/broker/jobs.go

+)
+
+type metricsHookFn func()
+type runFn func(*apb.Executor) <-chan apb.StatusMessage


I think means that we can delete the Provisioner, Binder all functions

shawn-hurley · 2018-02-19T21:23:09Z

pkg/apb/executor.go

+}
+
+// Executor - Composite executor interface.
+type Executor interface {


shawn-hurley · 2018-02-19T21:32:06Z

pkg/apb/types.go

-// Provisioner defines a function that knows how to provision an apb
-type Provisioner func(si *ServiceInstance, statusUpdate chan<- JobState) (string, *ExtractedCredentials, error)
+// Provisioner - a function that knows how to provision an apb
+type Provisioner func(si *ServiceInstance) (<-chan StatusMessage, error)


I think we can delete these interfaces with this PR?

I've actually been going back and forth on this. I deleted them and then force pushed them back in. I'm trying to test asb_jobs, and the runFn should render these moot, so I'll probably pull them again.

I don't see Provisioner used anywhere.

eriknelson · 2018-02-19T21:51:59Z

@shawn-hurley Are you taking issue with the Executor.ExtractedCredentials? I went back and forth on this one... originally I did not have an accessor because it's a pointer type and could easily be mutated by a caller, but I added the accessor method for consistency because it felt silly to have the rest of the fields retrieved by method and not this one.

I think you should be able to get ExtractedCredentials from the Executor somehow.

shawn-hurley · 2018-02-19T21:55:00Z

No we are exposing how we pull creds from the pod in the ext_creds.go I think. If it’s out of scope just tell me to go away until your done :)

…

On Mon, Feb 19, 2018 at 4:52 PM Erik Nelson ***@***.***> wrote: @shawn-hurley <https://github.com/shawn-hurley> Are you taking issue with the Executor.ExtractedCredentials? I went back and forth on this one... originally I did not have an accessor because it's a pointer type and could easily be mutated by a caller, but I added the accessor method for consistency because it felt silly to have the rest of the fields retrieved by method and not this one. I think you should be able to get ExtractedCredentials from the Executor somehow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#773 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABza922jf5j9eqVs8QXpo4_OJ4Qg3J8Xks5tWe0BgaJpZM4SJ47R> .

eriknelson · 2018-02-19T22:07:18Z

@shawn-hurley can you elaborate? This is all good discussion; we're going to have to figure out how to split up the changes so this refactoring doesn't apply to the whole world. I would really like to keep PRs small and focused, and I know I've expressed that to you. I'm completely violating that here.

I'm also finding it super helpful to post some code and have you guys comment, and iterate. I often hit questions that I'm not comfortable taking a stance with on my own.

jmrodri

Looking great. Two things:

verify this is good: e.extractedCredentials = creds, if so, proceed.
question about the executor status methods

I'd be okay if the sync work was a separate PR.

jmrodri · 2018-02-20T01:17:34Z

pkg/apb/bind.go

+		metrics.ActionStarted("bind")
+		executionContext, err := e.executeApb(
+			"bind", instance.Spec, instance.Context, parameters)
+		defer runtime.Provider.DestroySandbox(


It doesn't matter, what I was saying was tying statements together.

go func() { e.start() metrics.ActionStarted("bind") defer runtime.Provider.DestroySandbox( executionContext.PodName, executionContext.Namespace, executionContext.Targets, clusterConfig.Namespace, clusterConfig.KeepNamespace, clusterConfig.KeepNamespaceOnError, ) executionContext, err := e.executeApb("bind", instance.Spec, instance.Context, parameters) if err != nil { log.Errorf("Problem executing apb [%s] bind", executionContext.PodName) e.finishWithError(err) return }

So the err != nil follows the executeApb so it is obvious that it is related and highly unlikely that someone will come and add another line of code that set err. As it stands now the defer statement separates the executeApb and the err check. If someone comes in later they could accidentally set err and we'll be eating an err.

jmrodri · 2018-02-20T01:18:56Z

pkg/apb/bind.go

+			executionContext.Namespace,
+			instance.Spec.Runtime,
+		)
+		e.extractedCredentials = creds


What if there is an err with ExtractCredentials? Do we want to set e.extractedCredentials before checking the err value? If this is a conscience decision then okay, just wanted to make sure we're not setting something invalid.

It was actually originally written this way, I thought about it and the expectation is that creds will be nil if an error is returned from ExtractCredentials. Your comment is probably safer, however. Will update.

jmrodri · 2018-02-20T01:21:27Z

pkg/apb/deprovision.go

+		if err != nil {
+			log.Errorf("Problem executing apb [%s] deprovision", executionContext.PodName)
+			e.finishWithError(err)
+			return


same comment from bind.go regarding defer separating the err check.

jmrodri · 2018-02-20T01:22:05Z

pkg/apb/provision.go

-	return provisionOrUpdate(executionMethodProvision, instance, stateUpdates)
+	go e.provisionOrUpdate(executionMethodProvision, instance)
+
+	return e.statusChan


👍 provision.go looks okay

jmrodri · 2018-02-20T01:23:15Z

pkg/apb/update.go

-	// provisionOrUpdate as an implementation detail.
-	return provisionOrUpdate(executionMethodUpdate, instance, statusUpdate)
+	go e.provisionOrUpdate(executionMethodUpdate, instance)
+


update.go is okay

jmrodri · 2018-02-20T01:29:26Z

pkg/apb/watch_pod.go

+
+func watchPod(
+	podName string, namespace string, podClient v1.PodInterface,
+	updateDescription updateDescriptionFn,


watch_pod changes 👍

jmrodri · 2018-02-20T01:31:00Z

pkg/broker/broker.go

@@ -1241,7 +1242,7 @@ func (a AnsibleBroker) Update(instanceUUID uuid.UUID, req *UpdateRequest, async
 	log.Debugf("toPlanName: [%s]", toPlan.Name)
 	log.Debugf("PreviousValues: [ %+v ]", req.PreviousValues)
 	log.Debugf("ServiceInstance Parameters: [%v]", *si.Parameters)
-	ujob := NewUpdateJob(si, apb.Update)
+	ujob := &UpdateJob{si}


I'm okay with using the job structs directly 👍

jmrodri · 2018-02-20T01:40:09Z

pkg/broker/jobs.go

+			return e.Update(j.serviceInstance)
+		},
+	}
+	job.Run(token, msgBuffer)


I looked over the entire jobs.go. ❗ looks great. I like it.

jmrodri · 2018-02-20T01:40:57Z

pkg/metrics/metrics.go

-// BindingJobStarted - Add a provision job to the counter.
-func BindingJobStarted() {
+// BindJobStarted - Add a provision job to the counter.
+func BindJobStarted() {
 	defer recoverMetricPanic()


okay, this is fine.

jmrodri · 2018-02-20T01:44:57Z

pkg/apb/executor.go

+	status := e.lastStatus
+	status.State = StateInProgress
+	e.lastStatus = status
+	e.statusChan <- status


I'm confused by this, we copy the lastStatus into status, set its state, then set it back on lastStatus. Why isn't this just:

func (e *executor) start() { log.Debug("executor::start") e.lastStatus.State = StateInProgress e.statusChan <- e.lastStatus }

I think I was on autopilot here and it felt like a redux action, which is of course totally irrelevant here. I'll update 😄

shawn-hurley · 2018-02-20T14:33:43Z

@eriknelson re: extracted credentials

This is what I was thinking about: https://github.com/openshift/ansible-service-broker/blob/master/pkg/apb/ext_creds.go#L48

I agree that this should be a follow-on, your right that this is something that can be done separately. I mentioned it here because #771 I thought was referring to the apb package and not just the actions. Sorry if this is off topic.

eriknelson · 2018-02-20T14:48:27Z

@shawn-hurley I actually had the same thought and had marked it private, but the broker actually calls it in broker.go as part of recovery, so I left it alone.

shawn-hurley · 2018-02-20T14:51:26Z

I didn't realize that we may want to move recover out of the broker then. I think that may require a new issue to track and discuss, thoughts @jmrodri @rthallisey @eriknelson?

eriknelson · 2018-02-20T15:01:51Z

@shawn-hurley IMO recover is firmly a concept in the broker domain. I'm not sure where else it would belong aside from there.

We were previously sending a bunch of duplicate messages, now the jobs trust the status messages. This should fix ci issues.

* start -> actionStarted * finishWithSuccess -> actionFinishedWithSuccess * finishWithError -> actionFinishedWithError

Adds testify

eriknelson · 2018-02-21T00:30:59Z

@shawn-hurley @maleck13 @rthallisey Think this should be considered complete at this point. I'm planning on submitting another PR with the sync work.

eriknelson · 2018-02-21T02:10:03Z

This utterly ballooned to be almost unreviewable after I added the vendor stuff. If you look at the individual commits prior to that, I added jobs.go tests and in an effort to test it completely, I needed an Executor mock. I used mockery to generate mock_Executor.go, which was enormously helpful. It requires testify, which I had to add to the vendor deps. Glide was not helpful in this regard.

Gotta say, it was easy to integrate and incredibly useful. I would encourage us to continue with that pattern for mocking deps.

shawn-hurley

One other question that I asked in IRC because, it is for my edification and not something I want to block this PR.

shawn-hurley · 2018-02-21T14:18:15Z

pkg/apb/mock_Executor.go

@@ -0,0 +1,134 @@
+// Code generated by mockery v1.0.0


I am wondering if we should have a mock package for this?

Just makes it more clear in your import that you're importing a mock?

I started thinking about this as well, but it implies each pkg would have a corresponding child mock package? Does that make sense?

what if you were to create a top level mock package that contained shared mocks instead of a mock package in each individual package?

I don't think we want to be mixing mocks for a bunch of different packages into one package.

eriknelson · 2018-02-21T14:42:32Z

Thank you for the reviews everyone! Merging since there is a bunch of work dependent on this, and it's green.

* Initial commit moving things around in apb Introduces the Executor as well. * Remove individual jobs * Add consolidated jobs * Some minor modifications to apb func sigs * Update metrics names Binding -> Bind Unbinding -> Unbind * Use raw structs instead of New* variants The New funcs really are not offering much. * Mark non-bindable APBs finished * Update skipExection APBs with a completion desc * Fix lint errors * Protect statusChan closures with mutex * Use golang friendly getter names * Fix existing tests * Add initial jobs testing * Move to Executor interface for testing * Only set extractedCredentials with no err * Remove old apb run fns * Remove verbose status setting * Fix some of the job report logic We were previously sending a bunch of duplicate messages, now the jobs trust the status messages. This should fix ci issues. * Update executor hook names for more clarity * start -> actionStarted * finishWithSuccess -> actionFinishedWithSuccess * finishWithError -> actionFinishedWithError * Initial tests plus some fixes * Add mock executor and finish jobs_tests * Vendor updates Adds testify * Remove debug var

eriknelson added 6 commits February 18, 2018 15:07

Initial commit moving things around in apb

3897fff

Introduces the Executor as well.

Remove individual jobs

a4bcdbd

Add consolidated jobs

de4dfdb

Some minor modifications to apb func sigs

2701d9c

Update metrics names

8defc16

Binding -> Bind Unbinding -> Unbind

Use raw structs instead of New* variants

cbf74fe

The New funcs really are not offering much.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 18, 2018

eriknelson added tech-debt 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 18, 2018

eriknelson requested a review from jmrodri February 18, 2018 23:50

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 18, 2018

eriknelson requested review from shawn-hurley and rthallisey February 18, 2018 23:50

eriknelson added 2 commits February 18, 2018 19:13

Mark non-bindable APBs finished

a5d2fb4

Update skipExection APBs with a completion desc

3375b35

eriknelson added the needs-review label Feb 19, 2018

jmrodri approved these changes Feb 19, 2018

View reviewed changes

Fix lint errors

dcec1e3

shawn-hurley reviewed Feb 19, 2018

View reviewed changes

maleck13 reviewed Feb 19, 2018

View reviewed changes

eriknelson mentioned this pull request Feb 19, 2018

Public apb action methods should be thoughtfully considered, and reworked. #771

Closed

eriknelson added 3 commits February 19, 2018 09:27

Protect statusChan closures with mutex

5aefe19

Use golang friendly getter names

83339ff

Fix existing tests

1caffe1

eriknelson force-pushed the pub-apb-api branch from efee7e1 to 1caffe1 Compare February 19, 2018 16:08

eriknelson added 2 commits February 19, 2018 13:12

Add initial jobs testing

71d2353

Move to Executor interface for testing

5a61157

shawn-hurley reviewed Feb 19, 2018

View reviewed changes

jmrodri suggested changes Feb 20, 2018

View reviewed changes

Only set extractedCredentials with no err

4ab070d

eriknelson added 6 commits February 20, 2018 10:10

Remove old apb run fns

a331b65

Remove verbose status setting

37b45dd

Fix some of the job report logic

360b7b2

We were previously sending a bunch of duplicate messages, now the jobs trust the status messages. This should fix ci issues.

Update executor hook names for more clarity

4505bdc

* start -> actionStarted * finishWithSuccess -> actionFinishedWithSuccess * finishWithError -> actionFinishedWithError

Initial tests plus some fixes

b04683f

Add mock executor and finish jobs_tests

17f8fbe

jmrodri approved these changes Feb 20, 2018

View reviewed changes

Vendor updates

f7a5895

Adds testify

eriknelson removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2018

eriknelson changed the title ~~[WIP] apb pkg public interface overhaul~~ apb pkg public interface overhaul Feb 20, 2018

Remove debug var

c0babad

eriknelson mentioned this pull request Feb 21, 2018

Save extracted credentials into a secret #775

Merged

6 tasks

shawn-hurley approved these changes Feb 21, 2018

View reviewed changes

maleck13 approved these changes Feb 21, 2018

View reviewed changes

eriknelson merged commit 38b79bf into openshift:master Feb 21, 2018

eriknelson deleted the pub-apb-api branch February 21, 2018 14:42

eriknelson mentioned this pull request Feb 21, 2018

Consider mock organization? #778

Closed

apb pkg public interface overhaul #773

apb pkg public interface overhaul #773

Conversation

eriknelson commented Feb 18, 2018 • edited Loading

jmrodri 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

eriknelson Feb 20, 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

Choose a reason for hiding this comment

shawn-hurley Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maleck13 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

eriknelson commented Feb 19, 2018

shawn-hurley commented Feb 19, 2018 via email

eriknelson commented Feb 19, 2018 • edited Loading

jmrodri 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

shawn-hurley commented Feb 20, 2018

eriknelson commented Feb 20, 2018 • edited Loading

shawn-hurley commented Feb 20, 2018

eriknelson commented Feb 20, 2018

eriknelson commented Feb 21, 2018

eriknelson commented Feb 21, 2018 • edited Loading

shawn-hurley 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

eriknelson commented Feb 21, 2018

eriknelson commented Feb 18, 2018 •

edited

Loading

eriknelson Feb 20, 2018 •

edited

Loading

shawn-hurley Feb 19, 2018 •

edited

Loading

eriknelson commented Feb 19, 2018 •

edited

Loading

eriknelson commented Feb 20, 2018 •

edited

Loading

eriknelson commented Feb 21, 2018 •

edited

Loading