ansible / awx-ee

An Ansible execution environment for AWX project
https://quay.io/ansible/awx-ee
Other
133 stars 156 forks source link

Add new python module (explicit) and ansible collections #92

Closed kladiv closed 1 year ago

kladiv commented 2 years ago

Hi @shanemcd. this PR should resolve #90 and #91. In addition:

kladiv commented 1 year ago

Hi @relrod, i added all those modules in requirements.txt 'cause i see that lots of collections are included in the requirements.yml of devel branch

I guess most of the included python modules are required by Ansible modules of those collections. E.g. for:

Without some python modules, there should be some Ansible collections modules won't work. Isn't it?

To keep AWX EE slim, maybe additional collections could be removed?

relrod commented 1 year ago

@kladiv necessary deps should automatically get installed with the collections. If there's something you've noticed missing, it's likely something that should be reported to the collections.

For example, the azure collection has https://github.com/ansible-collections/azure/blob/dev/meta/execution-environment.yml to point to its list of required python deps and ansible-builder should resolve/install these.

kladiv commented 1 year ago

Hi @relrod, i see not all collections included in the requirements.yml of devel branch use this method (meta/execution-environment.yml) so some deps are still missing.

I suggest you check internally if it's strictly required that all those collections should be already included in the base AWX EE or you can keep it slim removing some external collections.

To resolve #90 and #91 i changed this PR to add only:

For some reason PR is closed. Are you able to check and reopen it?

relrod commented 1 year ago

@kladiv First, thanks so much for your work on this!

We've not gotten any reports that I'm aware of about these collections being broken due to missing deps. Do you have a specific example or two of something that doesn't work from some collection we include in the current EE?

That file I linked is one way that ansible-builder can resolve dependencies. It can also look at a repo-root-level requirements.txt and bindep.txt :) So I think we are okay, but if you notice something missing/broken, please let us know!

To resolve https://github.com/ansible/awx-ee/issues/90 and https://github.com/ansible/awx-ee/issues/91 i changed this PR to add only:

netaddr google-cloud-storage

Researching a bit more -- the google.cloud collection has a requirements.txt file already, and I just checked and we actually do bring in google-cloud-storage already as a result:

[rick@fedora awx-ee]$ docker run -it --rm quay.io/ansible/awx-ee pip list | grep google-cloud-storage
google-cloud-storage               2.7.0

This might have been something the collection fixed since #91 was opened. But I don't think we actually need that change anymore.

So that leaves netaddr... I think we could do one of two things here.

Just adding netaddr on its own doesn't make too much sense, I don't think, because I don't think any collection we currently install actually makes use of it.

So then - we could discuss pulling in the ansible.utils collection (which would bring in netaddr as a dep). It seems like some people have been wanting to use ipaddr (#90 and comments therein) and next_nth_usable (ansible/awx#10370). These are both in ansible.utils.

There's also https://github.com/ansible/network-ee which already has ansible.utils, so we could just refer users to that EE. But I suspect it is useful to have these filters available in a more general setting like awx-ee here.

So to that end, I think my suggestion would be: Let's add ansible.utils to _build/requirements.yml -- and that would be the only change in this PR (so then revert the changes to _build/requirements.txt).

I think I would be inclined to approve that PR, because I think the things in ansible.utils are generally useful, and the size difference to the final image is pretty negligible (2.08GB up to 2.12GB in my quick testing).

I might bounce it off @shanemcd for a final +1 after that, just to make sure I'm not crazy, though :wink:

relrod commented 1 year ago

Hi @kladiv

Are you able to make the changes above? I'd like to get this PR out of the queue.

relrod commented 1 year ago

Going to close this for now, feel free to open a new PR with the requested changes above. Thanks!