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

Implement script storage #36

Open
Yoric opened this issue Mar 5, 2016 · 8 comments
Open

Implement script storage #36

Yoric opened this issue Mar 5, 2016 · 8 comments

Comments

@Yoric
Copy link
Collaborator

Yoric commented Mar 5, 2016

We need some kind of CRUD API for Thinkerbell.

Possibly something along the lines of:

impl<Env> ScriptManager<Env> {
  /// Create a new script manager at a given path (if provided).
  ///
  /// This directory will be used to load/store scripts. If the directory doesn't exist, attempt to create it or return an error.
  fn new<F>(path: Option<String>) -> Result<Self, Error>;

  /// Attempt to read, parse and launch the scripts at the given path, asynchronously.
  ///
  /// If any script causes an error, report that script, but do not let that error prevent other scripts from being read/parsed/launched.
  ///
  /// Only scripts that are enabled should be loaded.
  fn load<F>(&self) -> HashMap<String, Result<(), Error>>;

  /// Attempt to add a new script, asynchronously.
  ///
  /// If the script can be parsed/compiled/executed, this script is immediately saved to the disk.
  fn put<F>(&self, id: &String, source: &String) -> Result<(), Error>;

  /// Attempt to enable/disable a script.
  ///
  /// Disabling a script stops it and stores the information to disk.
  /// Enabling a script attempts to start it and stores the information to disk.
  fn set_enabled<F>(&self, id: &String, enabled: bool) -> Result<(), Error>;

  /// Attempt to remove a script.
  ///
  /// If the script is running, stop it. Either way, remove it from the disk.
  fn remove<F>(&self, id: &String) -> Result<(), Error>;
}

Note that none of the methods should cause a panic, ever.

For a v2, we should also handle argument on_event of Execution::<Env>::run(), but this can wait for a followup.

edit Reworked with a sync API.

@Yoric Yoric added this to the Integrate with FoxBox milestone Mar 5, 2016
@mcav mcav self-assigned this Mar 7, 2016
@mcav mcav closed this as completed Mar 8, 2016
@mcav
Copy link
Contributor

mcav commented Mar 8, 2016

oops, wrong button!

@mcav mcav reopened this Mar 8, 2016
@Yoric
Copy link
Collaborator Author

Yoric commented Mar 8, 2016

I would have gone for one file per rule, in json – with a directory for active rules and another one for inactive ones. I don't think that sqlite adds anything useful to the mix.

@mcav
Copy link
Contributor

mcav commented Mar 8, 2016

Works for me.

@mcav
Copy link
Contributor

mcav commented Mar 9, 2016

@Yoric A couple questions:

  • Do you have a WIP branch where thinkerbell compiles with the latest taxonomy? I'm muddling through this but I'm not sure what your intent is.
  • taxonomy::API should be Sync in addition to Send, right? The spec you've outlined here requires API to be called from multiple threads (as a result of async callbacks).

@Yoric
Copy link
Collaborator Author

Yoric commented Mar 9, 2016

Do you have a WIP branch where thinkerbell compiles with the latest taxonomy? I'm muddling through this but I'm not sure what your intent is.

No, I'm afraid that I haven't updated thinkerbell to the latest taxonomy. Since the Taxonomy API is still changing, as I'm writing the real implementation of foxbox_taxonomy::api::API, I don't think you should spend time getting Thinkerbell to build with the head. But the Cargo.toml has a pinned changeset that should let it compile. Doesn't it work?

taxonomy::API should be Sync in addition to Send, right? The spec you've outlined here requires API to be called from multiple threads (as a result of async callbacks).

Making it Sync would be a nightmare, but thinkerbell uses it with just Send without problem, so I don't think that this is necessary. On the other hand, the name API is perhaps poorly chosen. Perhaps APIHandle would be better, as it is implemented behind the scenes as a Sender and a back-end thread.

@mcav
Copy link
Contributor

mcav commented Mar 9, 2016

But the Cargo.toml has a pinned changeset that should let it compile. Doesn't it work?

Yeah it compiles on its own, but I'm not sure how to write ScriptManager in a way that doesn't require API to be Sync rather than Send. I put up my branch so far here in case you see anything horribly wrong. (That changeset is dirty/incomplete/broken, as I was trying to pull it closer to date with taxonomy.)

I'm wondering if it may make sense to hold off on finishing this until taxonomy's more baked. In theory, ScriptManager should be straightforward to implement (i.e. reading files, loading them into the script manager); I'm just snagging myself on the hookup points with the API. Or, if you can see what I'm doing wrong in handling the API, I can try to get unstuck.

@Yoric
Copy link
Collaborator Author

Yoric commented Mar 9, 2016

I feel that you don't even need to touch the API for this issue. Let me take a look at your branch.

@Yoric
Copy link
Collaborator Author

Yoric commented Mar 10, 2016

@mcav Actually, I realize that sqlite would add something to the mix. It would save us from command injections, path injections, etc. So I revise my judgement and I would suggest heading towards sqlite, either now or in a future iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@Yoric @mcav and others