Cleanup policy for builds #13788

coreydaley · 2017-04-17T15:11:32Z

Adds ability to set successfulBuildsHistoryLimit and failedBuildsHistoryLimit on
buildConfigs which will prune old builds.

Closes #13640
Completes https://trello.com/c/048p7YRO/1044-5-cleanup-policy-for-builds-builds

coreydaley · 2017-04-17T15:11:54Z

(5) Cleanup policy for builds [builds]

coreydaley · 2017-04-26T03:52:33Z

[test]

coreydaley · 2017-04-26T03:53:25Z

[testextended][extended:core(builds)]

coreydaley · 2017-04-27T00:44:29Z

@openshift/devex ptal

jim-minter · 2017-04-27T09:31:33Z

pkg/build/controller/common/util.go

+	sortedBuilds := make([]buildapi.Build, len(builds))
+	copy(sortedBuilds, builds)
+
+	for i := len(sortedBuilds) - 1; i >= 0; i-- {


is there a good reason not to use sort.Sort here?

jim-minter · 2017-04-27T09:34:38Z

pkg/build/controller/common/util.go

+		return nil
+	}
+
+	if bcList.Items[0].Spec.FailedBuildsHistoryLimit != nil || bcList.Items[0].Spec.SuccessfulBuildsHistoryLimit != nil {


style: would be tempted to do:

if bcList.Items[0].Spec.FailedBuildsHistoryLimit == nil && bcList.Items[0].Spec.SuccessfulBuildsHistoryLimit == nil { return nil }

jim-minter · 2017-04-27T09:48:09Z

pkg/build/controller/common/util.go

+// HandleBuildPruning handles the deletion of old successful and failed builds
+// based on settings in the BuildConfig.
+func HandleBuildPruning(build *buildapi.Build, buildLister buildclient.BuildLister, buildConfigLister buildclient.BuildConfigLister, buildDeleter pruner.BuildDeleter) error {
+	if build.Status.Config == nil {


I think you should do the following: use util.ConfigNameForBuild to get the bc name from the build. Then use a .Get() to get the single bc without listing.

+1! didn't know that existed, that should make things much more simple

jim-minter · 2017-04-27T09:55:51Z

pkg/build/controller/common/util.go

+	}
+
+	if len(bcList.Items) == 0 {
+		glog.V(0).Infof("There are no buildconfigs associated with this build: %v/%v", build.Namespace, build.Name)


I don't think V(0) logging is warranted in this routine.

Fixed in next push

jim-minter · 2017-04-27T09:56:50Z

pkg/build/controller/common/util.go

+
+	if bcList.Items[0].Spec.FailedBuildsHistoryLimit != nil || bcList.Items[0].Spec.SuccessfulBuildsHistoryLimit != nil {
+
+		bList, err := buildLister.List(build.Namespace, kapi.ListOptions{


I'd consider using util.BuildConfigBuilds here, then you may not need your SortBuildsByPhase routine.

jim-minter · 2017-04-27T09:59:52Z

pkg/build/controller/common/util.go

+		if bcList.Items[0].Spec.FailedBuildsHistoryLimit != nil {
+			failedBuildsHistoryLimit := int(*bcList.Items[0].Spec.FailedBuildsHistoryLimit)
+			glog.V(4).Infof("Preparing to prune %v old failed builds.", (len(failedBuilds) - failedBuildsHistoryLimit))
+			if failedBuildsHistoryLimit == 0 && len(failedBuilds) > 0 {


If you negate your sorting predicate, you should be able to simpify this to something like successfulBuilds[successfulBuildsHistoryLimit:] here.

coreydaley · 2017-04-27T14:27:35Z

Thanks @jim-minter , your suggestions cleaned up the code quite a bit, and I'll have those "tricks" in my arsenal for next time!

coreydaley · 2017-04-27T15:27:57Z

@openshift/api-review ptal

jim-minter · 2017-04-27T15:28:29Z

pkg/build/client/clients.go

@@ -11,6 +11,11 @@ type BuildConfigGetter interface {
 	Get(namespace, name string) (*buildapi.BuildConfig, error)
 }

+// BuildConfigLister provides methods for listing the BuildConfigs.


The changes to this file may now be superfluous.

Fixed in next push

jim-minter · 2017-04-27T15:31:47Z

pkg/build/controller/common/util.go

+		return nil
+	}
+
+	successfulBuilds, err := buildutil.BuildConfigBuilds(buildLister, build.Namespace, bcName, isCompletedBuild)


if you move these lines into if buildConfig.Spec.SuccessfulBuildsHistoryLimit != nil { (x2), you can remove the if clause above.

fixed in next push

jim-minter · 2017-04-27T15:40:21Z

pkg/build/controller/common/util.go

+	if buildConfig.Spec.SuccessfulBuildsHistoryLimit != nil {
+		successfulBuildsHistoryLimit := int(*buildConfig.Spec.SuccessfulBuildsHistoryLimit)
+		glog.V(4).Infof("Preparing to prune %v old successful builds.", (len(successfulBuilds.Items) - successfulBuildsHistoryLimit))
+		if successfulBuildsHistoryLimit == 0 && len(successfulBuilds.Items) > 0 {


I don't think the if ... else is necessary, I think just the following will do:

if len(successfulBuilds.Items) > successfulBuildsHistoryLimit { buildsToDelete = append(buildsToDelete, successfulBuilds.Items[successfulBuildsHistoryLimit:]...) }

Yep, that's a leftover from the way it was being done before, fixed in next push

jim-minter · 2017-04-27T15:41:32Z

pkg/build/api/v1/types.go

+
+	// SuccessfulBuildsHistoryLimit is the number of old successful builds to retain.
+	// This field is a pointer to allow for differentiation between an explicit zero and not specified.
+	SuccessfulBuildsHistoryLimit *int32 `json:"successfulBuildsHistoryLimit,omitempty" protobuf:"varint,4,opt,name=successfulBuildsHistoryLimit"`


I think you need to validate that these values, if they exist, are >= 0

implemented in next push

deads2k · 2017-04-27T18:15:49Z

re[test]

deads2k · 2017-04-27T18:18:27Z

re[test]

0xmichalis · 2017-04-27T19:26:28Z

pkg/build/api/validation/validation.go

@@ -148,6 +148,18 @@ func ValidateBuildConfig(config *buildapi.BuildConfig) field.ErrorList {
 			"run policy must Parallel, Serial, or SerialLatestOnly"))
 	}

+	if config.Spec.SuccessfulBuildsHistoryLimit != nil {
+		if *config.Spec.SuccessfulBuildsHistoryLimit < 0 {


There is a ValidateNonnegativeField helper

fixed in next push

0xmichalis · 2017-04-27T19:32:49Z

pkg/build/controller/buildpod/controller.go

@@ -236,7 +246,7 @@ func (bc *BuildPodController) HandlePod(pod *kapi.Pod) error {
 		case buildapi.BuildPhaseError, buildapi.BuildPhaseFailed:
 			bc.recorder.Eventf(build, kapi.EventTypeNormal, buildapi.BuildFailedEventReason, fmt.Sprintf(buildapi.BuildFailedEventMessage, build.Namespace, build.Name))
 		}
-		common.HandleBuildCompletion(build, bc.runPolicies)
+		common.HandleBuildCompletion(build, bc.buildLister, bc.buildConfigGetter, bc.buildDeleter, bc.runPolicies)


Handling this only when a running build transitions to a new phase is not optimal. For example, if I set the limits to a BC without running any build, I would expect any prunable builds to be removed.

if you are just setting the limits on a BC, what would trigger the pruning then? saving the BC?

0xmichalis · 2017-04-27T19:34:06Z

pkg/build/controller/common/util.go

+		successfulBuildsHistoryLimit := int(*buildConfig.Spec.SuccessfulBuildsHistoryLimit)
+		glog.V(4).Infof("Preparing to prune %v old successful builds.", (len(successfulBuilds.Items) - successfulBuildsHistoryLimit))
+		if len(successfulBuilds.Items) > successfulBuildsHistoryLimit {
+			buildsToDelete = append(buildsToDelete, successfulBuilds.Items...)


You shouldn't append all of the builds?

fixed in next push (delete the wrong line!)

0xmichalis · 2017-04-27T19:35:41Z

pkg/build/controller/common/util.go

+	for i, b := range buildsToDelete {
+		glog.V(4).Infof("Pruning old build: %s/%s", b.Namespace, b.Name)
+		if err := buildDeleter.DeleteBuild(&buildsToDelete[i]); err != nil {
+			return err


You can continue deleting other builds and return an aggregate error.

Just concat all the err strings?

There is a NewAggregate helper which takes a slice of errors

bparees · 2017-04-27T20:15:43Z

pkg/build/api/validation/validation.go

@@ -148,6 +148,16 @@ func ValidateBuildConfig(config *buildapi.BuildConfig) field.ErrorList {
 			"run policy must Parallel, Serial, or SerialLatestOnly"))
 	}

+	if config.Spec.SuccessfulBuildsHistoryLimit != nil {
+		allErrs = append(allErrs, validation.ValidateNonnegativeField(int64(*config.Spec.SuccessfulBuildsHistoryLimit), specPath.Child("successfulBuildsHistoryLimit"))...)
+


delete newline

updated in next push

bparees · 2017-04-27T20:15:46Z

pkg/build/api/validation/validation.go

+
+	if config.Spec.FailedBuildsHistoryLimit != nil {
+		allErrs = append(allErrs, validation.ValidateNonnegativeField(int64(*config.Spec.FailedBuildsHistoryLimit), specPath.Child("failedBuildsHistoryLimit"))...)
+


delete newline

updated in next push

bparees · 2017-04-27T20:21:26Z

pkg/build/controller/buildpod/controller.go

@@ -64,13 +68,19 @@ func NewBuildPodController(buildInformer cache.SharedIndexInformer, podInformer
 	eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: v1core.New(extkc.Core().RESTClient()).Events("")})

 	buildListerUpdater := buildclient.NewOSClientBuildClient(oc)
+	buildConfigGetter := buildclient.NewOSClientBuildConfigClient(oc)
+	buildDeleter := pruner.NewBuildDeleter(oc)


refactor BuildDeleter out of the prune backage and into where the other build clients live.

fixed in next push

bparees · 2017-04-27T20:24:27Z

pkg/build/controller/common/util.go

+
+func isFailedBuild(build buildapi.Build) bool {
+	return build.Status.Phase == buildapi.BuildPhaseFailed
+}


i don't think it's worth introducing these helpers unless you're going to check multiple phases, but if you are set on keeping them, please move them into pkg/build/util.go where we already have some helpers for checking build phases.

Note that there is an existing IsBuildComplete helper which checks if the build is in any terminal state. you might want to rename it to IsBuildTerminated or change it to IsBuildActive and reverse all the boolean checks that were calling the old function.

fixed in next push

bparees · 2017-04-27T20:26:22Z

pkg/build/controller/common/util.go

+}
+
+func (b ByStartTimestamp) Less(i, j int) bool {
+	return !b[j].Status.StartTimestamp.Time.After(b[i].Status.StartTimestamp.Time)


i'd expect to sort by creation time, not start time.

fixed in next push

bparees · 2017-04-27T20:27:39Z

pkg/build/controller/common/util.go

+// HandleBuildPruning handles the deletion of old successful and failed builds
+// based on settings in the BuildConfig.
+func HandleBuildPruning(build *buildapi.Build, buildLister buildclient.BuildLister, buildConfigGetter buildclient.BuildConfigGetter, buildDeleter pruner.BuildDeleter) error {
+	bcName := buildutil.ConfigNameForBuild(build)


if you get back nil/emptystring here, just bail, it means the build isn't associated with a buildconfig so there's nothing to do.

bparees · 2017-04-27T20:30:05Z

pkg/build/controller/common/util_test.go

+		t.Errorf("should have set the CompletionTimestamp, but instead it was nil")
+	}
+	if build.Status.Duration > 0 {
+		t.Errorf("should have set the Durationi to 0s, but instead it was %v", build.Status.Duration)


s/Durationi/Duration/

fixed in next push

bparees · 2017-04-27T20:31:41Z

pkg/build/controller/common/util_test.go

+		},
+		Spec: buildapi.BuildConfigSpec{
+			SuccessfulBuildsHistoryLimit: &buildsToKeep,
+			FailedBuildsHistoryLimit:     &buildsToKeep,


use different numbers for the failed count and successful count. by using the same number you have no way of knowing if your code is using the right field for the right build type.

fixed in next push

bparees · 2017-04-27T20:33:40Z

pkg/build/controller/common/util.go

+func HandleBuildPruning(build *buildapi.Build, buildLister buildclient.BuildLister, buildConfigGetter buildclient.BuildConfigGetter, buildDeleter pruner.BuildDeleter) error {
+	bcName := buildutil.ConfigNameForBuild(build)
+
+	buildConfig, err := buildConfigGetter.Get(build.Namespace, bcName, metav1.GetOptions{})


instead of going to the api to the get the buildconfig (and the builds), we should be going to the sharedinformer cache. @csrwng might be able to give you some pointers.

coreydaley · 2017-05-02T16:26:27Z

@bparees @Kargakis ptal

bparees

a handful of minor nits, a follow up for using caches, and also i'd like to see at least one extended test case that also validates the pruning behavior for both completed and failed builds.

bparees · 2017-05-04T03:12:37Z

pkg/build/controller/buildconfig_controller.go

 	// recorder is used to record events.
 	Recorder record.EventRecorder
 }

 func (c *BuildConfigController) HandleBuildConfig(bc *buildapi.BuildConfig) error {
 	glog.V(4).Infof("Handling BuildConfig %s/%s", bc.Namespace, bc.Name)

+	glog.V(4).Infof("Handling Build Pruning: %v/%v", bc.Namespace, bc.Name)


this logging is redundant, it doesn't add any information to what was logged above.

fixed in next push

bparees · 2017-05-04T03:17:34Z

pkg/build/controller/buildconfig_controller.go

+	glog.V(4).Infof("Handling Build Pruning: %v/%v", bc.Namespace, bc.Name)
+	if err := buildutil.HandleBuildPruning(bc.Name, bc.Namespace, c.BuildLister, c.BuildConfigGetter, c.BuildDeleter); err != nil {
+		return err
+	}


why isn't this returning the same wrapper error message as HandleBuildCompletion does when it has an error pruning?

i think you should proceed through the rest of the HandleBuildConfig logic and return an aggregated error object at the end, rather than quitting here if something goes wrong. Even if pruning fails, we still want to kick off the new build.

(Unless the HandleBuildConfig event gets retried when an error is encountered?)

fixed in next push

bparees · 2017-05-04T03:22:15Z

pkg/build/controller/common/util.go

+		sort.Sort(ByCreationTimestamp(successfulBuilds.Items))
+
+		successfulBuildsHistoryLimit := int(*buildConfig.Spec.SuccessfulBuildsHistoryLimit)
+		glog.V(4).Infof("Preparing to prune %v old successful builds.", (len(successfulBuilds.Items) - successfulBuildsHistoryLimit))


move this log into the if block, and/or log more details: number of builds found, number of builds configured to keep, number that will be pruned.

fixed in next push

bparees · 2017-05-04T03:22:35Z

pkg/build/controller/common/util.go

+		sort.Sort(ByCreationTimestamp(failedBuilds.Items))
+
+		failedBuildsHistoryLimit := int(*buildConfig.Spec.FailedBuildsHistoryLimit)
+		glog.V(4).Infof("Preparing to prune %v old failed builds.", (len(failedBuilds.Items) - failedBuildsHistoryLimit))


same for this log message.

fixed in next push

bparees · 2017-05-04T03:24:36Z

pkg/build/controller/common/util_test.go

+	}
+
+	if !reflect.DeepEqual(failedStartingBuilds.Items[:3], failedRemainingBuilds.Items) {
+		t.Errorf("the two most recent failed builds should be left, but they were not")


s/two/three/

and for both this and the case above, it's generally useful to print the actual and expected values (in this case the lists of builds) to help debug if it does fail.

fixed in next push

bparees · 2017-05-04T03:27:39Z

pkg/build/controller/common/util.go

+
+// HandleBuildPruning handles the deletion of old successful and failed builds
+// based on settings in the BuildConfig.
+func HandleBuildPruning(buildConfigName string, namespace string, buildLister buildclient.BuildLister, buildConfigGetter buildclient.BuildConfigGetter, buildDeleter buildclient.BuildDeleter) error {


i'm still a little bothered this function isn't using the shared informer caches to get the buildconfig+builds, but we can fix it in a follow up. Please open an issue to track the need to deal with it.

Opened #14049

bparees · 2017-05-15T17:38:01Z

test/extended/builds/build_pruning.go

+			fmt.Fprintf(g.GinkgoWriter, "%v", err)
+		}
+
+		o.ExpectWithOffset(1, int32(len(builds.Items))).To(o.Equal(*buildConfig.Spec.SuccessfulBuildsHistoryLimit), "there should be %v completed builds left after pruning, but instead there were %v", *buildConfig.Spec.SuccessfulBuildsHistoryLimit, len(builds.Items))


why is this an expect with offset?

fixed in next push

bparees · 2017-05-15T17:38:15Z

test/extended/builds/build_pruning.go

+			fmt.Fprintf(g.GinkgoWriter, "%v", err)
+		}
+
+		o.ExpectWithOffset(1, int32(len(builds.Items))).To(o.Equal(*buildConfig.Spec.FailedBuildsHistoryLimit), "there should be %v failed builds left after pruning, but instead there were %v", *buildConfig.Spec.FailedBuildsHistoryLimit, len(builds.Items))


this one too

fixed in next push

liggitt · 2017-05-15T17:59:44Z

API LGTM (matches successfulJobsHistoryLimit/failedJobsHistoryLimit naming and types)

bparees · 2017-05-15T20:16:43Z

flake #14197

[merge]

bparees · 2017-05-16T01:44:58Z

@coreydaley you pushed another commit, what did you change? that cancels my merge tag, fyi.

coreydaley · 2017-05-16T04:04:35Z

@bparees I just rebased, didn't see your merge tag, was trying to rerun the tests.

bparees · 2017-05-16T04:22:34Z

@coreydaley you appear to have picked up a compile error w/ your rebase due to a signature change:

[WARNING] `go test` had the following output to stderr:
# github.com/openshift/origin/pkg/build/controller/common
pkg/build/controller/common/util_test.go:91: not enough arguments in call to SetBuildCompletionTimeAndDuration

Adds ability to set successfulBuildsHistoryLimit and failedBuildsHistoryLimit on buildConfigs which will prune old builds. Closes #13640 Completes https://trello.com/c/048p7YRO/1044-5-cleanup-policy-for-builds-builds

coreydaley · 2017-05-16T14:53:08Z

@bparees Fixed in latest push

openshift-bot · 2017-05-16T15:07:08Z

Evaluated for origin test up to 2afdad0

openshift-bot · 2017-05-16T15:07:17Z

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1496/) (Base Commit: b96ef9a)

openshift-bot · 2017-05-16T15:07:19Z

Evaluated for origin testextended up to 2afdad0

openshift-bot · 2017-05-16T17:01:01Z

continuous-integration/openshift-jenkins/test SUCCESS

openshift-bot · 2017-05-16T17:06:00Z

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

coreydaley · 2017-05-16T17:39:35Z

@bparees ptal and hit me with another merge tag

bparees · 2017-05-16T17:46:46Z

[merge]
/kabam

bparees · 2017-05-19T00:46:58Z

flake #14236
[merge]

bparees · 2017-05-19T04:19:50Z

flake #14241
[merge]

bparees · 2017-05-19T13:51:30Z

flake #14129
[merge]

bparees · 2017-05-19T16:11:25Z

flake #13980
[merge]

bparees · 2017-05-19T19:25:24Z

flake #14259
[merge]

openshift-bot · 2017-05-19T19:29:39Z

Evaluated for origin merge up to 2afdad0

openshift-bot · 2017-05-20T00:37:46Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/712/) (Base Commit: 938d40f) (Image: devenv-rhel7_6247)

php-coder · 2017-06-08T11:22:17Z

test/extended/builds/build_pruning.go

+
+		builds, err := oc.Client().Builds(oc.Namespace()).List(metav1.ListOptions{})
+		if err != nil {
+			fmt.Fprintf(g.GinkgoWriter, "%v", err)


Here and above: was it intentionally to not print a newline at the end of the error?

jim-minter reviewed Apr 27, 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 27, 2017

coreydaley changed the title ~~(WIP) Cleanup policy for builds~~ Cleanup policy for builds Apr 27, 2017

jim-minter reviewed Apr 27, 2017

View reviewed changes

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

0xmichalis reviewed Apr 27, 2017

View reviewed changes

bparees requested changes Apr 27, 2017

View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 3, 2017

bparees requested changes May 4, 2017

View reviewed changes

bparees added the needs-api-review label May 15, 2017

bparees reviewed May 15, 2017

View reviewed changes

liggitt added api-approved and removed needs-api-review labels May 15, 2017

(WIP) Cleanup policy for builds

2afdad0

Adds ability to set successfulBuildsHistoryLimit and failedBuildsHistoryLimit on buildConfigs which will prune old builds. Closes #13640 Completes https://trello.com/c/048p7YRO/1044-5-cleanup-policy-for-builds-builds

bparees approved these changes May 19, 2017

View reviewed changes

openshift-bot merged commit 2c04b90 into openshift:master May 20, 2017

coreydaley mentioned this pull request Jun 5, 2017

Add documentation for build pruning openshift/openshift-docs#4536

Merged

php-coder reviewed Jun 8, 2017

View reviewed changes


		if bcList.Items[0].Spec.FailedBuildsHistoryLimit != nil \|\| bcList.Items[0].Spec.SuccessfulBuildsHistoryLimit != nil {

		bList, err := buildLister.List(build.Namespace, kapi.ListOptions{


		if config.Spec.FailedBuildsHistoryLimit != nil {
		allErrs = append(allErrs, validation.ValidateNonnegativeField(int64(*config.Spec.FailedBuildsHistoryLimit), specPath.Child("failedBuildsHistoryLimit"))...)

Cleanup policy for builds #13788

Cleanup policy for builds #13788

Conversation

coreydaley commented Apr 17, 2017

coreydaley commented Apr 17, 2017

coreydaley commented Apr 26, 2017

coreydaley commented Apr 26, 2017

coreydaley commented Apr 27, 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

coreydaley commented Apr 27, 2017

coreydaley commented Apr 27, 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

deads2k commented Apr 27, 2017

deads2k commented Apr 27, 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

coreydaley commented May 2, 2017

bparees 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

liggitt commented May 15, 2017 • edited Loading

bparees commented May 15, 2017

bparees commented May 16, 2017

coreydaley commented May 16, 2017

bparees commented May 16, 2017

coreydaley commented May 16, 2017

liggitt commented May 15, 2017 •

edited

Loading