Skip to content

v3(services): deleting msi synchronously#1729

Merged
kirederik merged 2 commits intocloudfoundry:masterfrom
cloudfoundry-incubator:v3-delete-service-instance-171726168
Jul 8, 2020
Merged

v3(services): deleting msi synchronously#1729
kirederik merged 2 commits intocloudfoundry:masterfrom
cloudfoundry-incubator:v3-delete-service-instance-171726168

Conversation

@kirederik
Copy link
Copy Markdown

[#171726168](https://www.pivotaltracker.com/story/show/171726168)

Co-authored-by: George Blue <gblue@pivotal.io>
Co-authored-by: Brian Butz <bbutz@pivotal.io>
@kirederik kirederik added the sapi label Jul 7, 2020
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented Jul 7, 2020

CLA Check

@cf-gitbot
Copy link
Copy Markdown

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

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

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

Comment thread app/jobs/v3/delete_service_instance_job.rb Outdated
def asynchronous_destroy(service_instance)
delete_job = V3::DeleteServiceInstanceJob.new(
service_instance.guid,
:deprovision,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It feels a bit like we are exposing the details of the Job by providing the method we are going to send to the broker client within the action. It also feels a bit weird since the method that will be called on the client is being put into the database through the job queueing.

I think you hinted at a potential refactor across all these jobs, so it makes sense to keep this for now and maybe after the refactoring there might be an obvious way to expose this detail outside the job?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, this is pretty much a work in progress at this point and not quite there yet. I'll keep your point in mind when consolidating the jobs 🤔

Co-authored-by: Brian Butz <butzopower@gmail.com>
@kirederik kirederik merged commit 80e350f into cloudfoundry:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants