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

proposal: simplify the plugin system #3930

Open
Tracked by #3927
tbruyelle opened this issue Jan 2, 2023 · 7 comments
Open
Tracked by #3927

proposal: simplify the plugin system #3930

tbruyelle opened this issue Jan 2, 2023 · 7 comments
Labels
component:apps Related to Ignite Apps. type:enh Improvement or update to existing feature(s).
Milestone

Comments

@tbruyelle
Copy link
Contributor

tbruyelle commented Jan 2, 2023

The plugin system is quite complex, especially because of the cobra.Command serialization, which needs a lot of boilerplate code because of pflags which is not serializable at all and uses a go interface to store the flag value (see here).

Like done in the cli-plugin-network (see here), we could only rely on os.Args and rebuild the cobra.Command in the plugin, instead of serialize it over the network. This only requires to truncate os.Args first item ignite, because in the plugin the command declared starts with network.
This also has the benefit of using standard cobra code in the plugin, instead of a switch statement on ExecutedCommand.Use.

But to achieve that we need to remove some features:

  • the ability to add a plugin command to a ignite sub command.
    It's currently possible to have plugin that adds a command to ignite scaffold, like ignite scaffold wasm. The simplification would allow to only add a command after ignite, because it's not possible to truncate os.Args properly in that case (imagine having to truncate ignite scaffold --path ./xxx/yyy wasm to wasm --path ./xxx/yyy, moreover you have to redeclare the --path flag in the plugin).

  • For the same reason ignite command should not have persistent flags at root (it currently doesn't have any).

  • the ability to access the command flags from a hooked command.
    Let's say a plugin adds a hook to ignite chain build, then inside the plugin ExecuteHook(Pre|Post|CleanUp) methods, it's possible to access the command flags via hook.ExecutedCommand.Flags(). If we remove the command serialization, the plugin would have only access to os.Args and so eventually repeat the hooked command declaration to read the flags.

My feeling on this is mixed, that's why I would like to collect opinions, please share yours !

@tbruyelle
Copy link
Contributor Author

tbruyelle commented Jan 2, 2023

Additional notes:

  • Since the serialization of cobra.Command is not complete, issues like Plugin system doesn't support hidden commands #3350 can raise on a regular basis. With the simplification this is no longer a problem.
  • The simplification will break existing plugins. Authors will have to update their plugin code to make it works with the CLI version that has the simplification.

@fadeev
Copy link
Contributor

fadeev commented Jan 5, 2023

I think this could be an interesting topic to discuss on the Community call 👍

@fadeev
Copy link
Contributor

fadeev commented Jan 5, 2023

Not being able to mount commands in places like ignite chain and ignite scaffold means there are limits to how plugins can really "extend" CLI from users' point of view. Each org extending the CLI would have to mount under their own top-level name.

ignite acme scaffold foo instead of ignite scaffold foo.

And it will make it harder for us to extract functionality into independent plugins as well.

@fadeev
Copy link
Contributor

fadeev commented Jan 5, 2023

Is it possible to enforce flags to be submitted after the command?

❌ ignite scaffold --path ./xxx/yyy wasm
✅ ignite scaffold wasm --path ./xxx/yyy

@tbruyelle
Copy link
Contributor Author

Is it possible to enforce flags to be submitted after the command?

❌ ignite scaffold --path ./xxx/yyy wasm
✅ ignite scaffold wasm --path ./xxx/yyy

Yes that's probably doable, and after we can safely truncate. I just wonder if this isn't going to be complicated if the command also has arguments.

That said, the plugin still needs to redefine the path flag internally to be able to read it, whereas this flag is defined in the parent comment scaffold.

@fadeev
Copy link
Contributor

fadeev commented Jan 5, 2023

But arguments will follow the command, right? You can't have ignite scaffold arg1 arg2 foo, it's always ignite scaffold foo arg1 arg2.

@tbruyelle
Copy link
Contributor Author

You're right, maybe it's not that complicated.

@salmad3 salmad3 transferred this issue from ignite/cli Feb 1, 2024
@salmad3 salmad3 transferred this issue from ignite/apps Feb 1, 2024
@salmad3 salmad3 added component:apps Related to Ignite Apps. type:enh Improvement or update to existing feature(s). labels Feb 2, 2024
@salmad3 salmad3 moved this from Backlog to Todo in Ignite CLI Masterboard Feb 2, 2024
@salmad3 salmad3 added this to the v29 milestone Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:apps Related to Ignite Apps. type:enh Improvement or update to existing feature(s).
Projects
Status: Todo
Development

No branches or pull requests

3 participants