✨ crd conversion webhook implementation #351

droot · 2019-03-06T17:57:19Z

This PR implements CRD Conversion Webhook handler.

The design has been documented here.

Note:
Need to add more tests. I am in process of converting the design doc in markdown format (doc link shared above)

droot · 2019-03-07T21:26:07Z

k8s-ci-robot · 2019-03-07T21:26:08Z

@droot: GitHub didn't allow me to request PR reviews from the following users: shawn-hurley, JoelSpeed.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @shawn-hurley @JoelSpeed

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.

JoelSpeed

I've taken a read over the design and over the implementation and left a few comments

I'm so glad you've started work on this @droot as I need to bump a kubebuilder API version soon and need this 😅 Let me know if there's any way I can help with this

JoelSpeed · 2019-03-11T11:45:59Z

pkg/webhook/conversion/conversion.go

+		}
+	}
+
+	// neigher src nor dst are Hub, means both of them are spoke, so lets get the hub


Typo here

Suggested change

// neigher src nor dst are Hub, means both of them are spoke, so lets get the hub

// neither src nor dst are Hub, means both of them are spoke, so lets get the hub

JoelSpeed · 2019-03-11T11:49:14Z

pkg/webhook/conversion/conversion.go

+	if err != nil {
+		return err
+	}
+	// shall we get Hub for dst type as well and ensure hubs are same ?


Commented on the design doc but I think this is probably work doing, the current result, if I've read this properly, is that err = dst.(conversion.Convertible).ConvertFrom(hub) will result in an error later down (approx failed to convert from hub version: unsupported type) which may not be immediately obvious as to what the problem is, WDYT?

Might be worth making this a TODO?

I added a check at the beginning of the function to ensure both src and dst belong to same API group to ensure src/dst have same hub type.

JoelSpeed · 2019-03-11T11:55:33Z

pkg/webhook/conversion/testData/pkg/apis/jobs/v2/externaljob_types.go

+// v2.ExternalJob is the storage version so mark this as Hub
+// Storage version doesn't need to implement any conversion methods because
+// default conversionHandler implements conversion logic for storage version.
+// Adding comment annotation here to mark it as storage version


Annotation not set yet?

JoelSpeed · 2019-03-11T11:59:38Z

pkg/webhook/conversion/testData/pkg/apis/jobs/v1/externaljob_types.go

+
+// since v1.ExternalJob is a spoke, it needs to be convertable.
+// It needs to implement convert to/from storage version
+// TODO(droot): evaluate advantages of taking `conversion.Hub` as input to


My 2 cents on this is that the current design has two ways to indicate that a type is the storage version, the annotation and this conversion.Hub, I would imagine it would be very easy to forget to do one. Is there a good way to remove one of the two?

I had kinda expected ConvertTo and ConvertFrom to take a runtime.Object initially but I concluded that it's probably a disadvantage as you lose some compile time checks theoretically

Is there a good way to remove one of the two?

Annotations (we have started calling them markers now because annotation/tags are so overloaded in k8s world :)) is primarily needed for controller-gen to generate the manifests for conversion webhook. Only way to remove them would be to make controller-gen parse these types and examine if they implement these methods (which can be done, but is a bit stretch looking at current state of the code in controller-gen). Till then, this is the only option.

I had kinda expected ConvertTo and ConvertFrom to take a runtime.Object initially but I concluded that it's probably a disadvantage as you lose some compile time checks theoretically

Agreed. using Hub has type-safety as well readability from an end-user's perspective. So will took out that TODO.

shawn-hurley · 2019-03-11T19:44:49Z

@droot did I miss how someone could move to a new hub version? It was not immediately clear to me how this would work.

I think the overall simplification of the concept is really nice! I need to read through the code still but the design doc is really good!

_{Sent with GitHawk}

DirectXMan12 · 2019-03-11T23:37:38Z

@shawn-hurley I've never seen something that wasn't a little bit painful, but the process here would basically be:

Mark the new type as hub with the interface
Unmark the old type
Migrate your ConvertTo and ConvertFrom methods to the new hub
Add new conversions for your old hub

While this is a bit of a pain, it's not more painful (in my experience) that the k/k method, which, from my experience with HPA v2, looks a lot like:

Update the internal version to have a convenient representation for supporting the new external version
Generate the new generated conversions
Realize that the internal version looks a lot like the new external version, so now you need to rewrite your existing conversions to work with the new internal version
Write conversions for the old external version that used to look a lot like the internal version
(bonus round) realize that the new internal version needs to be slightly different from the external version for some reason (maybe it makes round-tripping a lot easier), so now you have to write conversions for your new external version too.

Now, that's not a high bar, but I also don't see a good way to make things better without supporting arbitrary conversion graphs and gradually growing a strange web of conversions.

pkg/conversion/conversion.go

coderanger · 2019-03-13T17:39:29Z

pkg/webhook/conversion/conversion.go

@@ -0,0 +1,249 @@
+/*
+Copyright 2018 The Kubernetes Authors.


It isn't 2018 :-)

droot · 2019-03-13T20:02:07Z

@JoelSpeed

I'm so glad you've started work on this @droot as I need to bump a kubebuilder API version soon and need this 😅 Let me know if there's any way I can help with this

Awesome. Thanks for reviewing the PR. Usability feedback will be super important and I am glad that you will be using it your project, so keep us posted from end-user's perspective. Have addressed the comments and added a few more tests.

droot · 2019-03-13T20:06:11Z

@shawn-hurley

@droot did I miss how someone could move to a new hub version? It was not immediately clear to me how this would work.

Pretty much along the lines as @DirectXMan12 described above. I will add those to the design doc.

mengqiy · 2019-03-20T20:31:17Z

pkg/webhook/conversion/conversion.go

+	var hub conversion.Hub
+	var isHub, hubFoundAlready bool
+	for _, gvk := range gvks {
+		o, _ := wh.scheme.New(gvk)


The error should be checked here. If the gvk is not registered in the scheme, scheme.New will return an error.

Good pt. Added catch.

mengqiy · 2019-03-20T20:38:24Z

pkg/webhook/conversion/decoder.go

+}
+
+// Decode decodes the inlined object.
+func (d *Decoder) Decode(content []byte) (runtime.Object, *schema.GroupVersionKind, error) {


I feel we can move the decoder in admission pkg to a shared the place and reuse it potentially.

Added a TODO for now. I am sure conversion pkg will evolve as it goes to Beta and we can reevaluate it at that point.

not convinced we need this at all (see above comment)

Yes, @DirectXMan12 is right. We don't need the decoder for conversionRequest now, we only need for processing the objects embedded in the the request.

mengqiy · 2019-03-20T20:39:19Z

pkg/webhook/conversion/response.go

+func errored(err error) *apix.ConversionResponse {
+	return &apix.ConversionResponse{
+		Result: metav1.Status{
+			Code:    http.StatusOK,


It seems to be wrong here.

Good catch, tool it out.

mengqiy · 2019-03-20T20:44:34Z

pkg/webhook/conversion/conversion.go

+		return
+	}
+
+	// TODO(droot): may be move the conversion logic to a separate module to


IMO it's a good idea and align with what admission package. It may look like

func (wh *Webhook) ServeConvert(req ConversionRequest, resp ConversionResponse) { // Do something } func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Decoding http request to get ConversionRequest // Do conversion wh.ServeConvert(convReq, convResp) // Write the conversion response to http.ResponseWriter }

I want to keep this as TODO for now and revisit later. The good thing it can be done as internal refactoring later.

droot

Thanks for the review @mengqiy. Addressed some comments. PTAL.

droot · 2019-03-25T19:01:46Z

pkg/webhook/conversion/conversion.go

+		return
+	}
+
+	// TODO(droot): may be move the conversion logic to a separate module to


I want to keep this as TODO for now and revisit later. The good thing it can be done as internal refactoring later.

droot · 2019-03-25T19:04:54Z

pkg/webhook/conversion/conversion.go

+	var hub conversion.Hub
+	var isHub, hubFoundAlready bool
+	for _, gvk := range gvks {
+		o, _ := wh.scheme.New(gvk)


Good pt. Added catch.

droot · 2019-03-25T19:07:03Z

pkg/webhook/conversion/decoder.go

+}
+
+// Decode decodes the inlined object.
+func (d *Decoder) Decode(content []byte) (runtime.Object, *schema.GroupVersionKind, error) {


Added a TODO for now. I am sure conversion pkg will evolve as it goes to Beta and we can reevaluate it at that point.

droot · 2019-03-25T19:17:23Z

pkg/webhook/conversion/response.go

+func errored(err error) *apix.ConversionResponse {
+	return &apix.ConversionResponse{
+		Result: metav1.Status{
+			Code:    http.StatusOK,


Good catch, tool it out.

droot · 2019-03-25T19:32:18Z

/cc @mbohlool

k8s-ci-robot · 2019-03-25T19:32:19Z

@droot: GitHub didn't allow me to request PR reviews from the following users: mbohlool.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mbohlool

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.

droot · 2019-03-25T19:48:04Z

@mbohlool @mengqiy @JoelSpeed This is good to go from my perspective. I think this is a good starting point to get feedback for the CRD conversion feature.

mengqiy

Code LGTM. I'm OK having some TODOs and revisit them later.

mengqiy · 2019-03-26T02:03:13Z

pkg/webhook/conversion/conversion.go

+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/conversion"
+
+	"encoding/json"


nit: please group the imports.

mengqiy · 2019-03-26T17:43:11Z

pkg/webhook/conversion/conversion_suite_test.go

+
+func TestConversionWebhook(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecsWithDefaultAndCustomReporters(t, "application Suite", []Reporter{envtest.NewlineReporter{}})


application Suite

maybe conversion suite?

mengqiy · 2019-03-26T17:51:34Z

pkg/webhook/conversion/conversion_test.go

+
+	It("should convert objects successfully", func() {
+
+		v1Obj := &jobsv1.ExternalJob{


v1Obj is used in all test cases and is static. It can extract out of It and put it in BeforeEach or others.

I know it looks more verbose in current form, but it makes the test more readable from perspective of input and output and expectations.

I'd just move it to outside, or at least make a helper -- it'll make it way easier to refactor later.

Made a helper for this one because recently added objects using one of the core resource (Deployment).

shawn-hurley

Wonder if it would make sense to add tests testing the src and dest are not convertible or if src is the hub and if dest is the hub and make sure each happy path is covered?

droot · 2019-03-28T18:56:50Z

Wonder if it would make sense to add tests testing the src and dest are not convertible or if src is the hub and if dest is the hub and make sure each happy path is covered?

I just added a test to cover the case where hub is not defined. One test which is missing now is (spoke-to-spoke conversion), I will have to define a two API group under testData to do that.

DirectXMan12 · 2019-03-28T21:40:58Z

pkg/conversion/conversion.go

+limitations under the License.
+*/
+
+package conversion


Godocs on the package

DirectXMan12 · 2019-03-28T21:41:53Z

pkg/conversion/conversion.go

+	ConvertFrom(src Hub) error
+}
+
+// Hub defines capability to indicate whether a versioned type is a Hub or not.


This sentence is a bit awkward in its construction -- I'd go with Hub marks that a given type is the hub type for conversion. This means that all conversions will first convert to the hub type, then convert from the hub type to the destination type. All types besides the hub type should implement Convertible.

Thanks for the suggestion, that reads much better.

DirectXMan12 · 2019-03-28T21:43:02Z

pkg/webhook/conversion/conversion.go

+limitations under the License.
+*/
+
+package conversion


DirectXMan12 · 2019-03-28T21:43:18Z

pkg/webhook/conversion/conversion.go

+)
+
+var (
+	log = logf.Log.WithName("conversion_webhook")


hyphens instead of underscores, generally

DirectXMan12 · 2019-03-28T21:43:33Z

pkg/webhook/conversion/conversion.go

+		return err
+	}
+
+	// inject the decoder here too, just in case the order of calling this is not


This is not needed, so took it out.

DirectXMan12 · 2019-03-28T22:17:27Z

pkg/webhook/conversion/conversion_test.go

+
+		convReview := doRequest(convReq)
+
+		Expect(convReview.Response.ConvertedObjects).Should(HaveLen(1))


Expect().To (more idiomatic)

Good point, reads better. Done

DirectXMan12 · 2019-03-28T22:18:33Z

pkg/webhook/conversion/conversion_test.go

+		}
+
+		convReview := doRequest(convReq)
+		Expect(convReview.Response.Result.Message).Should(


blech. I hate directly checking error message strings. Can't we just check codes?

Need to explore a bit more, but in general I agree checking error messages is bad.

I explored bit more about using codes here, couldn't find an easy way to do it at this level which is conversionRequest/Response level. At this level, we can only indicate status and failure message (which contains the reason for failure). I removed reason string checking here because that makes the tests brittle.

DirectXMan12 · 2019-03-28T22:18:57Z

pkg/webhook/conversion/decoder.go

+}
+
+// Decode decodes the inlined object.
+func (d *Decoder) Decode(content []byte) (runtime.Object, *schema.GroupVersionKind, error) {


not convinced we need this at all (see above comment)

DirectXMan12 · 2019-03-28T22:19:15Z

pkg/webhook/conversion/response.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func errored(err error) *apix.ConversionResponse {


just move this into the other file where it's used, probably

Yes, killed the file.

DirectXMan12 · 2019-03-28T22:19:59Z

pkg/webhook/conversion/testData/pkg/apis/addtoscheme_jobs_v1.go

@@ -0,0 +1,25 @@
+/*


move this under example/ (or just use example/), just to be certain we don't cause import issues

droot

Thanks for the review @DirectXMan12 . Addressed the comments. PTAL.
I have addressed the comments in a separate commit for making it easier to review. I am putting it on hold so that I can squash the commits before the merge.

droot · 2019-04-01T18:37:07Z

pkg/conversion/conversion.go

+	ConvertFrom(src Hub) error
+}
+
+// Hub defines capability to indicate whether a versioned type is a Hub or not.


Thanks for the suggestion, that reads much better.

droot · 2019-04-01T18:37:57Z

pkg/webhook/conversion/conversion.go

+)
+
+var (
+	log = logf.Log.WithName("conversion_webhook")


droot · 2019-04-01T18:39:03Z

pkg/webhook/conversion/conversion.go

+		return err
+	}
+
+	// inject the decoder here too, just in case the order of calling this is not


This is not needed, so took it out.

droot · 2019-04-01T18:42:51Z

pkg/webhook/conversion/conversion.go

+	convertReview := &apix.ConversionReview{}
+	// TODO(droot): figure out if we want to split decoder for conversion
+	// request from the objects contained in the request
+	err = wh.decoder.DecodeInto(body, convertReview)


Actually, you are right. Thanks for pointing out, this has simplified the code even further.

droot · 2019-04-01T18:51:59Z

pkg/webhook/conversion/conversion.go

+	srcGVK := src.GetObjectKind().GroupVersionKind()
+	dstGVK := dst.GetObjectKind().GroupVersionKind()
+
+	if srcGVK.GroupKind().String() != dstGVK.GroupKind().String() {


Yes, you are right :)

droot · 2019-04-01T19:49:31Z

pkg/webhook/conversion/response.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func errored(err error) *apix.ConversionResponse {


Yes, killed the file.

droot · 2019-04-01T20:03:53Z

pkg/webhook/conversion/testData/pkg/apis/addtoscheme_jobs_v1.go

@@ -0,0 +1,25 @@
+/*


droot · 2019-04-01T20:31:21Z

pkg/conversion/conversion.go

+limitations under the License.
+*/
+
+package conversion


droot · 2019-04-01T20:31:34Z

pkg/webhook/conversion/conversion.go

+limitations under the License.
+*/
+
+package conversion


droot · 2019-04-01T20:37:04Z

pkg/webhook/conversion/conversion_test.go

+		}
+
+		convReview := doRequest(convReq)
+		Expect(convReview.Response.Result.Message).Should(


Need to explore a bit more, but in general I agree checking error messages is bad.

droot

@DirectXMan12 I think this is good to go. PTAL a look.

droot · 2019-04-23T18:20:21Z

pkg/webhook/conversion/conversion_test.go

+		}
+
+		convReview := doRequest(convReq)
+		Expect(convReview.Response.Result.Message).Should(


I explored bit more about using codes here, couldn't find an easy way to do it at this level which is conversionRequest/Response level. At this level, we can only indicate status and failure message (which contains the reason for failure). I removed reason string checking here because that makes the tests brittle.

DirectXMan12

/lgtm
/approve

just tackle the nits in a follow-up -- they're not functionally blocking

DirectXMan12 · 2019-05-08T21:31:35Z

pkg/webhook/conversion/conversion.go

+
+// allocateDstObject returns an instance for a given GVK.
+func (wh *Webhook) allocateDstObject(apiVersion, kind string) (runtime.Object, error) {
+	gvk := schema.FromAPIVersionAndKind(apiVersion, kind)


could put this in a sync.Once, or something. We can tackle in a follow-up

DirectXMan12 · 2019-05-08T21:32:24Z

pkg/webhook/conversion/conversion_test.go

+		}
+
+		scheme = kscheme.Scheme
+		Expect(jobsapis.AddToScheme(scheme)).Should(Succeed())


nit: Expect().To

k8s-ci-robot · 2019-05-08T21:34:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, droot

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~~ [DirectXMan12,droot]

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

droot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2019

k8s-ci-robot requested review from DirectXMan12 and pwittrock March 6, 2019 17:57

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2019

droot assigned DirectXMan12 Mar 6, 2019

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 6, 2019

droot requested a review from mengqiy March 6, 2019 17:58

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 6, 2019

droot mentioned this pull request Mar 6, 2019

🏃 Design for CRD version conversion kubernetes-sigs/kubebuilder#624

Merged

JoelSpeed reviewed Mar 11, 2019

View reviewed changes

coderanger reviewed Mar 13, 2019

View reviewed changes

JoelSpeed mentioned this pull request Mar 13, 2019

REQUEST: New membership for JoelSpeed kubernetes/org#596

Closed

6 tasks

droot force-pushed the feature/crd-conversion-webhook branch from a095ab3 to c56ac5b Compare March 13, 2019 19:58

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 13, 2019

droot force-pushed the feature/crd-conversion-webhook branch 2 times, most recently from 9c4ac2f to c5be062 Compare March 13, 2019 20:52

JoelSpeed mentioned this pull request Mar 19, 2019

Resource Deletion Concerns pusher/faros#101

Open

mengqiy reviewed Mar 20, 2019

View reviewed changes

droot force-pushed the feature/crd-conversion-webhook branch from c5be062 to 90b9818 Compare March 25, 2019 19:18

droot commented Mar 25, 2019

View reviewed changes

droot force-pushed the feature/crd-conversion-webhook branch 2 times, most recently from 785e79e to 3b9e1e1 Compare March 25, 2019 19:45

droot changed the title ~~WIP ✨ crd conversion webhook implementation~~ ✨ crd conversion webhook implementation Mar 25, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2019

mengqiy reviewed Mar 26, 2019

View reviewed changes

shawn-hurley reviewed Mar 26, 2019

View reviewed changes

droot force-pushed the feature/crd-conversion-webhook branch from 3b9e1e1 to d88c13b Compare March 28, 2019 18:54

DirectXMan12 suggested changes Mar 28, 2019

View reviewed changes

droot commented Apr 1, 2019

View reviewed changes

droot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2019

droot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2019

droot added 4 commits April 23, 2019 10:18

✨ crd conversion webhook implementation

16c1485

✨ address review comments

cf37108

🏃 Gopkg.lock synchronized

16edb6e

🏃 removed the error message checking from the tests

7cf7a99

droot force-pushed the feature/crd-conversion-webhook branch from 4cac767 to 7cf7a99 Compare April 23, 2019 18:08

droot commented Apr 23, 2019

View reviewed changes

DirectXMan12 suggested changes May 8, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2019

k8s-ci-robot merged commit faa41bc into kubernetes-sigs:master May 8, 2019

	// neigher src nor dst are Hub, means both of them are spoke, so lets get the hub
	// neither src nor dst are Hub, means both of them are spoke, so lets get the hub


		It("should convert objects successfully", func() {

		v1Obj := &jobsv1.ExternalJob{


		convReview := doRequest(convReq)

		Expect(convReview.Response.ConvertedObjects).Should(HaveLen(1))

✨ crd conversion webhook implementation #351

✨ crd conversion webhook implementation #351

Conversation

droot commented Mar 6, 2019 • edited Loading

droot commented Mar 7, 2019

k8s-ci-robot commented Mar 7, 2019

JoelSpeed 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

droot Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

shawn-hurley commented Mar 11, 2019

DirectXMan12 commented Mar 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Mar 13, 2019

droot commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

droot 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

droot commented Mar 25, 2019

k8s-ci-robot commented Mar 25, 2019

droot commented Mar 25, 2019

mengqiy 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

shawn-hurley left a comment

Choose a reason for hiding this comment

droot commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

droot 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

droot commented Mar 6, 2019 •

edited

Loading

droot Mar 13, 2019 •

edited

Loading