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: PoC for passing data to plugins #3498

Closed
wants to merge 6 commits into from
Closed

Conversation

clockworkgr
Copy link
Collaborator

After discussions with @tbruyelle I went with a simpler approach than what I originally had in mind in order to not break existing plugins.

The basic idea is that Manifest has flags that can be toggled in the plugin implementation.

Ignite command reads those toggles and augments the plugin config passed to the plugin with data generated/fetched on the fly.

@tbruyelle
Copy link
Contributor

tbruyelle commented May 10, 2023

Suggestion: if you need to access chain.Chain in the plugin, what about turning cmd.newChainWithHomeFlags() public, so your plugin can invoke it, like many other ignite commands ?

EDIT: or even simpler, have a look at the main.go of a scaffolded plugin, there's a getChain() function, that returns exactly the chain.Chain struct.

ignite/cmd/plugin.go Outdated Show resolved Hide resolved
@clockworkgr
Copy link
Collaborator Author

Suggestion: if you need to access chain.Chain in the plugin, what about turning cmd.newChainWithHomeFlags() public, so your plugin can invoke it, like many other ignite commands ?

EDIT: or even simpler, have a look at the main.go of a scaffolded plugin, there's a getChain() function, that returns exactly the chain.Chain struct.

The main idea is to reduce plugin dependencies to the CLI project as much as possible so people don't have to figure out too much about CLI internals.

I know about the getChain method but assume someone wants to get some more complex data and it gets complicated quickly.

Also, importing CLI pkgs to get data kinda defeats the purpose of having the clean plugin interface separating the two.

@clockworkgr
Copy link
Collaborator Author

Suggestion: if you need to access chain.Chain in the plugin, what about turning cmd.newChainWithHomeFlags() public, so your plugin can invoke it, like many other ignite commands ?
EDIT: or even simpler, have a look at the main.go of a scaffolded plugin, there's a getChain() function, that returns exactly the chain.Chain struct.

The main idea is to reduce plugin dependencies to the CLI project as much as possible so people don't have to figure out too much about CLI internals.

I know about the getChain method but assume someone wants to get some more complex data and it gets complicated quickly.

Also, importing CLI pkgs to get data kinda defeats the purpose of having the clean plugin interface separating the two.

Also, passing data instead of relying on pkg imports allows non-golang plugins to have more capabilities

@tbruyelle
Copy link
Contributor

tbruyelle commented May 10, 2023

The main idea is to reduce plugin dependencies to the CLI project as much as possible so people don't have to figure out too much about CLI internals.

A plugin already depends on CLI, because it needs to register the structs used in the RPC calls. So at least we don't add new dependency.

I know about the getChain method but assume someone wants to get some more complex data and it gets complicated quickly.

What other info ? if it's complex data, good luck for serializing them.

Also, importing CLI pkgs to get data kinda defeats the purpose of having the clean plugin interface separating the two.

I understand the purpose, but giving the difficulty I had to serialize complex struct like pflag.FlagSet, I don't feel it's the right path to let a plugin author deal with that, each time he needs more data.

Also it doesn't me hurt that a plugin tool depends on that tool.

@clockworkgr
Copy link
Collaborator Author

The main idea is to reduce plugin dependencies to the CLI project as much as possible so people don't have to figure out too much about CLI internals.

A plugin already depends on CLI, because it needs to register the structs used in the RPC calls. So at least we don't add new dependency.

not referring to reducing dependencies in terms of package numbers but in terms of understanding them. E.g. the module list (where this requirement originated) means you need to figure out cosmosgen and cosmosanalysis before replicating a bunch of code .

I know about the getChain method but assume someone wants to get some more complex data and it gets complicated quickly.

What other info ? if it's complex data, good luck for serializing them.

complex in terms of acquiring ...not necessarily in terms of structure. again...see above: Module list is just a bunch of package names and protoDir tuples in the end....Obtaining them is not straightforward however.

Also, importing CLI pkgs to get data kinda defeats the purpose of having the clean plugin interface separating the two.

I understand the purpose, but giving the difficulty I had to serialize complex struct like pflag.FlagSet, I don't feel it's the right path to let a plugin author deal with that, each time he needs more data.

Also it doesn't me hurt that a plugin tool depends on that tool.

the idea is that we provide a bunch of data ourselves in the first place based on a few toggle flags (like the WithPaths above) so processing is only done if required... Expanding as we go.

@tbruyelle
Copy link
Contributor

OK I think I see your point. We could also provide more functions inside the plugin main.go, just like we have getChain() that can be used to get the app path, we could add getModuleList(). So the user doesn't need to understand the cosmosanalysis package, he just calls that function.

This is again to bypass the hassle of serialization (even if I agree with you for the Module type it shouldn't be a problem), but also the hassle of changing the plugin interface (you will see this has several impacts in the different implementations).

@clockworkgr
Copy link
Collaborator Author

OK I think I see your point. We could also provide more functions inside the plugin main.go, just like we have getChain() that can be used to get the app path, we could add getModuleList(). So the user doesn't need to understand the cosmosanalysis package, he just calls that function.

This is again to bypass the hassle of serialization (even if I agree with you for the Module type it shouldn't be a problem), but also the hassle of changing the plugin interface (you will see this has several impacts in the different implementations).

This solves half the issue I guess...

Depends on how strongly we feel about non-golang plugin implementations.

@tbruyelle
Copy link
Contributor

Depends on how strongly we feel about non-golang plugin implementations.

Good point. That said for the moment it's not even possible to build a non-golang plugin, ignite/apps#58 must be solved first.

@@ -0,0 +1,255 @@
package common
Copy link
Member

Choose a reason for hiding this comment

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

The package name should be more specific, maybe it could be named as codeanalisys, appanalisys or codeinspect. Its purpose could be to provide a simpler or unified API to inspect/analize blockchain apps using the other specific analysis libraries within pkg.

It would be ideal to have it very well documented if the point is to make it easier for plugin writers to understand how to inspect the app code.

Comment on lines +71 to +75
/* Commented out to include indirect dependencies as proto files may import from them
if req.Indirect {
continue
}
*/
Copy link
Member

Choose a reason for hiding this comment

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

Right now I'm not sure if this would create an issue with the current implementation so maybe in this case I would keep the existing behaviour as default to avoid having any side effect in the existing implementation by adding a boolean argument like for example:

func ResolveDependencies(f *modfile.File, includeIndirect bool) ([]module.Version, error)

Ideally we should confirm that the removal of the if res.Indirect condition won't have any side effects.

@Pantani Pantani closed this Nov 14, 2023
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.

4 participants