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

feat: Enable external plugins to provide storage, publishing and auth mechanisms #1887

Closed
wants to merge 11 commits into from

Conversation

benbrown
Copy link
Contributor

@benbrown benbrown commented Jan 21, 2020

Description

This PR includes 3 interrelated bits:

  • Adjustments to the server to enable it to use a drop-in replacement for the file system storage
  • A new plugin loader that evaluates and ingests plugin packages
  • ability to plugin to storage, auth and publishing via the plugin
  • A storage plugin that implements MongoDB backed storage

Still To Do

  • plumb auth info down into storage layer
  • Rationalize differences between my changes to csharpBotconnector and changes already in master
  • Create a page in the app for managing Composer-wide settings such as DB connect string
  • Code review and revisions
  • Review of plugin manifest structure and capabilities

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@benbrown benbrown changed the base branch from stable to master January 21, 2020 18:58
@benbrown benbrown changed the title Enable external plugins to provide storage mechanism feat: Enable external plugins to provide storage mechanism Jan 21, 2020
add sample web route plugin
first tiny stab at publish -- TIME TO BRANCH
@boydc2014
Copy link
Contributor

Great to see, @benbrown i was working on a LocalBotManager service which implements "publish\history\rollback", it's ready now, so i was trying to implement a IPublisher in composer to hook up. I saw you already have a publisher, I will take a look see how i integrate with this. Also left some comments based on my learning and investigation.


import pluginLoader from '../services/pluginLoader';

export const PublishController = {
Copy link
Contributor

@boydc2014 boydc2014 Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should consider make this a REST api managing "publishers" which means:

  1. Get /publishers, return a bunches of publisher plugins with ID, having this will enable the UI to list out all the publishers and their recent activities.
  2. POST /publishers/:pid/publish, publishing using one publisher
  3. POST /publishers/:pid/(rollback/history/status)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree, this is part of the work I've done already in another branch!

]).join(' ')
);
}
pluginLoader.loadPluginsFromFolder(__dirname + '/../../../plugins', app).then(() => {
Copy link
Contributor

@boydc2014 boydc2014 Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadPluginsFromFolders is great. Can you support also loading from code, by that, i was facing such a scenario:

I was trying to create a HttpPublisherGenerator which takes a config (name, host, etc) and return a IPublisher, so that for different slot, i will register different Publisher hopefully like this

var httpPublishers:  IPublisher = [
    HttpPublisherGenerator(config1), // config 1 with product slot url
    HttpPublisherGenerator(config2), // config 2 with testing slot url  
]

So that, i wouldn't create a new folder only for a different endpoint.

My hope is those publishers are loaded together with plugins in the folder, and surfaced out through the same publisher server api.

Does this make sense to you, and what's your view on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having seen your implementation, I don't 100% agree with this.

I think you would register ONE instance of the HttpPublisher, and then pass in the appropriate config, which is a bot-specific setting rather than a Composer-wide setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, saw you comments there, i haven't thought about whether the publisher configuration is per-bot or per-composer, my gut feeling is making the config per bot would be more flexible, on the other hand making the configuration in composer will be easier to manage and a little clean. Let me mark that as a open question and think about that. cc
two chris @christopheranderson @cwhitten

One of my focus on prototype was trying to setup a structure that code-based publisher and config-based plugin (throw HttpPublisher) can work well together. So that, average users don't have to code to enable a plugin (i believe that's part of intention of your original design doc) and we still keep the possibility to let user code when user want to or have to, for features that can't achieve throw by configuring HttpPublisher, which meet @christopheranderson's point at that meeting.

Anyway, either way it's OK to me, and we need to make sure we surface the configuration out, become multiple plugins or multiple profiles (for one plugin).

@benbrown benbrown changed the title feat: Enable external plugins to provide storage mechanism feat: Enable external plugins to provide storage, publishing and auth mechanisms Feb 10, 2020
@benbrown
Copy link
Contributor Author

I am now exploring what it will take to make the storage layer identity aware -- that is, passing in user information from the auth system all the way down to each storage read/write operation.

@benbrown
Copy link
Contributor Author

Superceded by #2023

@benbrown benbrown closed this Feb 19, 2020
@benbrown benbrown deleted the benbrown/storage branch February 19, 2020 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants