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.

Sign up for GitHub

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

Jump to bottom

Pluggable Cli Commands #1938

Merged
merged 5 commits into from
Oct 10, 2024
Merged

Pluggable Cli Commands #1938

merged 5 commits into from
Oct 10, 2024

Conversation

sh-rp
Copy link
Collaborator

@sh-rp sh-rp commented Oct 9, 2024 •
edited
Loading

Description

This PR adds a plugin interface for CLI commands and converts all the existing commands to plugins.

General Notes:

  • The implementation of the commands has not changes, I just collected all wrappers in one file and the plugins in another file. It somehow does not fully feel right, maybe these wrappers should be part of the plugin class.
  • We need to decide on the plugin interface. Is this good or do we want something else?

TODOs before merge:

  • Manually test all commands (already done, but not with all possible combinations)
  • Check that the deploy commands behaves correctly when dependencies are missing, this is the only thing that significantly changed.
  • Add a cli command to our test plugin

@netlify Netlify
Copy link

netlify bot commented Oct 9, 2024 •
edited
Loading

✅ Deploy Preview for dlt-hub-docs canceled.

Name Link
🔨 Latest commit 76ca368
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/6707e58b0bda420008031faf

sh-rp
sh-rp commented Oct 9, 2024
View reviewed changes
dlt/cli/_dlt.py Show resolved Hide resolved
sh-rp
sh-rp commented Oct 9, 2024
View reviewed changes
dlt/cli/_dlt.py
subparsers.add_parser("telemetry", help="Shows telemetry status")
# install available commands
installed_commands: Dict[str, SupportsCliCommand] = {}
for c in commands:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

each plugin gets instantiated and registered with the main parser, overwriting of existing commands is prevented. This approach is quite safe, but it will not allow to modify existing parsers or subparsers, this is to be discussed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to be able to overwrite init command with my own. this prevents that? IMO we should allow rewriting

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think it is implemented. IMO you should first deduplicate command list, keeping the last occurence. It is better not to add overwritten subparsers to the parser

sh-rp
sh-rp commented Oct 9, 2024
View reviewed changes
dlt/common/configuration/specs/pluggable_cli_command.py Outdated
import argparse


class SupportsCliCommand(Protocol):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cli command interface

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmm this should go to cli module, ie. to where you keep the hook definition. this is a place for injectable contexts

@sh-rp sh-rp requested a review from rudolfix October 9, 2024 12:55
rudolfix
rudolfix requested changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

please add a cli to dlt test plugin that adds commands and overwrites existing. tests are already set up.

should we have a module that imports all hooks? for the benefit of plugin makers that then can see all hooks are "external" protocols. we ourselves do not need to import it.

otherwise this is pretty good. let's do PR soon :)

dlt/cli/_dlt.py
subparsers.add_parser("telemetry", help="Shows telemetry status")
# install available commands
installed_commands: Dict[str, SupportsCliCommand] = {}
for c in commands:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to be able to overwrite init command with my own. this prevents that? IMO we should allow rewriting

dlt/common/configuration/specs/pluggable_cli_command.py Outdated
import argparse


class SupportsCliCommand(Protocol):
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmmm this should go to cli module, ie. to where you keep the hook definition. this is a place for injectable contexts

@sh-rp sh-rp force-pushed the feat/1670-pluggy-cli-plugins branch from 8375e76 to 88760b1 Compare October 9, 2024 16:40
@sh-rp
Copy link
Collaborator Author

sh-rp commented Oct 10, 2024

@rudolfix the requests are adressed. With regards to:
should we have a module that imports all hooks? for the benefit of plugin makers that then can see all hooks are "external" protocols. we ourselves do not need to import it.
Should we put it in dlt.plugins and decide on a naming scheme for these protocols? I think it would be nice to have one central place where you can import all the protocols from. let me know it would be an easy change.

@sh-rp
* move plugin interface
143b412 
* add tests for cli plugin discovery
* allow plugins to overwrite core cli commands
@sh-rp sh-rp force-pushed the feat/1670-pluggy-cli-plugins branch from f90a694 to 143b412 Compare October 10, 2024 13:53
rudolfix
rudolfix requested changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

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

this is really good! pls. drop the WIP

only thing to consider: deduplicate commands before adding them to parser.

dlt/cli/_dlt.py
subparsers.add_parser("telemetry", help="Shows telemetry status")
# install available commands
installed_commands: Dict[str, SupportsCliCommand] = {}
for c in commands:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think it is implemented. IMO you should first deduplicate command list, keeping the last occurence. It is better not to add overwritten subparsers to the parser

dlt/cli/_dlt.py
installed_commands: Dict[str, SupportsCliCommand] = {}
for c in commands:
command = c()
command_parser = subparsers.add_parser(command.command, help=command.help_string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

here... what if we overwrite command?

@sh-rp sh-rp changed the title WIP: Pluggable Cli Commands Pluggable Cli Commands Oct 10, 2024
@rudolfix rudolfix marked this pull request as ready for review October 10, 2024 14:27
@sh-rp sh-rp merged commit e9efa4f into devel Oct 10, 2024
58 of 61 checks passed
@sh-rp sh-rp deleted the feat/1670-pluggy-cli-plugins branch October 10, 2024 17:18
@rudolfix rudolfix mentioned this pull request Oct 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudolfix rudolfix rudolfix requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sh-rp @rudolfix