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

Add ansible-galaxy collection list command #65022

Merged
merged 41 commits into from Feb 14, 2020

Conversation

samdoran
Copy link
Contributor

@samdoran samdoran commented Nov 18, 2019

SUMMARY

Add ansible-galaxy collection list.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/cli/galaxy.py

lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

This looks good. I had some minor, mostly cosmetic comments, but the overall functionality is quite nice.

lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
lib/ansible/cli/galaxy.py Show resolved Hide resolved
@samdoran
Copy link
Contributor Author

samdoran commented Jan 7, 2020

The current output I have looks like this:

sdoran@mbp-2018 ~/S/ansible (galaxy-collection-list)> ansible-galaxy collection list
# /Users/sdoran/.ansible/collections/ansible_collections
Collection                                   Version
-------------------------------------------- -----------------
alikins.collection_inspect                   0.0.49
dynatrace_innovationlab.dynatrace_collection 1.0.6
mwhahaha.failure                             1.0.0
newswangerd.collection_demo                  1.0.10
pureport.pureport                            0.0.8
samdoran.hotpotato                           4.42.9.99.24.dev4
samdoran.icecubes                            2.7.15
samdoran.zztop                               3.14

sdoran@mbp-2018 ~/S/ansible (galaxy-collection-list)> ansible-galaxy role list
# /Users/sdoran/.ansible/roles
Role               Version
------------------ -------
geerlingguy.docker 2.6.0
samdoran.repo_epel 1.1
# /usr/local/share/ansible/roles
Role                Version
------------------- -------
ansible_pull        *
ansible_tower-bak   *
bootstrap_pi        *
caddy               *
certbot             *
deprole1            *
dnsmasq             *
docker              1.2.3
elasticsearch       *
fail2ban            *
firewall            *
fish                *
...

The column width is dynamic based on name and version length with a sane minimum.

I'm open to input or format or any other information that should be displayed for collections.

I do not know if changing the default output of ansible-galaxy role list is a good idea since there may be a lot of people parsing that output. I just wanted to at least have the option of making them unified, but that code may not ship with this change.

@samdoran
Copy link
Contributor Author

samdoran commented Jan 7, 2020

@geerlingguy Do you have any opinions on the output of collection list and/or role list?

@geerlingguy
Copy link
Contributor

@samdoran - I like it overall, though I think having one space before each of the paths (e.g. # /Users/sdoran/.ansible/roles) would be nice, because it's hard to visually parse the output with each path running onto the next. As someone who sometimes has 3 or more role paths configured, it could be messy if it's all one line after the other!

The only other value-add I can think of is—sometimes there is a role in more than one of the configured paths, it would be really cool if the output could show "this is the one that would be chosen" with an asterisk or something.

For collections, I haven't played around much with pathing, but isn't it possible you can have more than one path, too, and would have a similar conundrum?

@samdoran samdoran changed the title [WIP] Start adding ansible-galaxy collection list options [WIP] Add ansible-galaxy collection list command Jan 17, 2020
@samdoran
Copy link
Contributor Author

@geerlingguy Do you mean:

 # /Users/sdoran/.ansible/roles
Role               Version
------------------ -------
geerlingguy.docker 2.6.0
samdoran.repo_epel 1.1
 # /usr/local/share/ansible/roles
Role                Version
------------------- -------

Or a newline between each path? This is how I have it currently:

> ansible-galaxy collection list

# /Users/sdoran/.ansible/collections/ansible_collections
Collection                                   Version
-------------------------------------------- -----------------
alikins.collection_inspect                   0.0.49
dynatrace_innovationlab.dynatrace_collection 1.0.6
newswangerd.collection_demo                  1.0.10
samdoran.hotpotato                           4.42.9.99.24.dev4
samdoran.icecubes                            2.7.15
samdoran.zztop                               3.14

# /usr/local/share/ansible/collections/ansible_collections
Collection        Version
----------------- -------
mwhahaha.failure  1.0.0
pureport.pureport 0.0.8

I think adding a leading space makes it look wonky:

> ansible-galaxy collection list

 # /Users/sdoran/.ansible/collections/ansible_collections
Collection                                   Version
-------------------------------------------- -----------------
alikins.collection_inspect                   0.0.49
dynatrace_innovationlab.dynatrace_collection 1.0.6
newswangerd.collection_demo                  1.0.10
samdoran.hotpotato                           4.42.9.99.24.dev4
samdoran.icecubes                            2.7.15
samdoran.zztop                               3.14

 # /usr/local/share/ansible/collections/ansible_collections
Collection        Version
----------------- -------
mwhahaha.failure  1.0.0
pureport.pureport 0.0.8

@geerlingguy
Copy link
Contributor

How you have it currently, then; no leading space, just a space (newline, that is, not technically a 'space') between each of the paths:

> ansible-galaxy collection list

# /Users/sdoran/.ansible/collections/ansible_collections
Collection                                   Version
-------------------------------------------- -----------------
alikins.collection_inspect                   0.0.49

# /usr/local/share/ansible/collections/ansible_collections
Collection        Version
----------------- -------
mwhahaha.failure  1.0.0

@samdoran samdoran force-pushed the galaxy-collection-list branch 3 times, most recently from d37a0c5 to f6e85e9 Compare January 22, 2020 22:31
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jan 29, 2020
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

This looks really good. It seems like the integration tests are just failing due to the diff comparison with extra verbosity. Since the roles code has been refactored a bit there could be tests added to ensure backwards compatibility.

lib/ansible/cli/galaxy.py Show resolved Hide resolved
lib/ansible/cli/galaxy.py Outdated Show resolved Hide resolved
@AlanCoding
Copy link
Member

I'm still digesting this, so I just wanted to leave some tentative comments right now.


The behavior for directions that do not contain roles vs. collections doesn't seem really consistent. No roles will give a warning.

$ ansible-galaxy role list
[WARNING]: - the configured path /Users/alancoding/.ansible/roles does not exist.
[WARNING]: - the configured path /usr/share/ansible/roles does not exist.

But collections will give this error

$ ansible-galaxy collection list -p nothing
ERROR! Unexpected Exception, this is probably a bug: [Errno 2] No such file or directory: b'/Users/alancoding/Documents/repos/ansible/nothing/ansible_collections'
to see the full traceback, use -vvv

This seems particularly problematic because I might do

$ mv ~/.ansible/collections/ansible_collections/ ~/.ansible/collections/ansible_collections_backup
$ ansible-galaxy collection list
ERROR! Unexpected Exception, this is probably a bug: [Errno 2] No such file or directory: b'/Users/alancoding/.ansible/collections/ansible_collections'
to see the full traceback, use -vvv

That doesn't prevent me from inspecting a folder, but it does prevent me from inspecting it without an error.


Here's a demo of the situation @geerlingguy was referencing:

$ ansible-galaxy collection list -p sam

# /Users/alancoding/Documents/repos/ansible/sam/ansible_collections
Collection                 Version
-------------------------- -------
alikins.collection_inspect 0.0.49
awx.awx                    9.0.1 
[WARNING]: Collection at '/Users/alancoding/.ansible/collections/ansible_collections/testns/testcoll' does not have a MANIFEST.json file, cannot detect version.

# /Users/alancoding/.ansible/collections/ansible_collections
Collection             Version
---------------------- -------
alancoding.awx_awesome 0.0.1
ansible.tower          3.6.0
awx.awx                9.1.0
testns.testcoll        * 

There are 2 different versions of the awx.awx collection, and when using this collections path, the older version will be used. I don't know if it would be more helpful to call out which will be used, or call out which will not be used, because the former is kind of a more notable and uncommon situation that requires user attention.


There's also not a ton of information here, just versions. It might be good to have checksums or something eventually, and an option for JSON output. Right now it seems pretty strictly geared for manual use.

@sivel
Copy link
Member

sivel commented Jan 30, 2020

ERROR! Unexpected Exception, this is probably a bug: [Errno 2] No such file or directory: b'/Users/alancoding/.ansible/collections/ansible_collections'
to see the full traceback, use -vvv

Tracebacks are never expected. I'd consider this a bug.

As for the 2 other features, I cannot guarantee we can add them right now. For reference, the other new feature we are adding is verify and it will verify the collection that ansible will use, and give you the path.

@samdoran
Copy link
Contributor Author

@AlanCoding Thank you for the feedback. Much appreciated!

The behavior for directions that do not contain roles vs. collections doesn't seem really consistent. No roles will give a warning.

That's a bug. It shouldn't traceback.

There are 2 different versions of the awx.awx collection, and when using this collections path, the older version will be used. I don't know if it would be more helpful to call out which will be used, or call out which will not be used, because the former is kind of a more notable and uncommon situation that requires user attention.

This would be rather tricky to do well since the listing logic has absolutely no relation to collection finding logic. At best, I'd have to try and mimmic what the Ansible engine is doing that in collection list in order to indicate which would be used. I have no idea how easy/hard that would be to do.

If you run ansible-galaxy collection list specific.collection, it will list all instances of a collection in the search paths. So you at least get an indication there are multiple instances of a collection.

There's also not a ton of information here, just versions.

I started with the same information we provide for role list (though I took the liberty of improving the output). I wasn't sure what other information would be helpful though we have discussed adding checksum/signature information. @s-hertel had the actual checksum displayed in the output of validate but we weren't sure how useful it was to actually display that. A user mostly cares "did the collection change" not "what's the exact checksum".

It might be good to have checksums or something eventually, and an option for JSON output. Right now it seems pretty strictly geared for manual use.

The current output is meant for human consumption. For programmatic consumption, I would rather implement --json, which would alleviate a lot of the "What should I put in the output?" questions.

As @sivel mentioned, that's a stretch goal at this point. MVP is listing collection output for human consumption.

A side note on consistency between role list and collection list: I would like them to look and operate the same. I've elected to redesign the output formatting for collection list because there's no sense copying the current output of role list since it leaves a lot to be desired.

Once this PR is merged (and I have a good bit of work left to do), I have a separate branch that changes the role list output to match collection list. We can work on that later because that has its own list of issues to discuss.

@samdoran
Copy link
Contributor Author

The behavior for directions that do not contain roles vs. collections doesn't seem really consistent. No roles will give a warning.

One more comment on this one: I decided not to warn if default directories do not exist. collection list will only warn if a non-default path doesn't exist. I would like to change the role list behavior to not warn about missing default dirs, but that's for later.

@samdoran
Copy link
Contributor Author

@AlanCoding I fixed the bug you reported and added a test for that scenario.

@AlanCoding
Copy link
Member

Confirmed the traceback mentioned earlier is resolved now.

The only other point I have is about help text. This command inspects multiple paths - really all of the paths in the config for collection paths. That's sensible, I don't disagree. But the help text doesn't really help.

  -p COLLECTIONS_PATH, --collections-path COLLECTIONS_PATH
                        The path to the directory containing your collections.
                        The default is the first writable one configured via
                        COLLECTIONS_PATHS:
                        ~/.ansible/collections:/usr/share/ansible/collections

Wording of "the first writable one" is erroneous for this command. It also further supports the incorrect assumption that the location given for -p is the only one inspected.

$ ansible-galaxy collection list -p sam awx.awx

# /Users/alancoding/Documents/repos/ansible/sam
Collection Version
---------- -------
awx.awx 9.0.1

# /Users/alancoding/.ansible/collections
Collection Version
---------- -------
awx.awx 9.1.0   

This inspects the folder sam, and the other locations in the config. There's a very non-obvious difference in the request ansible-galaxy collection list -p sam versus ANSIBLE_COLLECTIONS_PATHS=sam ansible-galaxy collection list.

@samdoran
Copy link
Contributor Author

samdoran commented Feb 7, 2020

That's a very good point. The current behavior is it prepends the value passed to -p to the list of collections paths. The language "the first writeable one" is due to the option coming from common help rather than being install specific. That statement makes sense in the context of install but not list (or validate) for that matter.

So should the language be changed to more accurately describe what's happening, or should supplying -p behave the same as ANSIBLE_COLLECTIONS_PATHS=sam ansible-galaxy collection list?

Also, you found a bug in my display code with very short collection names. Thanks!

@samdoran
Copy link
Contributor Author

samdoran commented Feb 7, 2020

@AlanCoding I have changed the help tex for install -p to be a more accurate description of what it does. Let me know if this more accurate:

  -p COLLECTIONS_PATH, --collection-path COLLECTIONS_PATH
                        Additional path to search for collections.

The path may exist, but if there is no ansible_collections dir inside that path,
an exception was raised in find_existing_collections().

Add integration test for this scenario
Units tests for the various private methods in support of collection list
- Create fixture for creating test objects
- Add function for controlling os.path.isdir results
Ensure that collections with small FQCNs display correctly.
Add unit tests
Add more fixtures for mocking objects during the specific collection tests
Give accurate description of what it actually does rather than trying to use language shared between sub commands.
The temp file path is really long on macOS, so the warning message gets wrapped
across multiple lines. That make seth grep fail. Switch to matching on a smaller
part of the warning.
Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

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

just a few docs nits

docs/docsite/rst/galaxy/user_guide.rst Outdated Show resolved Hide resolved
docs/docsite/rst/user_guide/collections_using.rst Outdated Show resolved Hide resolved
docs/docsite/rst/user_guide/collections_using.rst Outdated Show resolved Hide resolved
docs/docsite/rst/user_guide/collections_using.rst Outdated Show resolved Hide resolved
Improve help about what the '-p' option does and how it works.
If someone specifies the same path via '-p' that is the COLLECTIONS_PATHS,
do not list the collections twice.
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 14, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 14, 2020
@sivel
Copy link
Member

sivel commented Feb 14, 2020

HAPPY MERGE DAY!

@sivel sivel merged commit f506fd4 into ansible:devel Feb 14, 2020
@samdoran samdoran deleted the galaxy-collection-list branch February 14, 2020 20:10
@ansible ansible locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants