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

op/command_mapper #480

Closed
wants to merge 3 commits into from
Closed

op/command_mapper #480

wants to merge 3 commits into from

Conversation

drcege
Copy link
Collaborator

@drcege drcege commented Nov 11, 2024

Added the prototype implementation of CommandMapper, which corresponds to the ScriptOP raised in issue #412. The following issues need discussion:

  • Avoid creating subclasses like PythonCodesOperator or BashCodesOperator, as this hinders extensibility. Consider that if users want to support more shell types like zsh, or integrate with other programming languages or third-party tools, they would have to add new classes at the code level, which is cumbersome and unnecessary. With a single CommandMapper, we can maximize support for arbitrary extensions without modifying the source code.

  • Operator naming. Currently, the operator is implemented as a mapper; therefore, the Mapper suffix is used instead of the original ScriptOP. Other candidate names include ScriptMapper, CodeMapper, ExternalXXXMapper, etc. I have opted for CommandMapper because it is not limited to just code or scripts; it can actually execute any command, such as pre-compiled binary files.

Everyone is welcome to provide feedback on the above issues, and I will add relevant documentation after reaching a consensus.

@yxdyc
Copy link
Collaborator

yxdyc commented Nov 12, 2024

Good prototype and thoughts on the PR. I concur with the first point regarding avoiding the creation of multiple subclasses for handling different shell environments, programming languages, or tools. This approach does indeed support greater extensibility and reduces code maintenance burdens.

However, I think that abstracting a ScriptOP base class can still be beneficial, especially for purposes related to security and fault tolerance. For example, a unified management interface could be used to restrict high-risk system commands like rm, or to consistently check command return statuses. Additionally, for improved efficiency and flexibility, we could consider maintaining two subclasses with the option for batched=False/True, allowing users to choose the most suitable approach for their needs. For instance:

CommandMapper (command: str = '', batched=False, repair=True)
CommandFilter (command: str = '', batched=False, repair=True)

@drcege
Copy link
Collaborator Author

drcege commented Nov 20, 2024

Moved to #492

@drcege drcege closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants