Bug 302183 - [operations] ProvisioningSession is weird
Summary: [operations] ProvisioningSession is weird
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: unspecified   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.6 M6   Edit
Assignee: Susan McCourt CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2010-02-08 14:18 EST by Pascal Rapicault CLA
Modified: 2010-03-07 12:59 EST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal Rapicault CLA 2010-02-08 14:18:11 EST
I happen to be reviewing some of the operation API and I found that ProvisioningSession is a very weird object to have as API. It is used as a helper exporting most of the key services (planner, engine, provisioning event bus, etc). Also it has performProvisioningPlan which seems like it should not be API.

At this point my suggestion would be to change the visibility of performProvisioningPlan to be package visible and review the need for exporting all these other services directly.
Comment 1 Pascal Rapicault CLA 2010-02-11 11:16:31 EST
Susan, any comment on that?
Comment 2 Susan McCourt CLA 2010-02-11 11:52:45 EST
(In reply to comment #1).
> Susan, any comment on that?

sounds reasonable, I just hadn't had a chance to look at it yet.  The purpose of this class has evolved since we first started on the API.  It was a originally a (semi-opaque) token that held all of the proper services (before the provisioning agent support was added) and provided all of the old ProvisioningUtil methods.

In the first review pass on the operations API, we removed a bunch of methods that were no longer needed, and what remained were the methods that needed to be called from the UI.  I think it's important that the UI need not be "friended" by operations, so that we know we are providing the right level of API.

However it's possible that some of this can be moved to ProvisioningUI, etc.  I'll take a look.
Comment 3 Susan McCourt CLA 2010-02-11 11:54:18 EST
(In reply to comment #0)
> Also it has performProvisioningPlan which seems like it should not
> be API.

This is the only place where we "do the right thing" with respect to the
- look for installer plan
- install the installer plan and apply changes to the profile
- proceed with the install

This should not be implemented in client code, we need an API for this.  If the planner handled this instead, then we could punt from operations.
Comment 4 Susan McCourt CLA 2010-02-16 15:48:36 EST
This isn't really the same problem, but should be considered while reexamining the role of the ProvisioningSession, ProvisioningUI, and Provisioning Agent.

There are a number of UI services known by ProvisioningUI (LicenseManager, RepositoryTracker, Policy) that are currently accessed via OSGi lookup.  I'm wondering if these should instead be registered through the agent.

While working with the UI test suite, I had to do the following to create a unique "instance" of the UI...

- create a provisioning UI that knows about the test profile
- set up my own license manager that knows about the agent (agent is needed if we want to store licenses in the profile preferences)
- set up my own repository tracker to point to the right provisioning UI (so that it can get the right policy)....

Yuck.
Wondering if all the UI services should be registered with the agent, so that the agent is the token that gets all the unique UI services.

Not sure how that fits in with the notion of the "default profile id" to use for each UI.  I think that profile ID would still have to be retrieved from the provisioning UI since a provisioningAgent doesn't know about a profile id.
Comment 5 Pascal Rapicault CLA 2010-02-17 14:48:37 EST
So far the focus on the reorg of services has only been put on the headless part, however I think what you suggest just makes sense. In the long run this will allow for more flexibility.

As for the "default profile" I think we should try to minimize the places where this is being hardcoded and we should just make sure that we can instantiate the UI by just passing a profile or profile ID and let the callers pass down the necessary information.
Comment 6 Susan McCourt CLA 2010-03-04 21:43:32 EST
For M6 I'd like to focus on the API aspects, and revisit service registration (comment 4) later.

The issue is whether all these API methods are needed.

The ones I believe we absolutely need are:
- getProvisioningAgent() - if we want to get rid of the other service helper methods then we need to give callers access to the agent
- hasScheduledOperationsFor(String) 
- rememberJob(Job)
If we want clients to be able to prevent simultaneous operations rather than trigger the profile snapshot error, then we need to provide API to find out this information, and it needs to be in operations because that's where the code that finds this condition lives.  We have real use cases (PDE for example) where another component builds its own provisioning job but we want to track it.
- performProvisioningPlan (...) - this really belongs in the planner but regardless we need API to "do the right thing" with meta requirements.  I imagine we could deprecate this if/when the planner provides an equivalent.

The rest of the methods can either be moved to the UI bundle as internal methods or in the case of the service getters, the client can just get them from the agent.

This is what I'll implement tonight or later this weekend.  Just wanted to give Pascal a chance to comment first.
Comment 7 Susan McCourt CLA 2010-03-07 12:59:51 EST
Fixed in HEAD >03072010.
There was a PDE use of one of the methods that we want to make non-API.  That method will stay public and be marked deprecated until PDE can change this.  (Opened bug 304942 to get the PDE dependency removed.)