openshift / ansible-service-broker

Ansible Service Broker
Apache License 2.0
226 stars 84 forks source link

Public apb action methods should be thoughtfully considered, and reworked. #771

Closed eriknelson closed 6 years ago

eriknelson commented 6 years ago

This issue stems from much of the discussion on #619

There are a few items we felt needed to be addressed in a follow up PR, and tracked on this issue:

1) The apb pkg should expose the suite of actions (provision, depro etc.) with async and and sync variants. The async variants are expected to be non-blocking, and will return a channel for event-driven updates. It is the caller's responsibility to monitor this channel for status. The async variant is a recommended best practice. Alternatively, sync variants should also be available. These will block, and callers should assume the APBs have run to completion and the method should return the result of the execution.

2) The separation of Sync/Async jobs should be reverted. The WorkEngine is for asynchronous work, and there is little value for a sync job with status updates.

eriknelson commented 6 years ago

Just assigning you guys so you're aware of the work here.

jmrodri commented 6 years ago

Related is the common code in the apb functions: https://github.com/openshift/ansible-service-broker/issues/501

mhrivnak commented 6 years ago

Sounds reasonable. If we get the async API right, I think it'll be easy to add a blocking function in front of it that just watches the channel. Something more-or-less like this:

for {
  report := <- reportChan
  if report.State == "failed" {
    return nil, errors.New("oops it failed")
  } else if report.State == "success" {
    return report.Result, nil
  }
}
eriknelson commented 6 years ago

@mhrivnak +1, exactly what I was thinking. Trying to get that into #773 today and have the sync branches call them.