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

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

Closed
eriknelson opened this issue Feb 17, 2018 · 4 comments · Fixed by #773
Closed

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

eriknelson opened this issue Feb 17, 2018 · 4 comments · Fixed by #773
Assignees
Labels
3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 tech-debt

Comments

@eriknelson
Copy link
Contributor

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 eriknelson self-assigned this Feb 17, 2018
@eriknelson eriknelson added 3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 tech-debt labels Feb 17, 2018
@eriknelson
Copy link
Contributor Author

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

@jmrodri
Copy link
Contributor

jmrodri commented Feb 18, 2018

Related is the common code in the apb functions: #501

@mhrivnak
Copy link
Member

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.10 | release-1.2 Kubernetes 1.10 | Openshift 3.10 | Broker release-1.2 tech-debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants