-
Notifications
You must be signed in to change notification settings - Fork 31
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
Adhoc enqueue params #579
base: master
Are you sure you want to change the base?
Adhoc enqueue params #579
Conversation
4f94842
to
5128c68
Compare
Updated tests since 2071b23 so that they no longer fail with this PR |
this is definitely a piece of work that has value. i've found myself this division between manual properties and properties that come from opencast frustrating. i'll take a look at this PR next |
workflow_parameters = None | ||
if isinstance(params, dict): | ||
workflow = params.get('workflow') | ||
workflow_parameters = params.get('workflow_parameters') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is a good way to simplify the conditionals here as you can't have workflow
without workflow_parameters
i believe
I'm wondering whether this should be targeted at |
ah yeah i missed that. see https://github.com/teltek/Galicaster/blob/master/CONTRIBUTING.md. so use master please feel free to change this to master as i don't think it could be accepted against 2.0.x i think it is safe to still use |
Ok, that sounds promising then. I just changed target branch to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a really good idea. However, I think we should support these "dict" or "json" parameters on the serializer/deserializer classes, and the set/getProperty functions should not handle this kind of logic (doesn't make sense to keep a dictionary saved as a string within the mediapackage class).
self.properties[prop] = value | ||
return True | ||
property_value = value | ||
if prop == "enqueue_params": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To allow for this kind of API:
media_package.setProperty('enqueue_params', {'workflow': 'test'})
What we need to do is to support serializing/deserializing objects on the serializer/deserializer, not here.
Also, this support should be added generically, not just for an "enqueue_params" property.
Essentially, to add this functionality what we should do is to support dictionary properties, at least. And serialize/deserialize them on the corresponding classes. The setProperty function should not be the one carrying this logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Alfro, I agree but went with the minimal changes to provide this functionality for enqueue_params
as I wasn't sure how much else would be impacted by a bigger change to serialize/deserialize other properties as well.
The file where this data is stored has changed in master
from xml
to json
(not part of a built release yet as far as I'm aware) so that should make things easier.
Due to some changes for us I'm not sure if/when I'll have time to work on this next and I'm unlikely to be able to devote much time to development so someone else may want to continue with this.
This moves serializing/deserializing of enqueue_params property values for media packages to the
setProperty
andgetProperty
methods of mediapackage.py. It also ensures that wherever anenqueue_job
ordo_job
call is made, the enqueue_params are passed along the chain until it reaches the point at which to ingest the media package.In fixing this, it is possible to specify a target
workflow
andworkflow_parameters
per media package (rather than use the workflow specified inconf.ini
) for manual (adhoc) recordings.The code for ingesting and specifying the workflow is the _ingest method of
galicaster/core/worker.py
. Manual and scheduled recordings are handled differently:params
parameter passed to the method (viajobs
that are either queued or executed immediately)org.opencastproject.capture.agent.properties
to specify a workflow definitionCurrently (2.0.x), there are only two places that media package properties are set:
I had a requirement to do the same as the latter within a plugin, e.g.
media_package.setProperty('enqueue_params', {'workflow': 'test'})
.Rather than
import json
and serialize/deserialize everywhere that theenqueue_params
property needs to be set, this PR ensures that allenqueue_params
properties are serialized/deserialized as part of thesetProperty
/getProperty
methods ofmediapackage.py
. All other properties that are set remain as strings. So the only change here is to make sure that theenqueue_params
property values are in a consistent format.The
managerui
makes calls to the worker do_job and do_job_nightly methods but didn't pass theparams
parameter on to these methods so now theenqueue_params
property is fetched and passed with these calls. Similarly the recorder serviceenqueue_ingest
didn't pass any params down the line so the same change has been made here.Generally, storing JSON representations of Python objects within a text node of XML doesn't seem like the best way to do this but for now I've gone for the path of least resistance to get this working - perhaps this file could be
.json
in future...?Unfortunately, tests already appear to fail in 2.0.x (before this PR) when the. Tests pass for this but I've also been doing a mix of printing debug statements inmanual
config option of[ingest]
is set to eithernightly
orimmediately
so testing this one isn't particularly easy_ingest
,setProperty
,getProperty
, reviewing content written to thegalicaster.xml
file and using a test plugin to start the recording: