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: ash_cli module #63

Merged
merged 7 commits into from
May 23, 2023
Merged

feat: ash_cli module #63

merged 7 commits into from
May 23, 2023

Conversation

leopaul36
Copy link
Contributor

Linked issues

Changes

  • Add ash.avalanche.ash_cli Ansible module

Additional comments

There is still room for improvements:

  • The module is currently "stateless", it either return ok or fatal. We could return changed once we merge the transaction feature to Ash CLI.
  • We could also parse the command to see if it's part of an allowed list but I'm not sure we want to do this for maintainability.

@leopaul36
Copy link
Contributor Author

Docs PR: AshAvalanche/ash-docs#42

Copy link
Contributor

@Nuttymoon Nuttymoon left a comment

Choose a reason for hiding this comment

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

A few remarks:

  • A role named ash_cli already exists. What about ash_cmd or ash_command for the module name? (can be both with a symlink)
  • I would appreciate having the command run as a string in the output. E.g. command: /usr/local/bin/ash avalanche node info
  • We should handle positional arguments: maybe with a positionals module argument
    positionals=dict(type="list", required=False, default=[]),

plugins/modules/ash_cli.py Outdated Show resolved Hide resolved
plugins/modules/ash_cli.py Outdated Show resolved Hide resolved
plugins/modules/ash_cli.py Outdated Show resolved Hide resolved
plugins/modules/ash_cli.py Outdated Show resolved Hide resolved
plugins/modules/ash_cli.py Outdated Show resolved Hide resolved
@Nuttymoon
Copy link
Contributor

Note that the module actually already supports positionals if we pass them in the command argument. Yet I don't really like this option as it makes it quite unreadable with multiple positionals. (Anyway people can still do as they please even if we add positionals)

@leopaul36
Copy link
Contributor Author

A few remarks:

* A role named `ash_cli` already exists. What about `ash_cmd` or `ash_command` for the module name? (can be both with a symlink)

* I would appreciate having the command run as a string in the output. E.g. `command: /usr/local/bin/ash avalanche node info`

* We should handle positional arguments: maybe with a `positionals` module argument
  ```python
  positionals=dict(type="list", required=False, default=[]),
  ```
  • command can now accept a list of strings (or just a single string which will be parsed as a one item list)
  • Module has been renamed from ash_cli to ash_cmd
  • command is passed in the output on both failure and success

Co-authored-by: Gauthier Leonard <gauthier@e36knots.com>
@leopaul36 leopaul36 merged commit 0b2283f into main May 23, 2023
@leopaul36 leopaul36 deleted the 62-ash-cli-module branch May 23, 2023 11:02
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.

Implement an Ash CLI module
2 participants