Support valueFrom syntax for build env vars #14143

coreydaley · 2017-05-11T14:06:22Z

Trello card: https://trello.com/c/lDxfG8GO/1062-8-support-valuefrom-syntax-for-build-env-vars-techdebt
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1402468

coreydaley · 2017-05-11T14:06:41Z

(8) Support valueFrom syntax for build env vars [techdebt]

coreydaley · 2017-05-24T18:33:43Z

[test]

coreydaley · 2017-05-24T18:34:29Z

[testextended][extended:core(builds)]

coreydaley · 2017-05-25T18:22:26Z

@openshift/devex ptal

bparees · 2017-05-25T20:56:55Z

pkg/build/admission/defaults/api/validation/validation_test.go

+				},
+			},
+			errExpected: true,
+			errField:    "env[0].valueFrom.ResourceFieldRef",


if this isn't supported, is there another way for the user to reference the memory/cpu resource limits/requests for the build?

bparees · 2017-05-25T20:57:48Z

pkg/build/api/types.go

@@ -475,6 +475,10 @@ const (
 	// build pod but one was already present.
 	StatusReasonBuildPodExists StatusReason = "BuildPodExists"

+	// StatusReasonInvalidValueFrom indicates that and error occurred processing
+	// the supplied options for valueFrom in the build strategy environment
+	StatusReasonInvalidValueFrom StatusReason = "InvalidValueFrom"


EnvValueFrom

bparees · 2017-05-25T21:00:24Z

pkg/build/controller/build_controller.go

+		build.Status.Reason = buildapi.StatusReasonInvalidValueFrom
+		return err
+	}
+	*originalEnvVars = *processedEnvVars


this should be done at the same point where we apply env from the defaults/overrides.

in fact this processing should be done after that, so it can take into account default/override values that use a valuefrom reference.

bparees · 2017-05-25T21:01:35Z

pkg/build/controller/common/util.go

+		case e.Value != "":
+			value = e.Value
+		case e.ValueFrom.FieldRef != nil:
+			value, err = envutil.GetEnvVarFieldRef(build, e.ValueFrom)


so would they handle a resource ref by using a field ref?

bparees · 2017-05-25T21:06:56Z

pkg/build/controller/common/util.go

+		return nil, nil, utilerrors.NewAggregate(allErrs)
+	}
+
+	return inputEnv, &outputEnv, nil


i think i see why you did it, but i think it's weird you're returning the inputenv here. i think you should just return the resolved env.

(and make the caller get a handle to the envvar array they want to overwrite with the new values)

fixed in next push

bparees · 2017-05-25T21:16:34Z

pkg/util/env/env.go

+	}
+}
+
+func GetEnvVarSecretRefValue(client kclientset.Interface, namespace string, store *ResourceStore, secretSelector *kapi.SecretKeySelector) (string, error) {


this function has nothing to do w/ EnvVar so don't include it in the name. find some other way to disambiguate it from the private function.

fixed in next push

bparees · 2017-05-25T21:18:16Z

pkg/util/env/env.go

+	return GetEnvVarSecretRefValue(kubeClient, namespace, store, secretSelector)
+}
+
+func GetEnvVarConfigMapRefValue(client kclientset.Interface, namespace string, store *ResourceStore, configMapSelector *kapi.ConfigMapKeySelector) (string, error) {


this function has nothing to do w/ EnvVar so don't include it in the name. find some other way to disambiguate it from the private function.

fixed in next push

bparees · 2017-05-25T21:21:47Z

pkg/util/env/env.go

@@ -0,0 +1,134 @@
+package env


godoc for all the functions in this file.

Fixed in next push

bparees · 2017-05-25T21:25:58Z

pkg/util/env/env.go

+	return fieldpath.InternalExtractContainerResourceValue(from.ResourceFieldRef, c)
+}
+
+func GetEnvVarRefValue(f *clientcmd.Factory, store *ResourceStore, from *kapi.EnvVarSource, obj runtime.Object, c *kapi.Container) (string, error) {


why isn't there an equivalent of this for the build logic to call, instead of including all the switching in the build controller?

excellent idea, refactored in next push

bparees · 2017-05-25T21:26:49Z

pkg/build/controller/common/util.go

+		case e.Value != "":
+			value = e.Value
+		case e.ValueFrom.FieldRef != nil:
+			value, err = envutil.GetEnvVarFieldRef(build, e.ValueFrom)


so this is how they could reference the build limits/request values?

jim-minter · 2017-05-26T10:08:41Z

pkg/build/api/validation/validation.go

@@ -683,7 +683,12 @@ func ValidateStrategyEnv(vars []kapi.EnvVar, fldPath *field.Path) field.ErrorLis
 			allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, cIdentifierErrorMsg))
 		}
 		if ev.ValueFrom != nil {
-			allErrs = append(allErrs, field.Invalid(idxPath.Child("valueFrom"), ev.ValueFrom, "valueFrom is not supported in build strategy environment variables"))
+			if ev.Value != "" {
+				allErrs = append(allErrs, field.Duplicate(idxPath.Child("value"), fmt.Sprintf("cannot use 'value' and 'valueFrom' together for %v", ev.Name)))


AFAICS Kubernetes allows the user to specify value and valueFrom. If value is set, it takes precedence. I can see advantages in behaving in the same way, and I can see also advantages in being more restrictive. @bparees do you have an opinion on this?

@jim-minter good catch. we should be matching the k8s behavior.

fixed in next push

jim-minter · 2017-05-26T10:11:56Z

pkg/build/api/validation/validation.go

+			if ev.Value != "" {
+				allErrs = append(allErrs, field.Duplicate(idxPath.Child("value"), fmt.Sprintf("cannot use 'value' and 'valueFrom' together for %v", ev.Name)))
+			}
+			if ev.ValueFrom.ResourceFieldRef != nil {


I think that we can/should remove this code in favour of ValidateEnv() from vendor/k8s.io/kubernetes/pkg/api/validation, with an additional check if we don't permit ResourceFieldRef != nil.

jim-minter · 2017-05-26T10:15:30Z

pkg/build/api/types.go

@@ -710,7 +714,6 @@ type CustomBuildStrategy struct {
 	PullSecret *kapi.LocalObjectReference

 	// Env contains additional environment variables you want to pass into a builder container.
-	// ValueFrom is not supported.


Please also update docs in v1/types.go

fixed in next push

jim-minter · 2017-05-26T10:34:43Z

pkg/build/controller/common/util.go

+		var err error
+
+		if e.Value != "" {
+			r, err := regexp.Compile(`(\$\([A-Z_]+\))`)


If you have to use regexes, they should be package-level and non-exported, initialised using regex.MustCompile().

fixed in next push

jim-minter · 2017-05-26T10:54:43Z

pkg/build/controller/build_controller.go

+		build.Status.Reason = buildapi.StatusReasonInvalidEnvironmentVariable
+		return err
+	}
+	*originalEnvVars = *processedEnvVars


I think this code is mis-located. I think it is, or almost certainly will become, a problem to modify the original Build object in memory. You will need to locate this code at least after the kapi.Scheme.Copy in nextBuildPhase() and do your work on the buildCopy object.

(Also, I remember the discussion but not the outcome of it: was the decision definitely to update the build object with the resolved env vars as well as the resulting pod, or was it just the latter?)

The code for this is getting refactored with my pull #14289. At some point we'll need to reconcile.

I think i had a comment elsewhere that the processing of the env vars should be done in the same place/way that we're doing it for the builddefaulter/overrider.

jim-minter · 2017-05-26T11:00:28Z

pkg/build/controller/common/util.go

+// ProcessValueFrom supports the k8s pod syntax of allowing "valueFrom" in addition
+// to "value", where "valueFrom" uses the downward api to reference existing env
+// variables, jsonpath references to the build object, secrets, and configmaps.
+func ProcessValueFrom(build *buildapi.Build, client kclientset.Interface) (*[]kapi.EnvVar, error) {


(comment applies to whole PR) I think that working with types of *[]Foo is generally an antipattern. The implementation of []Foo is that it already contains a pointer to an underlying shared array. For this PR I'd sooner see rework to use []kapi.EnvVar; perhaps this will necessitate the addition of a helper function such as PutEnvToStrategy (or something), but I think that's preferable to *[]kapi.EnvVar.

given that this is a return value and not a parameter, it should be entirely possible to just return []Foo and let the caller look up the pointer if it wants to use it.

I'm disagreeing with the use of *[]kapi.EnvVar throughout this PR, not just as a return type here.

updated in next push

jim-minter · 2017-05-26T11:21:27Z

pkg/build/controller/common/util.go

+		var err error
+
+		if e.Value != "" {
+			r, err := regexp.Compile(`(\$\([A-Z_]+\))`)


This section is quite problematic. Please take a look at the use of k8s.io/kubernetes/third_party/forked/golang/expansion.Expand() in vendor/k8s.io/kubernetes/pkg/kubelet/kubelet_pods.go and use that as a model.

fixed in next push

jim-minter · 2017-05-26T11:26:58Z

pkg/build/generated/informers/externalversions/build/v1/interface.go

@@ -1,27 +0,0 @@
-// This file was automatically generated by informer-gen


Is the deletion of these files intentional?

Yes, ben told me to delete them

no i told you to delete the buildconfig related generated files:

you also seem to have fallen victim to the unnecessary buildconfig informer generation. delete all these generated files.

meaning the new ones your PR was introducing, not the existing ones.

jim-minter · 2017-05-26T11:34:18Z

pkg/build/util/util.go

@@ -307,3 +307,19 @@ func SafeForLoggingS2IConfig(config *s2iapi.Config) *s2iapi.Config {
 	newConfig.ScriptsURL, _ = s2iutil.SafeForLoggingURL(newConfig.ScriptsURL)
 	return &newConfig
 }
+
+// Gets the Env section of the build config regardless of the build strategy
+func GetEnvFromStrategy(build *buildapi.Build) *[]kapi.EnvVar {


This is duplicating non-exported code at least in pkg/build/admission/buildpodutil.go, pkg/build/admission/defaults/admission.go and pkg/build/util/util.go.
If you end up adding an update equivalent, there's one already in pkg/build/generator/generator.go.
If this isn't dealt with in this PR, please could you open a follow-on issue?

I refactored the updateBuildEnv from generator.go into pkg/build/util/util.go in the next push, and refactored pkg/build/admission/defaults/admission.go to use the new GetBuildEnv function that I added.

If you are referring to the buildEnvVar function in buildpodutil.go, that seems to not be doing the same thing that these are?

jim-minter · 2017-05-26T12:42:37Z

test/extended/builds/valuefrom.go

+		err := oc.Run("create").Args("-f", stiImageStreamFixture).Execute()
+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		g.By("creating test secret")


push this and the "creating test configmap" into g.JustBeforeEach() above to save duplication of code in each g.It()?

Fixed in next push

jim-minter · 2017-05-26T12:58:17Z

test/extended/builds/valuefrom.go

+
+	})
+
+	g.It("should fail processing ResourceFieldRef and value/valueFrom in s2i build strategy environment variables", func() {


the negative case shouldn't require an extended test

Fixed in next push

jim-minter · 2017-05-26T13:03:34Z

test/extended/builds/valuefrom.go

+
+	})
+
+	g.It("should fail processing reference $(ENV_VAR) to missing s2i build strategy environment variable", func() {


(refers to this and subsequent test:) Kubernetes doesn't class an unresolvable reference as an error (it just leaves $(ENV_VAR) in the input). Should we really diverge?

Fixed in next push

jim-minter · 2017-05-26T13:26:58Z

test/extended/builds/valuefrom.go

+
+)
+
+var _ = g.Describe("[builds][valueFrom] process valueFrom in build strategy environment variables", func() {


[Conformance] ?

fixed in next push

coreydaley · 2017-06-02T19:47:04Z

test/flake #14462

coreydaley · 2017-06-05T20:25:32Z

test/flake #14353

jim-minter · 2017-06-06T09:21:42Z

pkg/build/util/util.go

@@ -307,3 +307,41 @@ func SafeForLoggingS2IConfig(config *s2iapi.Config) *s2iapi.Config {
 	newConfig.ScriptsURL, _ = s2iutil.SafeForLoggingURL(newConfig.ScriptsURL)
 	return &newConfig
 }
+
+// GetBuildEnv gets the build strategy environment
+func GetBuildEnv(build *buildapi.Build) *[]kapi.EnvVar {


Should be func GetBuildEnv(build *buildapi.Build) []kapi.EnvVar

updated in next push

jim-minter · 2017-06-06T09:23:44Z

pkg/build/controller/common/util.go

+		mapEnvs[e.Name] = value
+	}
+
+	if allErrs != nil {


if len(allErrs) > 0 is a little more robust.

updated in next push

jim-minter · 2017-06-06T09:27:25Z

pkg/build/controller/common/util.go

+	buildEnv := buildutil.GetBuildEnv(build)
+	*buildEnv = outputEnv
+
+	return buildadmission.SetBuildInPod(pod, build, version)


Can SetBuildInPod be moved to pkg/build/util?

Re-factor at a later date

jim-minter · 2017-06-06T09:27:46Z

pkg/build/controller/common/util.go

+	var outputEnv []kapi.EnvVar
+	var allErrs []error
+
+	build, version, err := buildadmission.GetBuildFromPod(pod)


Can GetBuildInPod be moved to pkg/build/util?

I guess the answer to that may be, excessive work for this PR but worth scheduling for tech debt?

This is probably a good candidate for re-factoring at a later date.

jim-minter · 2017-06-06T09:28:23Z

pkg/build/controller/common/util.go

@@ -134,3 +140,48 @@ func HasBuildPodNameAnnotation(build *buildapi.Build) bool {
 	_, hasAnnotation := build.Annotations[buildapi.BuildPodNameAnnotation]
 	return hasAnnotation
 }
+
+// ProcessValueFrom supports the k8s pod syntax of allowing "valueFrom" in addition


A little more godoc might be nice - say something about what the function actually does

jim-minter · 2017-06-06T09:34:12Z

pkg/build/api/types.go

@@ -475,6 +475,10 @@ const (
 	// build pod but one was already present.
 	StatusReasonBuildPodExists StatusReason = "BuildPodExists"

+	// StatusReasonInvalidEnvironmentVariable indicates that an error occurred processing
+	// the supplied options for environment variables in the build strategy environment
+	StatusReasonInvalidEnvironmentVariable StatusReason = "InvalidEnvironmentVariable"


There's a bit of a mismatch between the name of this error and the actual reasons why ProcessValueFrom might return - is it possible to tighten this up at all? How do other parts of origin/k8s report this type of error?

@jim-minter are you suggesting a name more like invalidenvironmentvariablereference or invalidenvironmentvariablefrom or something?

For my part i'm ok with this as is, but maybe an easy improvement would be "UnresolveableEnvironmentVariable"?

updated in next push

jim-minter · 2017-06-06T09:58:46Z

pkg/build/api/validation/validation.go

@@ -682,8 +682,8 @@ func ValidateStrategyEnv(vars []kapi.EnvVar, fldPath *field.Path) field.ErrorLis
 		} else if len(kvalidation.IsCIdentifier(ev.Name)) > 0 {
 			allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, cIdentifierErrorMsg))
 		}
-		if ev.ValueFrom != nil {
-			allErrs = append(allErrs, field.Invalid(idxPath.Child("valueFrom"), ev.ValueFrom, "valueFrom is not supported in build strategy environment variables"))
+		if ev.ValueFrom != nil && ev.ValueFrom.ResourceFieldRef != nil {


ValidateStrategyEnv is called from ValidateBuildDefaultsConfig - may want to continue to prevent BuildDefaults from using ValueFrom - @bparees?

@jim-minter i don't see a compelling reason to disallow ValueFrom in BuildDefaults, do you have a specific concern?

bparees · 2017-06-12T01:18:47Z

flake #13977
flake #14575
flake #14573

bparees · 2017-06-13T16:19:07Z

flake #14615
[test]

bparees · 2017-06-13T17:09:26Z

extended test failure is a known flake.
[merge][severity:blocker]

Trello card: https://trello.com/c/lDxfG8GO/1062-8-support-valuefrom-syntax-for-build-env-vars-techdebt

coreydaley · 2017-06-15T16:58:41Z

@bparees @csrwng ptal

openshift-bot · 2017-06-15T17:06:32Z

Evaluated for origin testextended up to 8e08af2

openshift-bot · 2017-06-15T17:06:43Z

Evaluated for origin test up to 8e08af2

bparees · 2017-06-15T18:31:32Z

lgtm, will merge on passing tests.

openshift-bot · 2017-06-15T18:56:55Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2270/) (Base Commit: 84b8802)

bparees · 2017-06-15T19:34:52Z

[merge][severity:blocker]

openshift-bot · 2017-06-15T19:57:18Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/634/) (Base Commit: 84b8802) (Extended Tests: core(builds))

openshift-bot · 2017-06-16T21:55:52Z

Evaluated for origin merge up to 8e08af2

openshift-bot · 2017-06-17T00:07:43Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1024/) (Base Commit: 517be84) (PR Branch Commit: 8e08af2) (Extended Tests: blocker)

bparees · 2017-06-17T00:11:53Z

unrelated failure, has passing tests. manually merging.

=== BEGIN TEST CASE ===
test/cmd/images_tests.sh:125: executing 'oc import-image mysql' expecting success and text 'sha256:'
FAILURE after 30.357s: test/cmd/images_tests.sh:125: executing 'oc import-image mysql' expecting success and text 'sha256:': the command returned the wrong error code; the output content test failed
There was no output from the command.
Standard error from the command:
Error from server (Timeout): Timeout: request did not complete within allowed duration
=== END TEST CASE ===

smarterclayton · 2017-06-17T18:27:29Z

Causing origin-gce image merge test to fail: https://ci.openshift.redhat.com/jenkins/job/zz_origin_gce_image/272/

coreydaley · 2017-06-17T18:41:45Z

@smarterclayton @bparees looks like it's because GCE doesn't build new images. I'll submit a pull request to skip those tests on gce.

coreydaley · 2017-06-17T18:48:12Z

pull request #14721 submitted

bparees · 2017-06-17T20:24:19Z

@smarterclayton what's the difference between "test_pull_request_origin_extended_conformance_gce" (which passed) and "zz_origin_gce_image" ?

(I see why the other PR was created now)

smarterclayton · 2017-06-17T20:57:28Z

I'll just add them to the filter list for now - we can talk about an approach that scales re: backwards compat next week.

smarterclayton · 2017-06-18T01:51:06Z

pkg/util/env/env.go

+}
+
+// GenEnvVarRefValue returns the value referenced by the supplied EnvVarSource given the other supplied information
+func GetEnvVarRefValue(f *clientcmd.Factory, kc kclientset.Interface, ns string, store *ResourceStore, from *kapi.EnvVarSource, obj runtime.Object, c *kapi.Container) (string, error) {


Things in pkg/util are not allowed to reference clientcmd.Factory. Only things in a pkg/cmd scope may do that. This needs to be refactored to not reference that from here. It's also not really appropriate to accept client interfaces into util packages unless they are much more tightly scoped. I'd have expected this package to be pkg/pod/envresolve or similar.

smarterclayton · 2017-06-18T01:52:04Z

pkg/util/env/env.go

+	}
+
+	if from.SecretKeyRef != nil {
+		return getSecretRefValue(kubeClient, namespace, store, from.SecretKeyRef)


This method is a security escalation attack waiting to happen. Please add specific separate tests that verify that no call outside of ns is made, and add specific language to the call sites that doing so is not allowed.

Trello card: https://trello.com/c/lDxfG8GO/1062-8-support-valuefrom-syntax-for-build-env-vars-techdebt

Addresses the following comments against the first pull request (#14143): openshift/origin#14143 (review) openshift/origin#14143 (review)

bparees requested changes May 25, 2017

View reviewed changes

jim-minter reviewed May 26, 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 30, 2017

coreydaley changed the title ~~(WIP) Support valueFrom syntax for build env vars~~ Support valueFrom syntax for build env vars Jun 5, 2017

jim-minter reviewed Jun 6, 2017

View reviewed changes

bparees approved these changes Jun 14, 2017

View reviewed changes

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

(WIP) Support valueFrom syntax for build env vars

8e08af2

Trello card: https://trello.com/c/lDxfG8GO/1062-8-support-valuefrom-syntax-for-build-env-vars-techdebt

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

This was referenced Jun 16, 2017

[builds][Conformance][valueFrom] process valueFrom in build strategy environment variables failing on GCE provider #14689

Closed

Add tests for BuildPhaseError #14390

Closed

bparees merged commit 27afdba into openshift:master Jun 17, 2017

smarterclayton reviewed Jun 18, 2017

View reviewed changes

coreydaley mentioned this pull request Jun 19, 2017

Secure valueFrom resolution helper utility function #14749

Merged

simo5 pushed a commit to simo5/origin that referenced this pull request Dec 5, 2017

(WIP) Support valueFrom syntax for build env vars (openshift#14143)

3826736

Trello card: https://trello.com/c/lDxfG8GO/1062-8-support-valuefrom-syntax-for-build-env-vars-techdebt

		@@ -1,27 +0,0 @@
		// This file was automatically generated by informer-gen


		})

		g.It("should fail processing ResourceFieldRef and value/valueFrom in s2i build strategy environment variables", func() {


		})

		g.It("should fail processing reference $(ENV_VAR) to missing s2i build strategy environment variable", func() {


		)

		var _ = g.Describe("[builds][valueFrom] process valueFrom in build strategy environment variables", func() {

Support valueFrom syntax for build env vars #14143

Support valueFrom syntax for build env vars #14143

Conversation

coreydaley commented May 11, 2017 • edited Loading

coreydaley commented May 11, 2017

coreydaley commented May 24, 2017

coreydaley commented May 24, 2017

coreydaley commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 May 30, 2017 • edited Loading

Choose a reason for hiding this comment

jim-minter May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Jun 2, 2017

coreydaley commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

bparees commented Jun 12, 2017 • edited Loading

bparees commented Jun 13, 2017

bparees commented Jun 13, 2017 • edited Loading

coreydaley commented Jun 15, 2017

coreydaley commented May 11, 2017 •

edited

Loading

coreydaley May 30, 2017 •

edited

Loading

jim-minter May 26, 2017 •

edited

Loading

coreydaley commented Jun 5, 2017 •

edited

Loading

bparees commented Jun 12, 2017 •

edited

Loading

bparees commented Jun 13, 2017 •

edited

Loading