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

ansible: Add missing plugins dependencies #28839

Closed
wants to merge 4 commits into from
Closed

ansible: Add missing plugins dependencies #28839

wants to merge 4 commits into from

Conversation

maxbrunet
Copy link

This PR adds 13 missing Python dependencies for Ansible plugins (code).
It will ensure each plugin has its dependencies satisfied after each upgrade.

Added dependencies:

Dependency Used By
credstash lookup/credstash
flatdict callback/logentries
hvac lookup/hashi_vault
junit-xml callback/junit
keyring lookup/keyring
pychef lookup/chef_databag
python-logstash callback/logstash
python-memcached cache/memcached
pexpect connection/winrm
pymongo cache/mongodb
redis cache/redis, lookup/redis
textfsm filter/network
xmpppy callback/jabber

Not added dependencies:

Dependency Used By Reason
lxc-python2 connection/lxc LXC doesn't run on MacOS
python-selinux lookup/filetree Only useful on Linux

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@ilovezfs
Copy link
Contributor

ilovezfs commented Jun 10, 2018

The dependencies reflect the list at the top https://github.com/maxbrunet/homebrew-core/blob/f29b69d7a9b79ca9edef9e4b9df73282d742709b/Formula/ansible.rb#L22-L41

So that list needs to be updated accordingly otherwise the ones you're wanting to add will just be removed automatically at the next upgrade.

@maxbrunet
Copy link
Author

@ilovezfs I updated my commit with your feedback. Thank you for looking into it.

will just be removed automatically at the next upgrade.

Out of curiosity, I found the Bottles doc and brew-test-bot.rb but I don't understand the role of the comment in the build. And some resources aren't in the list (e.g. jmespath).

Would you mind to explain me how the @BrewTestBot bottle build and the "Collect requirements from" comment work and/or point out where it's configured?

@stale
Copy link

stale bot commented Jul 2, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added stale No recent activity and removed stale No recent activity labels Jul 2, 2018
@maxbrunet
Copy link
Author

Hi there, I rebased my commit on master and went through the checklist again.
Are there any reasons why it is not merged yet or it does not get further feedback?

Please, don't hesitate to let me know how I can help to improve this contribution or the reasons why you will not move forward with it.

@albertomurillo
Copy link

@maxbrunet

Please add mysqlclient used by database/mysql_db

  resource "mysqlclient" do
    url "https://files.pythonhosted.org/packages/ec/fd/83329b9d3e14f7344d1cb31f128e6dbba70c5975c9e57896815dbb1988ad/mysqlclient-1.3.13.tar.gz"
    sha256 "ff8ee1be84215e6c30a746b728c41eb0701a46ca76e343af445b35ce6250644f"
  end

will need a depends_on "mariadb" => :build as well

@ronalexssen
Copy link

hvac would be super helpful as it enables the use of the hashi_vault ansible plugin.

@stale
Copy link

stale bot commented Aug 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added stale No recent activity and removed stale No recent activity labels Aug 10, 2018
@maxbrunet
Copy link
Author

Bump!

I managed to write a minimal Python script to find the missing dependencies that I can now share (I was using a terrible one-liner before):
https://gist.github.com/maxbrunet/397a94cee3267c4ee3d3d13162a042cc

Usage:

source "$(brew --cellar ansible)/$(brew info ansible --json=v1 | jq -r '.[].linked_keg')/libexec/bin/activate"
python find_missing_deps.py

Since ModuleFinder is not super accurate, there is a list of excluded dependencies that you can see in the code, let me know if something seems incorrect.

And I also found two I missed before:

Dependency Used By
openshift inventory/openshift|k8s, lookup/k8s|_openshift
salt connection/saltstack

@albertomurillo

mysql_db is a module, I prefer to keep this PR focused on the plugins.
You could probably open another one for it if you'd like.


@ronalexssen

yes! hvac was my initial objective, but I wanted to make all plugins usable for everyone.
(I should have probably kept it simple since nobody wants to merge it 🙁)

@javian
Copy link
Contributor

javian commented Aug 25, 2018

@BrewTestBot test this please

Copy link
Member

@DomT4 DomT4 left a comment

Choose a reason for hiding this comment

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

This needs a revision bump if we're adding bits onto it.

This formula can be an absolute demon to update as-is, but I guess I don't have a firm objection as long as I'm not the one having to update it 🙃.

@maxbrunet
Copy link
Author

Thank you @javian, @DomT4 for looking into this, I bumped the revision.

This formula can be an absolute demon to update as-is

I guess, you're talking about finding/updating the resources for the core.
Maybe you should log an issue somewhere to find a solution (if not already done).
I hope my changes don't make it worse (or not too much) since it's for the plugins. If one breaks, somebody will just do a PR to fix it.

@javian
Copy link
Contributor

javian commented Aug 30, 2018

12:54:27 Error: 1 problem in 1 formula detected
12:54:27 ansible:
12:54:27   * C: 9: col 3: `revision` (line 9) should be put before `head` (line 8)

@javian
Copy link
Contributor

javian commented Aug 30, 2018

We were discussing if your script to detect the plugins could be turned into a test that would then check for any future plugin changes and fail if there were missing plugins in the formula - do you think that would be feasible @maxbrunet ?

@stale
Copy link

stale bot commented Sep 20, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added stale No recent activity and removed stale No recent activity labels Sep 20, 2018
@maxbrunet
Copy link
Author

Hey @javian, sorry for the delayed response, I fixed the revision bump.

About turning the script into a test, unfortunately, ModuleFinder is not accurate enough, therefore such test would require a lot of maintenance:

  • Satisfying one dependency can add many false positives.
  • I made the wrong assumption that any import outside of __main__ (the plugin itself) was a false positive but I found a few other missing dependencies under the module_utils package of Ansible (google-auth, infoblox-client, ...).

An other approach could be to programmatically look into the documentation of each plugin:
https://github.com/ansible/ansible/blob/v2.6.4/lib/ansible/plugins/inventory/gcp_compute.py#L11-L13
But not all plugins are documented that way like filters and tests:
https://github.com/ansible/ansible/blob/v2.6.4/lib/ansible/plugins/filter/json_query.py

We could also bump this proposal ansible/proposals#87 or similar.

@claco
Copy link
Contributor

claco commented Oct 5, 2018

First, I applaud your work and contributing to Homebew Ansible.

However, 👎. Please no. I don't want all of these dependencies, like pychef, pymongo, etc. (missing pyrax?), that drag in even more dependencies. Satisfying all of the dependencies for all of the hundreds, and growing modules, is some real bloat.

If anything, the forumla should only install the dependencies that the official Ansible PPA packages install in other distributions and operating systems.

Can we at least make these optional, using the the --with-chef, --with-mongo type options?

@javian
Copy link
Contributor

javian commented Oct 5, 2018

@claco in general we try and avoid options and find a setup that fits many but not all. When you say bloat what do you refer to ? Storage space ? Performance ?

@claco
Copy link
Contributor

claco commented Oct 11, 2018

That's a good question. RIght now, it's just:

Required: libyaml ✔, openssl ✔, python@2

I would say only things required to make the ansible commands work (ansible, ansible-playbook, ansible-galaxy, etc), and only things for the core modules with non multiple vendor offerings(files, services, system, utilities).

Maybe anything that is only about OS functionality in windows and linux/osx, and nothing that is needed where there is wide variance of the same operation across multiple vendors (cloud, net, messaging, queueing).

Admittedly, it's a tough question to answer.

@fxcoudert
Copy link
Member

the formula should only install the dependencies that the official Ansible PPA packages install in other distributions and operating systems

👍 to that. Following upstream packaging, or other distributions, makes a lot of sense.

@SMillerDev
Copy link
Member

@maxbrunet could you make it the same as in the PPA then?

@maxbrunet maxbrunet mentioned this pull request Nov 18, 2018
6 tasks
@maxbrunet
Copy link
Author

@claco I do agree with you and I was thinking about closing this PR for a while.
However, the problem remains and I'd like the discussion to continue, so I opened #34267.

@maxbrunet maxbrunet closed this Nov 18, 2018
@lock lock bot added the outdated PR was locked due to age label Dec 18, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants