Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return statusOK when resource exists #77

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Return statusOK when resource exists #77

merged 2 commits into from
Sep 11, 2019

Conversation

ataleksandrov
Copy link
Contributor

I have the same use case as in issue.

Added:

  • Provision handler returns status 200 OK if an instance is already present
  • Bind handler returns status 200 OK if a binding is already present

Pull request:

  • implementation
  • tests

@cf-gitbot
Copy link
Member

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/168191701

The labels on this github issue will be updated when the story is started.

@winnab
Copy link
Contributor

winnab commented Sep 2, 2019

Thanks so much, @ataleksandrov! We should be able to review this week and get back to you.

Copy link

@waterlink waterlink left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@@ -169,6 +170,7 @@ type UnbindSpec struct {

type Binding struct {
IsAsync bool `json:"is_async"`
AlreadyExists bool `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why this json:"-" annotation rather than json:"already_exists"? cc @FelisiaM

@@ -87,6 +87,16 @@ func (h APIHandler) Bind(w http.ResponseWriter, req *http.Request) {
return
}

if binding.AlreadyExists {
Copy link
Contributor

@winnab winnab Sep 5, 2019

Choose a reason for hiding this comment

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

We think it makes more sense to check if the instance already exists before checking if the response is async. Whether the response is async or synchronous, we want to return 200 if the instance already exists.

There would need to be a new test to be sure that if the broker response is asyn and says that the binding already exists that the returned response is 200.

}

var statusCode int
if provisionResponse.AlreadyExists {
Copy link
Contributor

Choose a reason for hiding this comment

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

We think it makes more sense to check if the instance already exists before checking if the response is async. Whether the response is async or synchronous, we want to return 200 if the instance already exists.

There would need to be a new test to be sure that if the broker response is asyn and says that the instance already exists that the returned response is 200.

@winnab
Copy link
Contributor

winnab commented Sep 5, 2019

@FelisiaM and I took a look and left a few questions/comments as single-line comments in files. Let us know if you have questions. Thanks!

@winnab winnab merged commit 0595f91 into pivotal-cf:master Sep 11, 2019
@winnab
Copy link
Contributor

winnab commented Sep 11, 2019

Looks good @ataleksandrov, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants