Make clarifications and add detail to API document #40

pmorie · 2016-11-19T23:30:40Z

Builds upon #31 to clarify and further elaborate on messages flow and API server / controller interactions.

arschles · 2016-11-21T23:22:06Z

docs/v1/api.md

+it has received a `Broker` resource and successfully called the backing CF
+broker's catalog endpoint.
+
+*TODO: Make explicit the relationship between service classes and plans*


does this mean that plans will be separate resources (e.g. ServicePlan)?

Not necessarily. It only means that we did not have an agreement on it and need to discuss as a group. I have my own opinions but we need to agree as a group.

I've created #45 to establish consensus on this. Are you cool with this remaining a TODO for now, @arschles ?

@pmorie yes, I'm ok that this remains a TODO

arschles · 2016-11-21T23:26:48Z

docs/v1/api.md

+    endpoint for the application to use the `Instance` via
+2.  The name of a Kubernetes `Secret` resource to hold credentials necessary to
+    use the service
+3.  The name and spec of a Kubernetes `ServiceInjectionPolicy` to create


There's no section for ServiceInjectionPolicy in this doc. Although we've talked about it, let's leave this point out or create a section for it here.

Since this resource and its admission controller must both exist, and neither are yet available, I'd prefer that we do the former.

@arschles Yeah, the ServiceInjectionPolicy is going to be worked out in the main repo. I haven't made the proposal yet -- I will tag you in when I do.

I had remembered this being a point of agreement in the F2F, but I am happy to remove it for now and add it in a follow-up PR.

pmorie · 2016-11-22T02:48:27Z

@arschles I have removed the ServiceInjectionPolicy bits from this for now, PTAL.

arschles · 2016-11-22T17:38:39Z

docs/v1/api.md


-## `Instance`
+*TODO: codify how asynchronous responses are handled in the controller*


arschles · 2016-11-22T17:40:11Z

LGTM

vaikas · 2016-11-30T18:27:38Z

docs/v1/api.md


-1. Make a request against a given CF service broker's catalog endpoint
+An administrator to creates an instance of the `Broker` resource to indicate


s/to creates/creates/

vaikas · 2016-11-30T18:29:31Z

docs/v1/api.md


-1. Make a request against a given CF service broker's catalog endpoint
+An administrator to creates an instance of the `Broker` resource to indicate
+their intent to consume that broker in the service catalog. 


instead of consume, I wonder if something that says about intent to make the Service Classes provided by that broker available in the service catalog would be clearer?
their intent to make Service Classes provided by that broker available in the service catalog

Yeah, that is a good idea.

vaikas · 2016-11-30T18:30:50Z

docs/v1/api.md

+controller receives an `ADD` watch event for a new `Broker`, it contacts the
+broker to determine what service classes the broker offers:
+
+1. Makes a request against a given CF service broker's catalog endpoint


Maybe explain what CF is before using it? Or somewhere put a link to the Open Service Broker API?
https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md

How about just s/CF service broker/broker/

vaikas · 2016-11-30T18:32:12Z

docs/v1/api.md


-1. *The `Broker` that caused the `ServiceClass` was deleted*
-1. *The `ServiceClass` itself was deleted*
+1.  The controller calls the provision endpoint on the broker 


Just like above you explicitly called the GET /v2/... maybe say here:
PUT /v2/service_instances/:instance_id

Yeah, I was planning on adding that level of detail in the next pass, if you are okay with that. We can add it now if you feel strongly.

vaikas · 2016-11-30T18:36:01Z

docs/v1/api.md


-*TODO: what happens when an `Instance` resource is deleted?*
+A `Binding` represents the binding between an application and an `Instance` of a


Consider adding a TODO to clarify what an application is in this context?

vaikas · 2016-11-30T18:36:51Z

docs/v1/api.md

+1.  The name of a Kubernetes core `Service` resource to provide a stable
+    endpoint for the application to use the `Instance` via
+2.  The name of a Kubernetes `Secret` resource to hold credentials necessary to
+    use the service


TODO for ConfigMap / Injection Policy, or whatever it was called?

vaikas · 2016-11-30T18:39:59Z

docs/v1/api.md

+1.  The controller calls the binding endpoint on the broker
+2.  The broker returns a response containing credentials and coordinates for
+    the binding
+3.  The controller creates a Kubernetes `Service` with the given name with the


TODO, I think there's other conditions here to keep track of the state (this is the discussion we had yesterday). If the object keeps state of the provisioning lifecycle. Like, after creating the binding, or something like that so that they don't get lost if things crash.

I do agree additional design and clarity on the specifics of API conditions is needed.

pmorie · 2016-12-01T01:05:38Z

@vaikas-google made some fixes, PTAL

vaikas · 2016-12-01T01:08:49Z

LGTM

duglin · 2016-12-01T01:16:13Z

docs/v1/api.md

+intent to make the service classes provided by that broker available in the
+catalog.
+
+The catalog controller has a watch on the `Broker` resource.  When the


"catalog controller" not "service catalog controller"? Are we intending to have two controllers?

Just me being imprecise. One controller.

duglin · 2016-12-01T01:18:59Z

docs/v1/api.md

+
+A `ServiceClass` represents an offering in the service catalog.  A
+`ServiceClass` is not usable directly; an `Instance` of a `ServiceClass` must be
+created to be consumed by an application.

 This resource is created by the service catalog's controller event loop after


ok for now, but I'm not a fan of repeating stuff since its asking for things to get out of sync. So we shouldn't repeat what was stated in the previous section.

I'm not a fan of repeating stuff either, but I want each thing to have an unambiguous statement of what it is associated with it in this manner.

duglin · 2016-12-01T01:21:21Z

docs/v1/api.md

+
+The service catalog controller has a watch on the `Instance` resource and
+receives an `ADD` watch event. The controller then attempts to provision a new
+instance:


should probably say:
...to provisions a new instance of the service via the corresponding service broker.
Otherwise it sounds like the controller is creating what the consumer just created.

duglin · 2016-12-01T01:22:54Z

docs/v1/api.md

-`Instance` created, it calls the provision endpoint on the backing CF service
-broker and writes `provisioned` into a `status` field of the
-`Instance`.
+A `Binding` represents the binding between an application and an `Instance` of a


perhaps "... represents a "used by" relationship between... "
or "... represents a link between ..."
just trying to avoid using the word "bind" again.

duglin · 2016-12-01T01:24:57Z

docs/v1/api.md

+1.  The name of a Kubernetes core `Service` resource to provide a stable
+    endpoint for the application to use the `Instance` via
+2.  The name of a Kubernetes `Secret` resource to hold credentials necessary to
+    use the service


Perhaps add something like:
If these values are not provided then the name of the service instance will be used by default.

pmorie · 2016-12-01T21:41:37Z

Thanks for the feedback, @duglin -- incorporated in latest push, PTAL.

duglin · 2016-12-01T23:34:35Z

LGTM
@arschles @vaikas-google wanna re-LGTM?

arschles

small nits in here, which can be fixed in a later PR if needed. still LGTM

arschles · 2016-12-02T00:25:04Z

docs/v1/api.md


-This resource is created by an administrator to instruct the service catalog's
-controller event loop to do the following:
+A `Broker` represents an entity that provides service classes for use in the


solid wording. 👍

arschles · 2016-12-02T00:26:35Z

docs/v1/api.md

+
+The service catalog controller has a watch on the `Broker` resource.  When the
+controller receives an `ADD` watch event for a new `Broker`, it contacts the
+broker to determine what service classes the broker offers:


How about adding this bit: "... it contacts the specified backing broker to determine ..."? I fear that without the "specified backing broker" language, the difference between Broker and broker will be confused.

arschles · 2016-12-02T00:28:56Z

docs/v1/api.md

+an instance of a service class.  The `Instance` has a reference to the
+`ServiceClass` to provision an instance of.
+
+The service catalog controller has a watch on the `Instance` resource and


I think we should make the watch more explicit: "...has a watch on Instance resources..."

arschles · 2016-12-02T00:30:20Z

docs/v1/api.md

-   to be discussed later)
-1. Updates the `Binding`'s status section with the fully qualified path to the
-   aforementioned secret
+The service catalog controller has a watch on the `Binding` resource.  When the


same here RE being explicit about the watch: "... has a watch on Binding resources ..."

refine preparing for and running e2e. Save off key artifacts

* Update docs

… (CRDs) solution (#2630) * Add basic validation to crds * Add webhook skeleton, remove api-server from chart, add webhoook server in chart, move PrepareForCreate login into webhook handler (#2) * Add webhook skeleton, remove api-server from chart, add webhoook server in chart, move PrepareForCreate login into webhook handler * Add logger and GVK matcher * Add test coverage for webhook (#6) * Add Status entry initialization in binding and instance controller (#5) * Change fs to label selector (#9) * Fix removing finalizer after switching to CRD /status sub-resource (#8) * Add tests to webhooks (#11) * Replace changevalidator with webhook (#14) * Replace default service plan with webhook (#10) * Add tests to webhooks - fix * Rewrite defaultServicePlan feature to webhook * Replace plugins by webhook (#16) * Replace ServiceBinding plugin by webhook * Replace Broker plugins by webhook * Adjust webhooks to multi validation handlers * Service Catalog going towards to CRDs (#18) * Migrate registry/strategy Updates to webhooks (#17) * Use Update instead of updateReference method (#19) * Replace tableconvertor with APC (#20) * Fix svcat tests after the rebase with the upstream master branch * Pre delete jobs - remove CRD after delete helm release (#21) * Apply fixes after executing `make verify` * Create docs about webhook implementation (#24) * Change the securePort for the webhook server because colidates with old api-server * Change import paths to kubernetes-sigs, and rebase with master * Apply fixes after rebase Fixes: * makefile targets, * instance deprovision operation * entries under additionalPrinterColumns in crds.yaml * unit tests after rebase * Update documentation (#40) * Update docs * Fix vendor after rebase with k8s 1.15 bump * Apply changes after review - remove the contrib/hack/crd folder - remove reference to Kyma project - rebase with current master - restore the image in chart - extract CRDs defintion to dedicated folder

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 19, 2016

arschles reviewed Nov 21, 2016

View reviewed changes

pmorie force-pushed the api-clarification branch from a97c99d to a7323ed Compare November 22, 2016 02:47

arschles reviewed Nov 22, 2016

View reviewed changes

docs/v1/api.md

## `Instance`

*TODO: codify how asynchronous responses are handled in the controller*

Copy link

Contributor

arschles Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#49

arschles assigned pmorie Nov 22, 2016

vaikas reviewed Nov 30, 2016

View reviewed changes

pmorie force-pushed the api-clarification branch from a7323ed to 6819d6e Compare December 1, 2016 01:03

duglin reviewed Dec 1, 2016

View reviewed changes

Make clarifications and add detail to API document

070b4e0

pmorie force-pushed the api-clarification branch from 6819d6e to 070b4e0 Compare December 1, 2016 21:40

arschles approved these changes Dec 2, 2016

View reviewed changes

arschles merged commit fb274b0 into kubernetes-retired:master Dec 2, 2016

jboyd01 pushed a commit to jboyd01/service-catalog that referenced this pull request Jan 22, 2019

Merge pull request kubernetes-retired#40 from jboyd01/add-ci-artifacts

abebed4

refine preparing for and running e2e. Save off key artifacts

mszostok referenced this pull request in mszostok/service-catalog Aug 14, 2019

Update documentation (#40)

b493ea5

* Update docs

mszostok referenced this pull request in mszostok/service-catalog Aug 16, 2019

Update documentation (#40)

b0415d7

* Update docs


		## `Instance`
		TODO: codify how asynchronous responses are handled in the controller


		1. Make a request against a given CF service broker's catalog endpoint
		An administrator to creates an instance of the `Broker` resource to indicate


		TODO: what happens when an `Instance` resource is deleted?
		A `Binding` represents the binding between an application and an `Instance` of a

Make clarifications and add detail to API document #40

Make clarifications and add detail to API document #40

Conversation

pmorie commented Nov 19, 2016 • edited

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

pmorie Nov 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Nov 22, 2016

Choose a reason for hiding this comment

arschles commented Nov 22, 2016

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

pmorie commented Dec 1, 2016

vaikas commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin Dec 1, 2016 • edited

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

pmorie commented Dec 1, 2016

duglin commented Dec 1, 2016

arschles left a comment • edited

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

pmorie commented Nov 19, 2016 •

edited

pmorie Nov 22, 2016 •

edited

duglin Dec 1, 2016 •

edited

arschles left a comment •

edited