Skip to content

Conversation

@mrodm
Copy link
Contributor

@mrodm mrodm commented Aug 29, 2023

This PR reviews usage of positional arguments in all commands and sub-commands.
Currently, if a positional argument is set in the command line, this parameter is ignored. I think that could be misleading.

This PR enforces to not use positional arguments if they are not needed.
As an example:

  • Use of a positional argument where it is not used by the command:
 $ elastic-package clean aa
Clean used resources
Build resources removed: /home/mariorodriguez/Coding/work/integrations/build/packages/elastic_package_registry
Temporary service logs removed: /home/mariorodriguez/.elastic-package/tmp/service_logs
Done
  • With this PR, that would raise an error:
 $ elastic-package clean aa
Error: unknown command "aa" for "elastic-package clean"

In case of elastic-package test and elastic-package benchmark commands the same positional arguments has been kept. Keeping that would show a more specific error for those commands.

Example:

  • Current error shown:
 $ elastic-package test aaa
Run test suite for the package
Error: unsupported test type: aaa
  • If it is used cobra.NoArgs:
 $ elastic-package test aaa
Error: unknown command "aaa" for "elastic-package test"

@mrodm mrodm self-assigned this Aug 29, 2023
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @mrodm

@mrodm mrodm marked this pull request as ready for review August 29, 2023 10:36
@mrodm mrodm requested a review from a team August 29, 2023 10:36
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Good one! 🚀

@mrodm mrodm merged commit c69d5e1 into elastic:main Aug 29, 2023
@mrodm mrodm deleted the review_args_in_commands branch August 29, 2023 16:07
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.

3 participants