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

Display sidecar plugin in the collections list #1531

Merged
merged 9 commits into from
May 18, 2023

Conversation

shatakshiiii
Copy link
Contributor

@shatakshiiii shatakshiiii commented May 16, 2023

This PR adds the ability to display the available sidecar plugins (such as sidecar fiter/test plugins) while browsing any particular collection through :collections subcommand.

As an example, in line 22 we can see the sidecar filter plugin cp_label that is available inside the theforeman.foreman collection. And from there, get the available documentation of that plugin.

Screenshot from 2023-05-18 11-53-51

Related to #1522

@shatakshiiii shatakshiiii added the bug Researched, reproducible, committed to fix label May 16, 2023
@shatakshiiii shatakshiiii marked this pull request as ready for review May 18, 2023 07:25
@ssbarnea
Copy link
Member

@cidrblock We really need to change the way navigator tests are done. Checking the full output does not make the project easy to maintain at all. Instead we should only perform behavior testing, where instead of checking the exact output, to only check for presence of a specific element in its output.

Test fixtures should not need to be refreshed on each new version of Ansible core or collection. Look that number of changed files in this change...

@shatakshiiii Note that his comment is about the way we run tests, not this particular change. I know that you only followed the current pattern.

@cidrblock
Copy link
Collaborator

cidrblock commented May 18, 2023

@cidrblock We really need to change the way navigator tests are done. Checking the full output does not make the project easy to maintain at all. Instead we should only perform behavior testing, where instead of checking the exact output, to only check for presence of a specific element in its output.

Test fixtures should not need to be refreshed on each new version of Ansible core or collection. Look that number of changed files in this change...

@shatakshiiii Note that his comment is about the way we run tests, not this particular change. I know that you only followed the current pattern.

In many cases we do not compare against the entire fixture and only look for specific strings in the output. Each fixture has an explanation of what the tests is doing, looking for something present, absent or to compare the fixture completely. https://github.com/ansible/ansible-navigator/pull/1531/files#diff-807c6572f0fb84b5db71b3dc767bc17bc77aa316d6a746077435080f7dc050d3R12

If the entire fixture is present, but compare fixtures is false, it is only included so a reviewer can "see" what the screen looked like.

The challenge with navigator was there was little to go on for testing a curses app, where the visual presentation of the data is as important as the data itself.

For this PR, it's almost completely fine to review just the changes to python files, and look at take a quick look at the json file to ensure they don't have any errors in them.

Open to suggestions here, I still struggle to find a better way to do it.

Copy link
Collaborator

@cidrblock cidrblock left a comment

Choose a reason for hiding this comment

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

awesome

@shatakshiiii shatakshiiii merged commit d4f77f9 into ansible:main May 18, 2023
@shatakshiiii shatakshiiii deleted the show_sidecar_plugin branch May 18, 2023 12:21
@shatakshiiii shatakshiiii self-assigned this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Researched, reproducible, committed to fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants