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: improvements to the plugin system #3511

Closed

Conversation

jeronimoalbi
Copy link
Member

@jeronimoalbi jeronimoalbi commented May 16, 2023

Related to #3498

@jeronimoalbi jeronimoalbi requested a review from clockworkgr May 16, 2023 17:34
@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented May 16, 2023

@clockworkgr I propose to remove WithPaths and WithModuleAnalysis properties from the Manifest type to keep the manifest clean and maybe avoid compatibility issues.

This PR against your POC branch already removed these and added other things which are needed.

I believe the right way to configure plugins right now is by using the plugins config file, it has a with option that can also be initialized when adding the plugin:

ignite plugin -g github.com/ignite/example-plugin \
  include-app-path=true \
  include-proto-paths=true

The config would look like:

plugins:
- path: github.com/ignite/example-plugin
  with:
    include-app-path: "true"
    include-proto-paths: "true"

Once configured these values should be available to the execution command which could check for things like:

if p.With["include-app-path"] == "true" {
	// Note: Init chain only when a relevant chain option is available
	execCmd.Data.AppPath = c.AppPath()
}

if p.With["include-proto-paths"] == "true" {
	// Extract proto paths and add them to execute command call
	execCmd.Data.ProtoImports = codeanalisys.ExtractProtoPaths(c.AppPath())
}

To accomplish this last part I modified the ExecutedCommand type by adding:

type ExecutedCommand struct {
	// ...

	// Data contains runtime data sent to the plugin.
	// Runtime data is generated right before execution
	// and can be configured using plugin config parameters.
	Data ExecuteData
}

The telescope plugin can then use something like:

fmt.Println(cmd.Data.ProtoImports)
fmt.Println(cmd.Data.AppPath)

This solution is a bit hacky but I thing would maybe be the less intrusive right now and would unblock you so you could create the telescope plugin.

We would need to register some extra types so they can be serialized by Go.

The idea can be further elaborated and improved but the intention here is to keep it as simple as possible until we can agree on a good API for the plugins that can accomplish what you need.

@jeronimoalbi
Copy link
Member Author

@clockworkgr what do you think?

@clockworkgr
Copy link
Collaborator

@clockworkgr what do you think?

Technically I wasnt blocked as the previous iteration also worked for me.

I like separating the data from the With map....however, i don't like making it configurable as technically the data is "required" for each specific plugin and not something that can be turned on or off...I.e. if a plugin needs the module list....its not really an option to be turned on or off...hence why it was in the manifest.

As far as compatibility goes, I think it's pretty much the same (one changes the manifest, the other changes the ExecuteCommand)

Did you have a look at the gRPC stuff?

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented May 17, 2023

I like separating the data from the With map....however, i don't like making it configurable as technically the data is "required" for each specific plugin and not something that can be turned on or off...I.e. if a plugin needs the module list....its not really an option to be turned on or off...hence why it was in the manifest.

Yes I though about that, but the idea I had in mind was to have a quick and clean solution aligned with the current implementation. Naturally the downside is that the plugin would have to check and return an error in case it's not properly configured.

Also using ExecuteData keeps the typing and we avoid the need of using JSON to send the data.

As far as compatibility goes, I think it's pretty much the same (one changes the manifest, the other changes the ExecuteCommand)

In a way, but the "directionality" is different, manifest is plugin->CLI while execute is CLI->plugin. Using the configuration based implementation avoids the need to make an initial call to the plugin's manifest.

In any case I have another idea I want to explore.

Did you have a look at the gRPC stuff?

Yes, I did, it's not difficult and can be done without breaking compatibility with plugins using the current protocol in case it would be relevant to keep compatibility.

@jeronimoalbi
Copy link
Member Author

jeronimoalbi commented Jun 9, 2023

This PR is not applicable anymore as we are moving to gRPC for plugin communications. See #3529.

@jeronimoalbi jeronimoalbi deleted the jeronimoalbi/plugin-comms-poc branch June 9, 2023 15:17
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.

3 participants