Add support for taskRun to expose digest of built images #721

nader-ziada · 2019-04-04T14:22:58Z

Changes

Part of #216 - once we agree on the digest part, I can add the image validation in this or another PR

This features requires the builder used to build the image to export
an oci standard layout file index.json
https://github.com/opencontainers/image-spec/blob/master/image-layout.md
If the index.json file is not present, the digest will not be
exposed in the taskRun
add a field in the image resource to indicate the location of the
index.json file -> indexpath
add a field in the taskRun status to include the image name and
digest -> resourcesResult
add a container step to collect the digest from all the files
the taskRun controller will collect the output from the
image-digest-exporter container and update the taskRun status

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

If an taskRun produces a container image with `index.json` file with the oci standard layout format, the digest of this built image will be exported in the taskRun status under resourcesResult

nader-ziada · 2019-04-04T15:33:38Z

/test pull-tekton-pipeline-integration-tests

nader-ziada · 2019-04-04T16:17:39Z

@bobcatfish the YAML tests are failing because getting latest from github.com/GoogleContainerTools/skaffold and running the unit tests fails, seems a problem upstream. This happens in the pipeline: https://github.com/tektoncd/pipeline/blob/master/examples/pipelineruns/pipelinerun.yaml

nader-ziada · 2019-04-05T13:24:48Z

/test pull-tekton-pipeline-integration-tests

nader-ziada · 2019-04-09T14:51:10Z

/test pull-tekton-pipeline-integration-tests

imjasonh · 2019-04-09T15:32:17Z

So after each step we'll inject a container that checks for contents in indexpath, and if it sees that, it will write it to its logs, where the controller will read from and parse accordingly. Is that correct?

A requested step like this:

- image: gcr.io/kaniko-project/executor
  args: ['--destination', 'gcr.io/foo/bar']

will end up as a pair of containers:

- image: gcr.io/kaniko-project/executor
  args: ['--destination', 'gcr.io/foo/bar']
- image: our-exporter
  args: ['--images=[...json...]'] # places to look for indexes

This requires the task author to know where the builder will write its index.json, and requires the controller to request 2 containers for every container the user requested. It would require the controller to link those two pods with all the volume mounts specifed in the initial container too, right?

I wonder if we can (ab)use termination message paths to specify a path in original container where it should write output data, then this will be directly available via the pod's status, without having to inject more containers or parse container logs.

This frees us from having to inject containers after each step, and from having to scrape logs, which might become brittle. It also means the controller tells the builder where it expects to read logs, rather than requiring the task author to know where the builder typically writes logs.

So a step defined as:

- image: gcr.io/kaniko-project/executor
  args: ['--destination', 'gcr.io/foo/bar']

will end up as a pod container like:

- image: gcr.io/kaniko-project/executor
  args: ['--destination', 'gcr.io/foo/bar']
  terminationMessagePath: '/tmp/r4nd0ms7r1ng'
  env:
  - name: BUILDER_OUTPUT
    value: /tmp/r4nd0ms7r1ng

If the builder sees that $BUILDER_OUTPUT is set, it will write to that path, and it will be bubbled up to the controller watching the pod via automatically-collected termination message. That path doesn't have to persist past the context of that single container.

One downside: docs for termination messages says:

The log output is limited to 2048 bytes or 80 lines, whichever is smaller.

Though elsewhere it says:

Will be truncated by the node if greater than 4096 bytes. The total message length across all containers will be limited to 12kb. Defaults to /dev/termination-log.

It might be possible to increase this limitation, or we might need to compress output or strip out unnecessary fields, or else risk only collecting partial data.

Am I understanding your approach correctly? Let me know if you'd prefer to chat over video, either at the next WG or ad-hoc. Sorry it's taken me so long to review this.

nader-ziada · 2019-04-09T15:48:15Z

@imjasonh
There will be only one container added at the end of the task to collect all the info from all the steps and add it to log output, the controller will loop on all of files and add them to the the taskRun output.

I added a field for the indexpath location in the resource, so users can specify where the builder they are using will output this file.

I thought about using the termination messages but was worried it would be considered the wrong usage.

bobcatfish

Nice work, I'm very excited about this functionality @nader-ziada !! 🎉 ❤️ 🎉

I left some miscellaneous feedback but it sounds like you and @imjasonh have a solid design in general!

My only question, and if this is something you're planning to address in a future iteration which is totally cool, but it's not clear to me how the pipelinerun controller is going to take the digest of the built image and make it available for downstream tasks (which can be using it in their templating e.g. "${inputs.resources.image.url}@${inputs.resources.image.digest}") ?

docs/resources.md

cmd/imagedigestexporter/main.go

bobcatfish · 2019-04-16T21:52:26Z

cmd/imagedigestexporter/main.go

+		ii, err := layout.ImageIndexFromPath(imageResource.IndexPath)
+		if err != nil {
+			// if this image doesn't have a builder that supports index.josn file,
+			// then it will be skipped


maybe log something in this case?

bumping this! :)

I don't want to log something here because the controller will be trying to parse the output of that container, also because not all builder will support this, I want it to be optional without errors.

hm okay :( too bad it looks like you couldn't have the controller only grab only stdout :(

Let me give that a try :)

@bobcatfish as far as I understand you have to attach to the pod to get the Stdout only, which would not work if the pod is completed. I think the get logs would be a safer bet in that case, wdyt?

I understand the issue with using log here, however silently skipping the image if the builder is not supported will make things easier to troubleshoot. You may also have the case of a supported image builder which is malfunctioning in a specific case and not producing the index.json.

Would it be an option to use the digest field itself to output a standard string e.g. "builder did not produce index.json" or so?

Maybe the controller could be aware of that error and not try to parse if it sees the error?

cmd/imagedigestexporter/main.go

pkg/apis/pipeline/v1alpha1/image_resource.go

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter_test.go

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

pkg/reconciler/v1alpha1/taskrun/taskrun.go

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml

nader-ziada · 2019-04-25T21:40:46Z

/hold

pushed some changes but still work in progress

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml - refactor image exporter code and add more unit tests

nader-ziada · 2019-04-29T17:32:06Z

/hold cancel

nader-ziada · 2019-04-29T17:33:39Z

@bobcatfish @imjasonh ready for another look,

I made the following changes:

Run the image digest exporter after each step
Move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed
Rename indexpath to outputimagepath
Add example yaml
Refactor image exporter code and add more unit tests
Add more documentation

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml - refactor image exporter code and add more unit tests Signed-off-by: Nader Ziada <nziada@pivotal.io>

bobcatfish

I left a bunch of miscellaneous comments, generally looking great tho and the approach is solid imo!

/approve
/meow space

docs/resources.md

examples/taskruns/task-output-image.yaml

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter.go

pkg/reconciler/v1alpha1/taskrun/taskrun.go

cmd/imagedigestexporter/main.go

tekton-robot · 2019-05-08T01:21:58Z

@bobcatfish:

In response to this:

I left a bunch of miscellaneous comments, generally looking great tho and the approach is solid imo!

/approve
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml - refactor image exporter code and add more unit tests Signed-off-by: Nader Ziada <nziada@pivotal.io>

bobcatfish

Bit more feedback! Interface looks great, just wondering about /tools/output-image.

I looked at the tests in a bit more detail, wondering if we can break up the AddOutputImageDigestExporter function a bit.

bobcatfish · 2019-05-09T19:21:49Z

cmd/imagedigestexporter/main.go

+		ii, err := layout.ImageIndexFromPath(imageResource.IndexPath)
+		if err != nil {
+			// if this image doesn't have a builder that supports index.josn file,
+			// then it will be skipped


bumping this! :)

docs/resources.md

pkg/apis/pipeline/v1alpha1/image_resource.go

pkg/apis/pipeline/v1alpha1/resource_types.go

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter_test.go

bobcatfish · 2019-05-09T19:34:33Z

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter_test.go

+				Args:    []string{"-images", fmt.Sprintf("[{\"name\":\"source-image-1\",\"type\":\"image\",\"url\":\"gcr.io/some-image-1\",\"digest\":\"\",\"OutputImageDir\":\"%s\"}]", currentDir)},
+			}},
+	}, {
+		desc: "image resource in task with multiple steps",


i'm trying to understand the difference between the two cases in this test and it seems like:

both have a resource that is used as an input AND and output

one has 1 step, the other has 2

I think there might be some more cases we'd want to test, like:

output resource only

when the OutputImageDir isn't set

This got me wondering if there is a way we could make the AddOutputImageDigestExporter function more focused so it would be a bit easier to tell what needed to be tested, and the test could be shorter: what if the function was divided up like this:

A function that can return all (if any) output image resources from a Task

A function that sets the OutputImageDir as needed

A function that adds the imageDigestExporterContainer

?

(I don't mind if you want to do that in a follow-up PR instead!)

when the outputImageDir is not set has a test in the task defaults since it get set there and output resource only don't would make any difference

for the refactor, makes sense, but I'd prefer if we do it in a follow up PR

for the refactor, makes sense, but I'd prefer if we do it in a follow up PR

kk!

docs/resources.md

pkg/reconciler/v1alpha1/taskrun/taskrun.go

bobcatfish · 2019-05-13T21:35:01Z

Left a few comments! Not sure about using /tools, maybe /builder/<something> makes more sense.

Also would like to see AddOutputImageDigestExporter refactored but I don't mind if that's a subsequent PR... as long as we don't forget haha :D

nader-ziada · 2019-05-13T21:39:24Z

@bobcatfish i’ll work on these last few comments this week. Thanks :)

vdemeester

Looks good 😻
Few nits and one questions in addition to @bobcatfish comments

cmd/imagedigestexporter/main.go

docs/resources.md

vdemeester · 2019-05-14T17:15:43Z

examples/taskruns/task-output-image.yaml

+          {
+            "mediaType": "application/vnd.oci.image.index.v1+json",
+            "size": 314,
+            "digest": "sha256:05f95b26ed10668b7183c1e2da98610e91372fa9f510046d4ce5812addad86b5"


I think buildah does, or we could use skopeo to fetch an image and write it in oci format

skopeo copy docker://busybox:latest oci:busybox_ocilayout:latest

But it can be done in a follow-up too 😉

pkg/apis/pipeline/v1alpha1/image_resource.go

vdemeester · 2019-05-14T17:19:39Z

pkg/apis/pipeline/v1alpha1/resource_types.go

-	// Resource Value stuff
+	// +optional
+	// Path to the index.json file for output container images
+	OutputImageDir string `json:"outputImageDir"`


Any reason to not re-use TargetPath for the output folder ? (might have been already discussed 🙇‍♂️)

don't know, didn't think about it 🤷🏼‍♂️

Asking that as this field is only gonna be useful for one type of resources and I feel it would make sense to re-use TargetPath.
@bobcatfish @abayer wdyt ?

I think it might bit a little confusing because it will have different meanings

I think it might bit a little confusing because it will have different meanings

Yeah I am 50/50 on that. It's close (as it is a "target" path), but user "may" think it's related to the image reference (although this is not where we define that one so…). I'm not sure how confusing it can be 😅 #dont-know-what-i-am-talking-about 😹

oooo yeah this is a good idea - I'm really tempted to re-use targetPath here too 🤔but also I'm torn because overloading it may be confusing. Since we're alpha (famous last words) I'm thinking go ahead with OutputImageDir for now and see how it goes?

pkg/reconciler/v1alpha1/taskrun/taskrun.go

tekton-robot · 2019-05-14T17:27:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, nader-ziada, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish,vdemeester]

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

- This features requires the builder used to build the image to export an oci standard layout file `index.json` https://github.com/opencontainers/image-spec/blob/master/image-layout.md - If the `index.json` file is not present, the digest will not be exposed in the taskRun - add a field in the image resource to indicate the location of the index.json file -> `indexpath` - add a field in the taskRun status to include the image name and digest -> `resourcesResult` - add a container step to collect the digest from all the files - the taskRun controller will collect the output from the image-digest-exporter container and update the taskRun status

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml - refactor image exporter code and add more unit tests Signed-off-by: Nader Ziada <nziada@pivotal.io>

- rename field to OutputImageDir instead of OutputImagePath - fixed documentation typos

- fix some docs and comments

afrittoli

Nice work, thank you! I look forward to having this feature merge in.
The only thing that concerns me a bit on this is using the container log to transport data, but that's definitely not a show-stopper, it may be something we can improve upon in future.

afrittoli · 2019-05-16T10:19:37Z

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter.go

+	return nil
+}
+
+// UpdateTaskRunStatusWithResourceResult if there an update to the outout image resource, add to taskrun status result


if there is an update to the output

bumping this - might merge anyway tho cuz it's been a long road and this PR is otherwise in pretty great shape - maybe @nader-ziada can fix in a follow up?

I can do a followup PR on that one

afrittoli · 2019-05-16T12:51:28Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+				if err != nil {
+					logger.Errorf("Error getting output from image-digest-exporter for %s/%s: %s", taskRun.Name, taskRun.Namespace, err)
+				}
+				err = resources.UpdateTaskRunStatusWithResourceResult(taskRun, logContent)


I've been trying to find an alternative to using the log file to share data, so that we could keep the log file to log things. The best idea I could come up with so far is to use the imagedigestreporter to update the TaskRun via patch and store the image digest there.
Did you consider any other alternative yet?

There was a doc sent out a while back: and this PR was based on the feedback I got.

i know what you mean @afrittoli ! it seems a bit odd but it also seems like the best option at the moment - but fortunately since this is an implementation detail, I feel like we're safe to go ahead with this and if we come across a better idea we can change it later :D

bobcatfish

Thanks for your patience @nader-ziada , looking great! 😻

A couple minor comments but I don't think there's anything that should stop us from merging - let's do it!

Thanks for adding this excellent new feature :D

/lgtm
/meow space

bobcatfish · 2019-05-21T13:17:22Z

cmd/imagedigestexporter/main.go

+		ii, err := layout.ImageIndexFromPath(imageResource.IndexPath)
+		if err != nil {
+			// if this image doesn't have a builder that supports index.josn file,
+			// then it will be skipped


Maybe the controller could be aware of that error and not try to parse if it sees the error?

bobcatfish · 2019-05-21T13:18:00Z

docs/developers/README.md

+The `/builder/` namespace is reserved on containers for various system tools, such as the following:
+
+ - The environment variable HOME is set to `/builder/home`, used by the builder tools and injected on into all of the step containers
+ - Default location for output-images `/builder/output-images` 


bobcatfish · 2019-05-21T13:20:11Z

pkg/apis/pipeline/v1alpha1/resource_types.go

-	// Resource Value stuff
+	// +optional
+	// Path to the index.json file for output container images
+	OutputImageDir string `json:"outputImageDir"`


oooo yeah this is a good idea - I'm really tempted to re-use targetPath here too 🤔but also I'm torn because overloading it may be confusing. Since we're alpha (famous last words) I'm thinking go ahead with OutputImageDir for now and see how it goes?

bobcatfish · 2019-05-21T13:23:19Z

pkg/reconciler/v1alpha1/taskrun/resources/image_exporter.go

+	return nil
+}
+
+// UpdateTaskRunStatusWithResourceResult if there an update to the outout image resource, add to taskrun status result


bumping this - might merge anyway tho cuz it's been a long road and this PR is otherwise in pretty great shape - maybe @nader-ziada can fix in a follow up?

pkg/reconciler/v1alpha1/taskrun/taskrun.go

tekton-robot · 2019-05-21T13:24:42Z

@bobcatfish:

In response to this:

Thanks for your patience @nader-ziada , looking great! 😻

A couple minor comments but I don't think there's anything that should stop us from merging - let's do it!

Thanks for adding this excellent new feature :D

/lgtm
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bobcatfish · 2019-05-21T13:46:07Z

Looks like there is some new linting @dlorenc added recently that has noticed a couple things so we can't merge just yet @nader-ziada :( 😭

I0521 13:30:49.876] pkg/reconciler/v1alpha1/taskrun/resources/image_exporter_test.go:285:41: Error return value of `UpdateTaskRunStatusWithResourceResult` is not checked (errcheck)
I0521 13:30:49.877] 			UpdateTaskRunStatusWithResourceResult(c.taskRun, c.podLog)
I0521 13:30:49.878] 			                                     ^
I0521 13:30:49.879] pkg/reconciler/v1alpha1/taskrun/taskrun_test.go:1742:57: Error return value of `(github.com/tektoncd/pipeline/vendor/k8s.io/client-go/tools/cache.Store).Add` is not checked (errcheck)
I0521 13:30:49.880] 				pipelineResourceInformer.Informer().GetIndexer().Add(r)
I0521 13:30:49.881] 				                                                    ^
I0521 13:31:11.248] pkg/apis/pipeline/v1alpha1/task_defaults.go:39:2: S1023: redundant `return` statement (gosimple)
I0521 13:31:11.296] 	return
I0521 13:31:11.296] 	^

nader-ziada · 2019-05-21T16:48:24Z

Thanks @bobcatfish, pushed a fix to the linting issues, happy to do a follow-up PR for other comments, there was already a refactor we talked about that I said I would do as a follow up :D

bobcatfish · 2019-05-21T17:55:51Z

Thanks @nader-ziada!! : heart:

/lgtm

- run the image digest exporter after each step - move the index path to the task definition instead of the resource since the user needs to know this at the time of creating the task so it can be used as part of the steps scripting if needed - rename indexpath to outputimagepath - add example yaml - refactor image exporter code and add more unit tests Signed-off-by: Nader Ziada <nziada@pivotal.io>

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 4, 2019

tekton-robot requested review from dlorenc and vdemeester April 4, 2019 14:23

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 4, 2019

nader-ziada force-pushed the imageoutput branch from a510bba to c73b1b2 Compare April 9, 2019 14:08

bobcatfish reviewed Apr 16, 2019

View reviewed changes

nader-ziada force-pushed the imageoutput branch from c73b1b2 to 80888fa Compare April 17, 2019 19:29

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

nader-ziada force-pushed the imageoutput branch from e3e3ee1 to e699154 Compare April 26, 2019 00:03

nader-ziada force-pushed the imageoutput branch from e699154 to 6232343 Compare April 29, 2019 17:31

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

nader-ziada force-pushed the imageoutput branch from 6232343 to 374637e Compare April 29, 2019 17:34

imjasonh mentioned this pull request May 3, 2019

Add buildah Task tektoncd/catalog#11

Merged

2 tasks

nader-ziada force-pushed the imageoutput branch from 374637e to 0e5def6 Compare May 6, 2019 19:10

bobcatfish reviewed May 8, 2019

View reviewed changes

bobcatfish reviewed May 9, 2019

View reviewed changes

khrisrichardson reviewed May 10, 2019

View reviewed changes

docs/resources.md Outdated Show resolved Hide resolved

khrisrichardson reviewed May 10, 2019

View reviewed changes

docs/resources.md Show resolved Hide resolved

bobcatfish reviewed May 13, 2019

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/taskrun.go Show resolved Hide resolved

vdemeester approved these changes May 14, 2019

View reviewed changes

nader-ziada force-pushed the imageoutput branch from 3497970 to c4ce799 Compare May 14, 2019 19:42

nader-ziada added 5 commits May 15, 2019 22:03

refactor and cleanup related to output image digest feature

3983658

- rename field to OutputImageDir instead of OutputImagePath - fixed documentation typos

improve returned errors in outoutimageexporter + other small fixes

9f34452

update the default location of output images

ba45ca6

- fix some docs and comments

nader-ziada force-pushed the imageoutput branch from c4ce799 to ba45ca6 Compare May 16, 2019 02:04

afrittoli reviewed May 16, 2019

View reviewed changes

bobcatfish reviewed May 21, 2019

View reviewed changes

tekton-robot assigned bobcatfish May 21, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2019

fix linting issues

32398ba

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2019

tekton-robot merged commit ea5cc07 into tektoncd:master May 21, 2019

chhsia0 mentioned this pull request Aug 8, 2019

Proposal: Dynamic Output Resources #1180

Closed

Add support for taskRun to expose digest of built images #721

Add support for taskRun to expose digest of built images #721

Conversation

nader-ziada commented Apr 4, 2019

Changes

Submitter Checklist

Release Notes

nader-ziada commented Apr 4, 2019

nader-ziada commented Apr 4, 2019

nader-ziada commented Apr 5, 2019

nader-ziada commented Apr 9, 2019

imjasonh commented Apr 9, 2019

nader-ziada commented Apr 9, 2019

bobcatfish 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

nader-ziada commented Apr 25, 2019

nader-ziada commented Apr 29, 2019

nader-ziada commented Apr 29, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

tekton-robot commented May 8, 2019

bobcatfish 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

bobcatfish commented May 13, 2019

nader-ziada commented May 13, 2019

vdemeester 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

tekton-robot commented May 14, 2019

afrittoli 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

bobcatfish 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

tekton-robot commented May 21, 2019

bobcatfish commented May 21, 2019

nader-ziada commented May 21, 2019

bobcatfish commented May 21, 2019