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

fix(anta.cli)!: Avoid requiring username, password, inventory for the get commands #447

Merged
merged 53 commits into from
Dec 7, 2023

Conversation

gmuloc
Copy link
Collaborator

@gmuloc gmuloc commented Nov 9, 2023

Description

we have 2 ways of solving the problem:

  1. Moving the required arguments to the groups where they are actually needed (but then we change the CLI and where the parameters have to be put so breaking change ++)
  2. Doing the stuff I am doing in this PR -> replace the required with some custom Group / callbacks to relax the requirement depending on the command being called. It turns out to be more cumbersome as we need to inject the CLI arguments in the context to pass to the callback. An alternative is to write all the logic in the custom Group class which may allow us to keep the required=True on username and inventory (and so, see it in the help, though in the help well having required on some options that is ONLY SOMETIMES required option is meeeeh)

Fixes #355, #479 (duplicate)

Decision taken is to move the CLI option NOW before we release v1.0.0 to not live with our previous decisions.
This turned this PR in some heavy but very welcomed refactoring from @mtache

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have run pre-commit for code linting and typing (pre-commit run)
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (tox -e testenv)

@gmuloc gmuloc changed the title Fix(anta.cli): Avoid requiring username, password, inventory for the get commands fix(anta.cli): Avoid requiring username, password, inventory for the get commands Nov 9, 2023
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

anta/cli/utils.py Outdated Show resolved Hide resolved
@gmuloc gmuloc force-pushed the issue/355 branch 2 times, most recently from 8caa26e to be526d7 Compare November 24, 2023 14:50
@gmuloc gmuloc marked this pull request as ready for review November 24, 2023 15:17
@gmuloc
Copy link
Collaborator Author

gmuloc commented Nov 24, 2023

mtache and others added 7 commits December 4, 2023 14:56
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
anta/cli/utils.py Show resolved Hide resolved
anta/cli/utils.py Show resolved Hide resolved
anta/cli/utils.py Show resolved Hide resolved
anta/cli/utils.py Outdated Show resolved Hide resolved
@titom73 titom73 added the Anta CLI All things around CLI label Dec 4, 2023
Copy link
Collaborator Author

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

:shipit:

@mtache mtache merged commit 6d4f37f into aristanetworks:main Dec 7, 2023
19 checks passed
@titom73 titom73 mentioned this pull request Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Anta CLI All things around CLI rn: fix(anta.cli)!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some anta get commands do not need mandatory options in anta
3 participants