Rename to "applications" and "services" #348

sbose78 · 2020-02-23T15:05:05Z

( .. Based on @isutton 's ideas )

~~### This is a proposal and open for discussion/debate.~~

Motivation

Simplify the API, " connect applications to services "
Ensure 100% backward compatibility, no breaking changes in this PR

Changes

Rename spec.applicationSelector to spec.applications
Rename spec.backingServiceSelectors to spec.services
Support a list of apps : spec.applications
In order to maintain backward compatibility, spec.applications, spec.applicationSelector , spec.backingServiceSelector and spec.backingServiceSelectors are all optional. The appropriate parameters have been marked as deprecated.
In a future PR, when spec.applicationSelector,spec.backingServiceSelector and spec.backingServiceSelectors are removed, we will mark spec.applications and spec.services as mandatory. Doing so now will break integrations.

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest

metadata:
  name: node-golang-todo-git

spec:

  applications:
  - resourceRef: node-todo-git
    group: apps
    version: v1
    resource: deployments

  services:
  - group: postgresql.baiju.dev
    version: v1alpha1
    kind: Database
    resourceRef: db-demo
  - group: etcd.database.coreos.com
    version: v1beta2
    kind: EtcdCluster
    resourceRef: etcd-demo

Here's how the API looked before:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: node-todo-git
spec:
  applicationSelector:
    resourceRef: node-todo-git
    group: apps
    version: v1
    resource: deployments
  backingServiceSelectors:
  - group: postgresql.baiju.dev
    version: v1alpha1
    kind: Database
    resourceRef: db-demo
  - group: etcd.database.coreos.com
    version: v1beta2
    kind: EtcdCluster
    resourceRef: etcd-demo

Care is being taken to maintain backward compatibility.

openshift-ci-robot · 2020-02-23T15:05:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sbose78
You can assign the PR to them by writing /assign @sbose78 in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov · 2020-02-23T16:03:58Z

Codecov Report

Merging #348 into master will increase coverage by 0.17%.
The diff coverage is 87.80%.

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   64.95%   65.12%   +0.17%     
==========================================
  Files          21       21              
  Lines        1418     1431      +13     
==========================================
+ Hits          921      932      +11     
- Misses        372      373       +1     
- Partials      125      126       +1

Impacted Files	Coverage Δ
...troller/servicebindingrequest/custom_env_parser.go	`71.42% <66.66%> (ø)`
pkg/controller/servicebindingrequest/binder.go	`61.07% <90.90%> (+0.79%)`	⬆️
pkg/controller/servicebindingrequest/planner.go	`93.93% <100.00%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8ca153...4a479d1. Read the comment docs.

otaviof · 2020-02-24T16:32:53Z

Overall, I like the proposal, the ServiceBinding will look a lot cleaner this way! Thanks.

Removed support for label selectors in spec.applications

However, I think we should keep label selectors, since they are a general approach to handle Kubernetes based workloads. Therefore, users could choose on using a fully-qualified resource name or labels, should be up to final users.

sbose78 · 2020-02-26T05:12:36Z

@isutton , thoughts on removing label selectors?

isutton · 2020-02-26T12:20:56Z

I've recently discussed about the labels topic with @otaviof and I'm fine supporting both direct references and label selectors for applications. The only question remaining is how to properly encode this information in the manifest.

One option might be keeping the applicationSelector field, but changing its internal fields to only represent or contain a Kubernetes' selector type, and introduce the applications field next to it containing a list of explicit application references.

This implies that watches should, if not already, be created to observe creation and updates of all supported applications' manifests in order to evaluate whether a binding should be applied to it or not.

sbose78 · 2020-02-27T19:14:02Z

We could have the following then:

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest

metadata:
  name: node-golang-todo-git

spec:

  applications:
  - group: apps
    version: v1
    resource: deployments
    labels: 
        environment: production
        app: nginx

And

apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest

metadata:
  name: node-golang-todo-git

spec:

  applications:
  - group: apps
    version: v1
    resource: deployments
    resourceRef: foo

sbose78 · 2020-02-27T21:27:46Z

cc @arthurdm @bbrowning @pdettori
Would love to hear your feedback/thoughts too.

arthurdm · 2020-02-27T21:57:19Z

hey @sbose78 - I like the new look of the CRD, cleaner and easier to understand! Assuming that #296 would also apply to applications, all good!

Side note (not meant to de-rail this PR, but since it also relates to updating the CRD)

We're looking to adopt (from the service binding spec) the ServiceBindingRequest CRD as the artifact by which applications use to request bindings. We have identified that we would likely need 2 new keys (per service):

one defining a service account to be used by the Service Binding Operator (SBO) to read the Secret exposed by the bindable service (secured with RBAC)
one defining the name of a Secret that contained complimentary binding data that SBO would combine with the data available from the bindable service. This is the case where an external subscription entity is used to fetch per-user API keys, or credentials/URI for a provisioned Db, and those are made available in a Secret. So SBO can combine it with the ones it finds, creating a single Secret (to optionally inject back into the app).

Probably this will make more sense once I write it down in a new PR to the spec, just fyi since you guys are thinking about changing the CRD with this PR. =)

The other thing is if apiVersion: apps.openshift.io/v1alpha1 could be made more generic so it doesn't necessarily tie into OpenShift? Some thoughts.

pdettori · 2020-02-27T22:09:55Z

@sbose78 great, this looks definitely cleaner. Perhaps for another day/discussion, but have you thought about using a name for the CRD kind that looks more like a noun than an action ? That is, ServiceBinding rather than ServiceBindingRequest ? I think that would be more consistent with the way most CRDs are named in Kubernetes and with desired state based management. A 'Request' seems more like a one-off thing, which I would not expect to keep maintaining the state of the binding, while I suppose that here the Service Binding Operator would reconcile a change in the backing service with the credentials injected in the app.

bbrowning · 2020-02-28T13:00:11Z

+1 to applications and services as more concise names for the things being connected

sbose78 · 2020-02-28T14:11:02Z

Hi, @arthurdm @pdettori , Thank you!

apiVersion: apps.openshift.io/v1alpha1

Yes, the group name must be changed. openshift must go from there. We'll track this and mark it for an openshift 4.5 timeframe because openshift consumes this API today.

but have you thought about using a name for the CRD kind that looks more like a noun than an action ? That is, ServiceBinding rather than ServiceBindingRequest ?

Yes! We did consider this and there seems to be significant consensus to start calling it servicebinding. I'll come up with a swift plan.

( field for a service to define a service account to be used by the Service Binding Operator (SBO) to read the Secret exposed by the bindable service (secured with RBAC)

This should be optional to handle a situation where the 'default' service account used by the service binding operator doesn't have the privileges to read a specific secret? I wonder if that situation would arise because the service binding operator's service account can read/write secrets in all namespaces.

one defining the name of a Secret that contained complimentary binding data that SBO would combine with the data available from the bindable service.

In this case, are you proposing we skip creating a separate binding secret and instead manage the one provided by the service, with additional information in it?

arthurdm · 2020-02-28T14:27:13Z

Hey @sbose78 - thanks for the feedback. It's great to see these things are in motion.

This should be optional to handle a situation where the 'default' service account used by the service binding operator doesn't have the privileges to read a specific secret?

That's right.

I wonder if that situation would arise because the service binding operator's service account can read/write secrets in all namespaces.

Interesting. Is that something that could be configurable? Just thinking if customers would want to have a finer grained RBAC for this, so that everything doesn't become bindable by anyone?

In this case, are you proposing we skip creating a separate binding secret and instead manage the one provided by the service, with additional information in it?

Ya - probably populating the given Secret with anything missing / additional would be easiest. Another approach is to just copy the information from the given Secret into the one SBO creates/manages/injects.

sbose78 · 2020-02-28T21:28:27Z

Interesting. Is that something that could be configurable? Just thinking if customers would want to have a finer grained RBAC for this, so that everything doesn't become bindable by anyone?

Custom service accounts would be hard since this operator needs to be watch all namespaces.

I propose a validation webhook & subject-access-review(SAR)-based design to ensure we validate "Does John have the privileges to modify deployment foo?"

What do you think?

arthurdm · 2020-02-28T22:43:51Z

ya - perhaps there's 2 sides to this:
(a) does the requester have authority to create a binding to this bindable resource?
(b) (if application injection is not toggled off) does the requester have authority to modify the deployment with the binding data?

From a spec perspective It would be nice to have a single "thing" that we pass into the ServiceBindingRequest where implementations (in this case, SBO) could check for both (a) and (b). I was thinking a service-account was a native way to achieve it, but you bring up a good point that the SBO would already have permissions for all of them. In a validation / SAR design, what would be the artifact we pass around? (let me know if we should move this into a new issue 😄 )

sbose78 · 2020-03-02T13:26:24Z

let me know if we should move this into a new issue

yes yes :)

sbose78 · 2020-03-02T14:43:50Z

If both labels and resourceRef are present in applications[i], should we compile a cumulative list of

objects by resourceRef
objects by label selectors ?

@DhritiShikhar @isutton

isutton · 2020-03-20T11:09:24Z

The reason I prefer using pointers is that it helps our code distinguish between the following:

[snip]

BackingServiceSelectors is used:

when [], the field might have been skipped in the CR. OR the user might have specified an empty list in the CR <-- this is an ambiguity.

I agree that some situations might require this approach (mostly when dealing with scalars and default values), but in this operator's particular case I'm unsure whether those precautions are required, since there are no semantic differences between an unpopulated or nil slice; additionally if any clean-up or amendment if necessary should be based on the differences between the previous state and the new desired state.

sbose78 · 2020-03-20T21:48:17Z

I had actually moved out the change to a different PR because of a different observation as well #369 . This PR no longer introduces that change.

Avni-Sharma · 2020-03-23T08:14:09Z

pkg/apis/apps/v1alpha1/servicebindingrequest_types.go

-	// different subresources owned by backing operator CR.
-	// +optional
-	DetectBindingResources bool `json:"detectBindingResources"`
+	ApplicationSelector *ApplicationSelector `json:"applicationSelector"`


this should be omitempty

Good catch, fixed :)

Avni-Sharma · 2020-03-31T07:41:44Z

/retest

sbose78 · 2020-04-01T01:27:55Z

/retest

sbose78 · 2020-04-02T04:14:15Z

/retest

sbose78 · 2020-04-02T04:15:14Z

/test 4.4-e2e

ron1 · 2020-04-02T14:16:14Z

Up until now, it has been my understanding that a single ServiceBindingRequest CR was intended to bind one or more applications to a single backing service. The motivation for backingServiceSelectors being plural was to support complex backing services that had needed binding properties spread around in multiple resources. For example, a backing Kafka service has needed binding properties stored in both its KafkaCluster CR and its KafkaUser CR.

Is the name change from backingServiceSelectors to services intended to change the scope of the SBR CR such that now it binds one or more applications to multiple backing services rather than a single backing service? If so, then I think it would be good to have a discussion about the pros and cons of such a scope change. To me, this is much bigger than a simple name change.

OTOH, if this name change does not change the scope and a SBR CR still represents binding to a single backing service with potentially multiple selectors, then maybe the name change should be from backingServiceSelectors to the more simple serviceSelectors. With this name, it is more clear that a single service is the binding target resource and that multiple selectors may be required in order to capture all the binding information.

WDYT?

isutton · 2020-04-02T14:57:01Z

Up until now, it has been my understanding that a single ServiceBindingRequest CR was intended to bind one or more applications to a *single* backing service. The motivation for backingServiceSelectors being plural was to support complex backing services that had needed binding properties spread around in multiple resources.

This behavior has already been changed in #322, by accepting more than one service selector or direct reference to a service. The rationale behind it is that typically applications depend on multiple services (databases, cache services, etc) and the configuration values exposed by those services are, again typically, required for the well-functioning of said applications. This simplifies the operator's work, since it has less related resources to manage (secrets or config maps).

Is the name change from backingServiceSelectors to services intended to change the scope of the SBR CR such that now it binds one or more applications to *multiple* backing services rather than a single backing service? If so, then I think it would be good to have a discussion about the pros and cons of such a scope change. To me, this is much bigger than a simple name change.

The behavior you've mentioned hasn't changed; one can still use one service entry with a service selector. It was added the ability to have more of those, although I agree that a change such as removing the service selector functionality should be discussed, although I'm fine with removing it since I would like to control which dependencies are being included in my application, which I don't have in the case of supporting service selectors.

OTOH, if this name change does not change the scope and a SBR CR still represents binding to a *single* backing service with potentially multiple selectors, then maybe the name change should be from backingServiceSelectors to the more simple serviceSelectors. With this name, it is more clear that a single service is the binding target resource and that multiple selectors may be required in order to capture all the binding information.

We had some internal discussions regarding this topic, and I personally consider it fine to have selectors for applications but not so much for services. I would love to hear the use cases you have in mind for service selectors, it'd be really helpful.

ron1 · 2020-04-02T18:03:43Z

@isutton Thanks for your reply.

In complex binding scenarios as is the case for Kafka with mutual TLS, the use of two independent service "entries" for the same Kafka service seems a bit unintuitive. In the binding secret at least, shouldn't the fact that I needed two service "entries" in order to bind to this one complex Kafka service be hidden? Is the thought that each service "entry" will have its own optional envvarprefix or id or mountpath and the two independent Kafka service "entries" will be associated with one another by assigning them the same envvarprefix/id/mountpath?

isutton · 2020-04-02T21:05:46Z

In complex binding scenarios as is the case for Kafka with mutual TLS <https://github.com/strimzi/strimzi-kafka-operator/blob/790793f351b508118a6681a5dbe742aabc4955e6/design/service-binding.md#consuming-clients-servicebindingrequest-4>, the use of two independent service "entries" for the same Kafka service seems unintuitive.

I don't understand this. The link you've provided seems fine, and according to what we've been designing. Could you be more specific to what exactly (with examples if possible) you find unintuitive?

In the binding secret at least, shouldn't the fact that I needed two service "entries" in order to bind to this one complex Kafka service be hidden? Is the thought that each service "entry" will have its own optional envvarprefix or id or mountpath and the two independent Kafka service "entries" will be associated with one another by assigning them the same envvarprefix/id/mountpath?

Having multiple services means that we should be careful to not glob settings from each similar service, so we've been discussing the idea of aggregate the configuration values collected from the services, and grouping those accordingly. This grouping would use the same structure k8s itself uses: {apiVersion}.{kind}.{name}. Using the resources from the example referenced before, the values v1beta1.Kafka.my-cluster and v1beta1.KafkaUser.my-user would be available to be used in a CustomEnvVar template.

arthurdm · 2020-04-03T00:05:41Z

we've been discussing the idea of
aggregate the configuration values collected from the services, and
grouping those accordingly.

@isutton - in this case, would you not just simply add the service's ID, as per this section? This section of the spec will apply also to injected values once this issue is written down, so it's consistent with mounted values. Would make sense to store these values with their IDs in the Secret.

ron1 · 2020-04-03T00:09:46Z

My point is simply that Kafka and KafkaUser should.not be treated as separate services. Instead their properties should be globbed together in the binding secret. How does the SBR know to glob these two services together?

@arthurdm Would these two services share a common ID which would allow their properties to be globbed together in the binding secret.

isutton · 2020-04-03T09:11:13Z

@isutton <https://github.com/isutton> - in this case, would you not just simply add the service's ID, as per this section <https://github.com/application-stacks/service-binding-specification#id-prefix>? This section of the spec will apply also to injected values once this issue <servicebinding/spec#15> is written down, so it's consistent with mounted values. Would make sense to store these values with their IDs in the secret themselves.

Each k8s resource already has an identifier, even if verbose: apiGroup, kind and name. The "id" field is, from my understanding, a shortcut for that tuple, and it might be error prone due to lack of type safety in both the value assignment and template definition. Adding this feature to be able to expose an environment variable DBUSER_HOST instead of DATABASE_USER_HOST, or a mount point dbuser instead of database/user seems overkill to me in this particular, but I would love to hear more about your thoughts on this and perhaps other use cases.

isutton · 2020-04-03T12:45:58Z

My point is simply that Kafka and KafkaUser should.not be treated as separate services. Instead their properties should be globbed together in the binding secret. How does the SBR know to glob these two services together?

To be more clear, following up the previous Kafka and KafkaUser examples, all the exposed configuration values specified in their CRDs will become available to all applications bound to those services in order to be used in Go template expressions to compose the intermediate resources that are then referenced in application resources. For example, in Go template those services will be accessible through the path .Kafka.my-cluster.status.listeners, or .KafkaUser.my-user.status.username, and KAFKA_MY_CLUSTER_STATUS_LISTENERS_0_ADDRESSES_0_HOST, KAFKA_MY_CLUSTER_STATUS_LISTENERS_0_ADDRESSES_0_PORT and so on, and KAFKAUSER_MY_USER_STATUS_USERNAME. The binding secret contains the configuration values that have been exposed by all the declared services that currently exist in the cluster, so I tend to think we're on the same page and this direction meets your needs. Please let me know if I missed something.

ron1 · 2020-04-03T13:00:10Z

I believe an intended goal is to provide a canonical Service Binding Schema for backing services. In the above example, Kafka is a single service but requires two service "entries" to accomplish its binding. Some of the binding schema elements are extracted from the Kafka CR and others are extracted from the KafkaUser CR. From a binding schema perspective, all binding properties for a single type=kafka should appear within a single "binding root" regardless of the CR from which they were extracted.

In other words, I do not want a Kafka "binding root" in the resulting binding secret that has the following subset of binding schema elements:

type - kafka
endpoints.<listener_name> - bootstrap server information for all listeners (TBD)
ca.p12 - CA certificate PKCS #12 archive file for storing certificates and keys (binding:env:object:secret:ca.p12)
ca.password - password for protecting the CA certficate PKCS #12 archive file (binding:env:object:secret:ca.password)

and another KafkaUser "binding root" that has the remaining subset of binding schema elements:

username - username for the consuming client (binding:env:object:secret:username)
user.p12 - user certificate for the consuming client PKCS #12 archive file for storing certificates and keys (binding:env:object:secret:user.p12)
user.password - password for protecting the user certficate PKCS #12 archive file (binding:env:object:secret:user.password)

Again, the resulting binding secret should have a single binding type=kafka that contains all the above properties. No binding type=kafkauser should be introduced.

If the service "entries" for Kafka and KafkaUser shared a common id, for example, that id could potentially serve as the "binding root" in the binding secret within which all the properties are located.

Avni-Sharma · 2020-04-06T13:48:55Z

deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml

-                service operator. Deprecation Notice: In the upcoming release, this
-                field would be depcreated. It would be mandatory to set "backingServiceSelectors".'
+              description: BackingServiceSelector is used to identify the backing
+                service operator. Deprecated. In the upcoming release, this field


backing service operator or any other k8s resource

Avni-Sharma · 2020-04-06T13:49:18Z

deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml

@@ -99,10 +99,78 @@ spec:
              - resource
              - version
              type: object
+            applications:
+              description: Applications are used to identify the application connecting


multiple applications

This is auto-generated from

// Applications are used to identify the application connecting to the // backing service. // +optional Applications *[]ApplicationSelector `json:"applications,omitempty"`

Avni-Sharma · 2020-04-06T13:54:55Z

deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml

@@ -251,8 +319,34 @@ spec:
              description: EnvVarPrefix is the prefix for environment variables
              type: string
            mountPathPrefix:
-              description: MountPathPrefix is the prefix for volume mount
+              description: "Important: Run \"operator-sdk generate k8s\" to regenerate


description here should not change, the previous one looks good MountPathPrefix is the prefix for volume mount

Avni-Sharma · 2020-04-06T13:56:59Z

pkg/apis/apps/v1alpha1/servicebindingrequest_types.go

+	// DetectBindingResources is flag used to bind all non-bindable variables from
+	// different subresources owned by backing operator CR.
+	// +optional
+	DetectBindingResources bool `json:"detectBindingResources"`


omitempty should be included for +optional fiellds

Do you mind if I fixed it in a different PR, since the change isn't related?

sbose78 · 2020-04-07T03:49:04Z

/retest

Avni-Sharma · 2020-04-13T09:35:50Z

pkg/controller/servicebindingrequest/planner.go

@@ -85,6 +85,9 @@ func (p *Planner) Plan() (*Plan, error) {
 	if p.sbr.Spec.BackingServiceSelectors != nil {
 		selectors = append(selectors, *p.sbr.Spec.BackingServiceSelectors...)
 	}
+	if p.sbr.Spec.Services != nil {


A similar condition for p.sbr.Spec.Applications is missing that should be checked in a similar was as for https://github.com/redhat-developer/service-binding-operator/blob/master/pkg/controller/servicebindingrequest/planner.go#L83

https://github.com/redhat-developer/service-binding-operator/pull/414/files#r407707455
On this, could we please have a chat here ^ @Avni-Sharma @DhritiShikhar

sbose78 · 2020-04-17T15:09:33Z

/hold

sbose78 · 2020-05-27T14:07:52Z

Replaced by #495

modify API

b2ee90f

sbose78 requested review from isutton, baijum, DhritiShikhar and Avni-Sharma February 23, 2020 15:05

openshift-ci-robot requested a review from akashshinde February 23, 2020 15:05

openshift-ci-robot added the size/L label Feb 23, 2020

sbose78 added 2 commits February 23, 2020 12:45

use applications[i].Group

a33dde8

add binder test

697b7ed

openshift-ci-robot added size/XL and removed size/L labels Feb 24, 2020

arthurdm mentioned this pull request Mar 3, 2020

CRD updates for advanced binding scenarios #355

Closed

bring back label selectors

b9ff654

openshift-ci-robot added the needs-rebase label Mar 5, 2020

Avni-Sharma reviewed Mar 23, 2020

View reviewed changes

resolve conflict

e71276d

isutton mentioned this pull request Apr 3, 2020

Define service grouping #411

Closed

Avni-Sharma reviewed Apr 6, 2020

View reviewed changes

sbose78 added 2 commits April 6, 2020 23:15

resolve conflict

28cb90e

update backingServiceSelector docs

4a479d1

Avni-Sharma reviewed Apr 13, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold label Apr 17, 2020

baijum added the breaking-api label May 13, 2020

sbose78 closed this May 27, 2020

Rename to "applications" and "services" #348

Rename to "applications" and "services" #348

Uh oh!

Conversation

sbose78 commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Uh oh!

openshift-ci-robot commented Feb 23, 2020

Uh oh!

codecov bot commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

otaviof commented Feb 24, 2020

Uh oh!

sbose78 commented Feb 26, 2020

Uh oh!

isutton commented Feb 26, 2020

Uh oh!

sbose78 commented Feb 27, 2020

Uh oh!

sbose78 commented Feb 27, 2020

Uh oh!

arthurdm commented Feb 27, 2020

Side note (not meant to de-rail this PR, but since it also relates to updating the CRD)

Uh oh!

pdettori commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbrowning commented Feb 28, 2020

Uh oh!

sbose78 commented Feb 28, 2020

Uh oh!

arthurdm commented Feb 28, 2020

Uh oh!

sbose78 commented Feb 28, 2020

Uh oh!

arthurdm commented Feb 28, 2020

Uh oh!

sbose78 commented Mar 2, 2020

Uh oh!

sbose78 commented Mar 2, 2020

Uh oh!

isutton commented Mar 20, 2020

Uh oh!

sbose78 commented Mar 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Avni-Sharma commented Mar 31, 2020

Uh oh!

sbose78 commented Apr 1, 2020

Uh oh!

sbose78 commented Apr 2, 2020

Uh oh!

sbose78 commented Apr 2, 2020

Uh oh!

ron1 commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isutton commented Apr 2, 2020 via email

Uh oh!

ron1 commented Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isutton commented Apr 2, 2020 via email

Uh oh!

arthurdm commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ron1 commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isutton commented Apr 3, 2020 via email

Uh oh!

isutton commented Apr 3, 2020 via email

sbose78 commented Feb 23, 2020 •

edited

Loading

codecov bot commented Feb 23, 2020 •

edited

Loading

pdettori commented Feb 27, 2020 •

edited

Loading

ron1 commented Apr 2, 2020 •

edited

Loading

ron1 commented Apr 2, 2020 •

edited

Loading

arthurdm commented Apr 3, 2020 •

edited

Loading

ron1 commented Apr 3, 2020 •

edited

Loading

ron1 commented Apr 3, 2020 •

edited

Loading