Clean-up some text around our status codes #178

duglin · 2017-05-03T14:33:56Z

Follow-on to this comment: https://github.com/openservicebrokerapi/servicebroker/pull/166/files#r113097103

Changes:

Add MUSTs/MAYs on each status code in the tables
Add a 404 as the more appropriate response after a DELETE, not 410
Allow for a 200 in spots where it was missing
Clarify things, like when a conflict happens
lots of typographical changes (spaces, wrong casing, etc...) - nits

Signed-off-by: Doug Davis [email protected]

cfdreddbot · 2017-05-03T14:33:57Z

Hey duglin!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

mattmcneeney · 2017-05-03T14:52:26Z

spec.md

 |---|---|---|
-|  accepts_incomplete | boolean  | A value of true indicates that the marketplace and its clients support asynchronous broker operations. If this parameter is not included in the request, and the broker can only provision an instance of the requested plan asynchronously, the broker should reject the request with a 422 as described below.  |
+| accepts_incomplete | boolean | A value of true indicates that the marketplace and its clients support asynchronous broker operations. If this parameter is not included in the request, and the broker can only provision an instance of the requested plan asynchronously, the broker should reject the request with a 422 as described below. |


Should this be "the broker should reject the request with a 422 Unprocessable Entity as described below" for consistency?

probably. As you mentioned below there are quite a few spots where we just use the number and not the text, but let me see if I can find them all.

mattmcneeney · 2017-05-03T14:52:57Z

spec.md

-| 422 Unprocessable Entity  | MUST be returned if the broker only supports asynchronous provisioning for the requested plan and the request did not include <code>?accepts_incomplete=true</code>. The expected response body is: <code>{ "error": "AsyncRequired", "description": "This service plan requires client support for asynchronous service operations." }</code>, as described below.  |
+| 200 OK | MUST be returned if the service instance already exists, is fully provisioned, and the requested parameters are identical to the existing service instance. The expected response body is below. |
+| 201 Created | MUST be returned if the service instance was provisioned as a result of this request. The expected response body is below. |
+| 202 Accepted | MUST be returned if the service instance provisioning is in progress. This triggers the platform marketplace to poll the [Service Instance Last Operation Endpoint](#polling) for operation status. Note that a resent `PUT` request MUST return a 202, not a 200, if the service instance is not yet fully provisioned. |


Same comment as above.

mattmcneeney · 2017-05-03T14:53:24Z

spec.md

-| 422 Unprocessable entity  | MUST be returned if the requested change is not supported or if the request cannot currently be fulfilled due to the state of the instance (e.g. instance utilization is over the quota of the requested plan). Broker SHOULD include a user-facing message in the body; for details see [Broker Errors](#broker-errors).  Additionally, a 422 can also be returned if the broker only supports asynchronous update for the requested plan and the request did not include <code>?accepts_incomplete=true</code>; in this case the expected response body is: <code>{ "error": "AsyncRequired", "description": "This service plan requires client support for asynchronous service operations." }</code>  |
+| 200 OK | MUST be returned if the request's changes have been applied. The expected response body is <code>{}</code>. |
+| 202 Accepted | MUST be returned if the service instance update is in progress. This triggers the platform marketplace to poll the [Service Instance Last Operation Endpoint](#polling) for operation status. Note that a resent `PATCH` request MUST return a 202, not a 200, if the requested update has not yet completed. |
+| 422 Unprocessable entity | MUST be returned if the requested change is not supported or if the request cannot currently be fulfilled due to the state of the instance (e.g. instance utilization is over the quota of the requested plan). Broker SHOULD include a user-facing message in the body; for details see [Broker Errors](#broker-errors). Additionally, a 422 can also be returned if the broker only supports asynchronous update for the requested plan and the request did not include <code>?accepts_incomplete=true</code>; in this case the expected response body is: <code>{ "error": "AsyncRequired", "description": "This service plan requires client support for asynchronous service operations." }</code> |


Looks like there are lots of places. Maybe it's OK to ignore!

mattmcneeney · 2017-05-03T14:55:04Z

spec.md

-| 422 Unprocessable Entity  | MUST be returned if the broker requires that <code>app_guid</code> be included in the request body. The expected response body is: <code>{ "error": "RequiresApp", "description": "This service supports generation of credentials through binding an application only." }</code>  |
+| 200 OK | MUST be returned if the binding already exists and the requested parameters are identical to the existing binding. The expected response body is below. |
+| 201 Created | MUST be returned if the binding was created as a result of this request. The expected response body is below. |
+| 409 Conflict | MUST be returned if a service binding with the same id, for the same service instance, already exists but with different parameters. The expected response body is <code>{}</code>, though the description field MAY be used to return a user-facing error message, as described in [Broker Errors](#broker-errors). |


Regarding the 409 Conflict, should we say "but with different parameters"? I'd imagine that the conflict error should be returned if you try to create a binding for the same service instance with an existing binding ID regardless of parameters?

My assumption is that all of the APIs in the spec are meant to be idempotent, in which case returning an error on the 2nd try violates that. Also, if the first response message is lost, which is why they're resending the request, if we return a 409 how will the client recover? I think that goes back to the idempotent nature of things. This also relates to another discussion we're having about how long, or if at all, a broker needs to hold on to the credentials for a binding. I know some people have said that brokers might be stateless but that seems a bit odd in the case of credentials since they need to hold on to them so they can verify that apps are using the right ones, right? So, if they have to hang on to them to so auth checking then is it really a burden to ask them to use them to replay a binding response message?

But, if my assumption is wrong about them being idempotent then we may need to look at all of the APIs to make sure we have a clear recovery plan for each when messages are lost.

Interested to hear what @shalako thinks on this point.

I think the spec was intending to give flexibility to brokers that may not be able to replay requests for some reason.

@duglin I spoke to a few of the CF service teams about holding onto credentials. It seems that most brokers are stateless and whatever downstream component they talk to give them credentials when they create a new binding (synchronously) which they immediately return to the platform. There isn't currently a way for brokers to ask whatever is downstream to give them the credentials again for a particular binding. I'm not saying this is the right way, but I think that's what today's world looks like.

vaikas · 2017-05-08T20:02:00Z

spec.md

-| operation  |  string | A broker-provided identifier for the operation. When a value for <code>operation</code> is included with asynchronous responses for [Provision](#provisioning), [Update](#updating_service_instance), and [Deprovision](#deprovisioning) requests, the broker client SHOULD provide the same value using this query parameter as a URL-encoded string.  |
+| service_id | string | ID of the service from the catalog. |
+| plan_id | string | ID of the plan from the catalog. |
+| operation | string | A broker-provided identifier for the operation. When a value for <code>operation</code> is included with asynchronous responses for [Provision](#provisioning), [Update](#updating_service_instance), and [Deprovision](#deprovisioning) requests, the broker client SHOULD provide the same value using this query parameter as a URL-encoded string. |


SHOULD for operation (the broker client SHOULD) is being changed to MUST in another PR, I assume it's going to get changed here as a merge?

yup - doing the rebase now and its picking up the MUST

duglin · 2017-05-08T21:59:42Z

rebased

LGTM

angarg12 · 2017-05-09T06:24:49Z

LGTM

avade · 2017-05-09T07:28:45Z

LGTM

angarg12 · 2017-05-09T14:30:51Z

Looks like it needs rebasing.

duglin · 2017-05-09T14:43:43Z

rebased

angarg12 · 2017-05-09T15:51:51Z

lgtm

duglin · 2017-05-09T17:54:51Z

ping @avade @vaikas-google @pmorie for some LGTMs

avade · 2017-05-10T09:19:11Z

LGTM

duglin · 2017-05-11T18:24:11Z

rebased

angarg12 · 2017-05-12T06:04:02Z

Looks like your script is already giving results 😄

tools/../spec.md: Can't find section '#route_services' in tools/../spec.md
tools/../spec.md - 443: Use 'SHOULD'

duglin · 2017-05-12T12:13:12Z

nice! :-)
fixed - I messed up on the rebase. Would be so much easier with shorter lines.

angarg12 · 2017-05-12T12:22:29Z

LGTM

mattmcneeney · 2017-05-12T13:53:19Z

@duglin we can cut all of the lines to 80 characters and add a travis check in the #192 PR if you'd like.

duglin · 2017-05-12T13:55:53Z

I would love that :-) I'll add comments there (later today) of other syntax changes that we should make, or that I've made in other PRs (that I can pull out).

Question is... do we merge #192 before or after the other current PRs? :-) While it might cause a ton of rebases (especially for me), it would make it easier to review those other PRs if #192 went in first.

mattmcneeney · 2017-05-12T13:58:59Z

@duglin Whilst #192 would mean other PRs need a rebase, the rebases should typically be fairly small as long as the PR is only touching a small bit of the spec. Some fixes that people (ie. me) had bundled into other PRs when they spotted them have been pulled into #192 so it should ensure the other PRs are cleaner too.

I'm probably biased (I've been rebasing the PR on each change which isn't the best job) but I think it's sensible enough for merge it soon. But happy with whatever everyone else would like.

duglin · 2017-05-23T01:16:23Z

rebase

duglin · 2017-05-28T11:56:56Z

ready for review

avade · 2017-05-30T10:33:30Z

spec.md

-| 410 Gone | Appropriate only for asynchronous delete operations. The platform MUST consider this response a success and remove the resource from its database. The expected response body is `{}`. Returning this while the platform is polling for create or update operations SHOULD be interpreted as an invalid response and the platform SHOULD continue polling. |
+| 200 OK | MUST be returned upon successful processing of this request. The expected response body is below. |
+| 404 Not Found | MUST be returned if the service instance does not exist. |
+| 410 Gone | MAY be returned if the broker knows about the service instance but it had been previously deleted. Appropriate only for asynchronous delete operations. The platform MUST consider this response a success and remove the resource from its database. The expected response body is `{}`. Returning this while the platform is polling for create or update operations SHOULD be interpreted as an invalid response and the platform SHOULD continue polling. While a `410 Gone` is allowed, the RECOMMENDED response is `404 Not Found`. |


@mattmcneeney / @zrob does CF already conform to this flow?

For async delete operations, brokers must return a 410. For async create and update, brokers must return a 200. If the resource does not exist, brokers must return a 410.

A 404 is problematic because it may be temporarily returned by a intermediating component (e.g. the CF router). We would not want a platform to remove a resource from its database as a result of a 404 response, since we cannot guarantee that a 404 was returned by the broker itself.

this feels like we're letting an impl detail bleed into the spec and, sort of, reinventing http response codes in the process. Can we discuss this on the next call?

agree with @shalako that this is a change to the spec, not appropriate for a PR titled clean-up text

perhaps a separate issue should be raised around what status codes should be supported. as is, this change potentially invalidates existing broker and platform implementations

I'll move this to a new issue since I think more discussions are needed.

avade · 2017-05-30T10:33:47Z

LGTM

duglin · 2017-05-31T10:49:08Z

rebased again

vaikas · 2017-05-31T16:03:48Z

LGTM

avade · 2017-06-01T15:07:42Z

LGTM

angarg12 · 2017-06-02T06:02:18Z

LGTM

shalako · 2017-06-02T17:16:48Z

spec.md

+| previous_values.service_id | string | ID of the service for the instance. |
+| previous_values.plan_id | string | ID of the plan prior to the update. |
+| previous_values.organization_id | string | ID of the organization specified for the instance. |
+| previous_values.space_id | string | ID of the space specified for the instance. |


I believe the escaped underscores were required to prevent the CF docs from styling characters between them as italic.

The escape char as been removed in other spots (I think by Matt) so this is just following that pattern. Notice like 449 doesn't have it. Or is there something special about these lines that makes it required here? @mattmcneeney ?

If it was remove elsewhere, it may be breaking CF docs. Members of this group who are involved with the CF community must consider themselves custodians of the CF broker docs, and consider impact when making nitpick changes.

That said, it is possible that @mattmcneeney has discussed the topic with the CF docs team and confirmed that removal of the escape char no longer causes a change in styling.

shalako · 2017-06-02T17:28:10Z

spec.md

-| 410 Gone | MUST be returned if the binding does not exist. The expected response body is `{}`. |
+| 200 OK | MUST be returned if the binding was deleted as a result of this request. The expected response body is `{}`. |
+| 404 Not Found | MUST be returned if the service binding does not exist. |
+| 410 Gone | MAY be returned if the binding is known to the service broker but had been previously deleted. The expected response body is `{}`. While a `410 Gone` is allowed, the RECOMMENDED response is `404 Not Found`. |


Since a 404 may be temporarily returned by an intermediating component, we wouldn't want the platform to execute a state change based on it. Only 200 and 410 should be supported responses.

reverting for now and we can discuss later - see previous comment on this

shalako · 2017-06-02T17:29:38Z

spec.md

+| 200 OK | MUST be returned if the service instance was deleted as a result of this request. The expected response body is `{}`. |
+| 202 Accepted | MUST be returned if the service instance deletion is in progress. This triggers the marketplace to poll the [Service Instance Last Operation Endpoint](#polling-last-operation) for operation status. Note that a resent `DELETE` request MUST return a `202 Accepted`, not a `200 OK`, if the delete request has not completed yet. |
+| 404 Not Found | MUST be returned if the service instance does not exist. |
+| 410 Gone | MAY be returned if the service instance is known to the broker but had been previously deleted. The expected response body is `{}`. While a `410 Gone` is allowed, the RECOMMENDED response is `404 Not Found`. |


Same issues with 404. We should not introduce this as a supported response without further discussion.

reverting for now

shalako · 2017-06-02T17:49:28Z

@duglin we can cut all of the lines to 80 characters and add a travis check in the #192 PR if you'd like.

Please don't cut all lines. Soft wrap is your friend.

mattmcneeney · 2017-06-02T18:14:18Z

I haven't spoken to the docs team, so maybe I shouldn't have removed the underscores. I'll check this and re-escape them if necessary.

…

On Fri, 2 Jun 2017 at 18:49, Shannon Coen ***@***.***> wrote: @duglin <https://github.com/duglin> we can cut all of the lines to 80 characters and add a travis check in the #192 <#192> PR if you'd like. Please don't cut all lines. Soft wrap is your friend. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#178 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG7UzHpwVDClxFQYOjBjvSzYSA4kuv3Dks5sAEsrgaJpZM4NPfVf> .

zrob · 2017-06-04T02:08:04Z

spec.md

@@ -387,16 +388,15 @@ $ curl http://username:password@broker-url/v2/service_instances/:instance_id?acc
 }' -X PUT -H "X-Broker-API-Version: 2.11" -H "Content-Type: application/json"
 ```

-### Response


was removing this heading accidental?

geez, not sure how I did this - thanks

zrob · 2017-06-04T02:15:01Z

spec.md

-| 200 OK | The requested changes have been applied. The expected response body is `{}`. |
-| 202 Accepted | Service instance update is in progress. This triggers the platform marketplace to poll the [Last Operation](#polling-last-operation) endpoint for operation status. |
+| 200 OK | MUST be returned if the request's changes have been applied. The expected response body is `{}`. |
+| 202 Accepted | MUST be returned if the service instance update is in progress. This triggers the platform marketplace to poll the [Last Operation](#polling-last-operation) for operation status. Note that a resent `PATCH` request MUST return a `202 Accepted`, not a `200 OK`, if the requested update has not yet completed. |


resent -> re-sent

done - and in the other spots too

zrob · 2017-06-04T02:18:09Z

spec.md

-| 200 OK | MAY be returned if the service instance already exists and the requested parameters are identical to the existing service instance. The expected response body is below. |
-| 202 Accepted | Service instance provisioning is in progress. This triggers the platform marketplace to poll the [Last Operation](#polling-last-operation) endpoint for operation status. |
+|---|---|
+| 200 OK | MUST be returned if the service instance already exists, is fully provisioned, and the requested parameters are identical to the existing service instance. The expected response body is below. |


A change from MAY to MUST feels like a spec change rather than a text fix. It looks like the intent is that 201 can be returned in the case the service instance already existed. Are we concerned with enforcing a change here?

Since PUT is supposed to be idempotent, I was trying to distinguish between "already there", "completed as a result of this request" and "doing it asynchronously" as the platform may want to know what happened on this particular request. IMO, the old wording implies what the new text says - just not as clearly.

I agree with @duglin on this matter. MAY is also significantly less useful (ie, not useful at all) compared to MUST here.

I agree that MUST clarifies things here, because what else would you return, as there is only one conditional that can trigger this case.

pmorie

Some nits, generally LGTM

pmorie · 2017-06-06T15:16:45Z

spec.md

-| 200 OK | MAY be returned if the service instance already exists and the requested parameters are identical to the existing service instance. The expected response body is below. |
-| 202 Accepted | Service instance provisioning is in progress. This triggers the platform marketplace to poll the [Last Operation](#polling-last-operation) endpoint for operation status. |
+|---|---|
+| 200 OK | MUST be returned if the service instance already exists, is fully provisioned, and the requested parameters are identical to the existing service instance. The expected response body is below. |


I agree with @duglin on this matter. MAY is also significantly less useful (ie, not useful at all) compared to MUST here.

pmorie · 2017-06-06T15:17:08Z

spec.md

 | 409 Conflict | MUST be returned if a service instance with the same id already exists but with different attributes. The expected response body is `{}`. |
 | 422 Unprocessable Entity | MUST be returned if the broker only supports asynchronous provisioning for the requested plan and the request did not include `?accepts_incomplete=true`. The expected response body is: `{ "error": "AsyncRequired", "description": "This service plan requires client support for asynchronous service operations." }`, as described below. |

+


spurious newline?

pmorie · 2017-06-06T15:20:14Z

spec.md

+|---|---|
+| 200 OK | MUST be returned if the service instance already exists, is fully provisioned, and the requested parameters are identical to the existing service instance. The expected response body is below. |
+| 201 Created | MUST be returned if the service instance was provisioned as a result of this request. The expected response body is below. |
+| 202 Accepted | MUST be returned if the service instance provisioning is in progress. This triggers the platform marketplace to poll the [Service Instance Last Operation Endpoint](#polling-last-operation) for operation status. Note that a re-sent `PUT` request MUST return a `202 Accepted`, not a `200 OK`, if the service instance is not yet fully provisioned. |
 | 409 Conflict | MUST be returned if a service instance with the same id already exists but with different attributes. The expected response body is `{}`. |


This should have the same guidance as we have elsewhere for conflicts - that user-facing errors may be supplied

Follow-on to this comment: https://github.com/openservicebrokerapi/servicebroker/pull/166/files#r113097103 Changes: - Add MUSTs/MAYs on each status code in the tables - Add a 404 as the more appropriate response after a DELETE, not 410 - Allow for a 200 in spots where it was missing - lots of typographical changes (spaces, wrong casing, etc...) - nits Signed-off-by: Doug Davis <[email protected]>

shalako · 2017-06-06T22:10:08Z

LGTM

avade · 2017-06-06T22:26:30Z

LGTM

vaikas · 2017-06-06T22:36:35Z

LGTM

zrob · 2017-06-06T22:40:34Z

LGTM

mattmcneeney reviewed May 3, 2017

View reviewed changes

duglin force-pushed the clarify202 branch from a957ac6 to 09299fd Compare May 4, 2017 12:22

duglin mentioned this pull request May 8, 2017

Move OSBDashboardURL from InstanceSpec to InstanceStatus and store the URL returned by the broker kubernetes-retired/service-catalog#762

Merged

vaikas reviewed May 8, 2017

View reviewed changes

duglin force-pushed the clarify202 branch from 09299fd to 6bf8a6e Compare May 8, 2017 21:44

duglin force-pushed the clarify202 branch from 6bf8a6e to 84fc715 Compare May 9, 2017 14:43

duglin force-pushed the clarify202 branch from 84fc715 to a9f8905 Compare May 11, 2017 18:23

duglin force-pushed the clarify202 branch from a9f8905 to 3cbbbc2 Compare May 12, 2017 12:12

duglin force-pushed the clarify202 branch from 3cbbbc2 to 2d218b9 Compare May 23, 2017 00:54

avade modified the milestone: 2.12 May 24, 2017

avade reviewed May 30, 2017

View reviewed changes

duglin force-pushed the clarify202 branch from ad54782 to 5afa320 Compare May 31, 2017 10:48

shalako reviewed Jun 2, 2017

View reviewed changes

zrob reviewed Jun 4, 2017

View reviewed changes

duglin force-pushed the clarify202 branch from 5afa320 to b622b02 Compare June 4, 2017 16:45

duglin mentioned this pull request Jun 4, 2017

Should we use 404's #216

Closed

duglin force-pushed the clarify202 branch 2 times, most recently from a338bb0 to 0b055a1 Compare June 4, 2017 22:33

pmorie reviewed Jun 6, 2017

View reviewed changes

duglin force-pushed the clarify202 branch from 0b055a1 to ce6d2bc Compare June 6, 2017 16:25

avade merged commit 3bbb81c into openservicebrokerapi:master Jun 6, 2017

duglin deleted the clarify202 branch June 7, 2017 19:58

pmorie mentioned this pull request Jun 13, 2017

Release Proposal: v2.12 #226

Merged

		\| 409 Conflict \| MUST be returned if a service instance with the same id already exists but with different attributes. The expected response body is `{}`. \|
		\| 422 Unprocessable Entity \| MUST be returned if the broker only supports asynchronous provisioning for the requested plan and the request did not include `?accepts_incomplete=true`. The expected response body is: `{ "error": "AsyncRequired", "description": "This service plan requires client support for asynchronous service operations." }`, as described below. \|

Clean-up some text around our status codes #178

Clean-up some text around our status codes #178

Conversation

duglin commented May 3, 2017

cfdreddbot commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duglin commented May 8, 2017 • edited by chipchilders Loading

angarg12 commented May 9, 2017 • edited by chipchilders Loading

avade commented May 9, 2017 • edited by chipchilders Loading

angarg12 commented May 9, 2017

duglin commented May 9, 2017

angarg12 commented May 9, 2017 • edited by chipchilders Loading

duglin commented May 9, 2017 • edited by chipchilders Loading

avade commented May 10, 2017 • edited by chipchilders Loading

duglin commented May 11, 2017

angarg12 commented May 12, 2017 • edited Loading

duglin commented May 12, 2017

angarg12 commented May 12, 2017 • edited by chipchilders Loading

mattmcneeney commented May 12, 2017

duglin commented May 12, 2017

mattmcneeney commented May 12, 2017

duglin commented May 23, 2017

duglin commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avade commented May 30, 2017 • edited by chipchilders Loading

duglin commented May 31, 2017

vaikas commented May 31, 2017 • edited by chipchilders Loading

avade commented Jun 1, 2017 • edited by chipchilders Loading

angarg12 commented Jun 2, 2017 • edited by chipchilders Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalako commented Jun 2, 2017

mattmcneeney commented Jun 2, 2017 via email

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 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

shalako commented Jun 6, 2017 • edited by chipchilders Loading

avade commented Jun 6, 2017 • edited by chipchilders Loading

vaikas commented Jun 6, 2017 • edited by chipchilders Loading

zrob commented Jun 6, 2017 • edited by chipchilders Loading

duglin commented May 8, 2017 •

edited by chipchilders

Loading

angarg12 commented May 9, 2017 •

edited by chipchilders

Loading

avade commented May 9, 2017 •

edited by chipchilders

Loading

angarg12 commented May 9, 2017 •

edited by chipchilders

Loading

duglin commented May 9, 2017 •

edited by chipchilders

Loading

avade commented May 10, 2017 •

edited by chipchilders

Loading

angarg12 commented May 12, 2017 •

edited

Loading

angarg12 commented May 12, 2017 •

edited by chipchilders

Loading

avade commented May 30, 2017 •

edited by chipchilders

Loading

vaikas commented May 31, 2017 •

edited by chipchilders

Loading

avade commented Jun 1, 2017 •

edited by chipchilders

Loading

angarg12 commented Jun 2, 2017 •

edited by chipchilders

Loading

shalako commented Jun 6, 2017 •

edited by chipchilders

Loading

avade commented Jun 6, 2017 •

edited by chipchilders

Loading

vaikas commented Jun 6, 2017 •

edited by chipchilders

Loading

zrob commented Jun 6, 2017 •

edited by chipchilders

Loading