ansible / ansible-builder

An Ansible execution environment builder
Other
289 stars 93 forks source link

introspect: add ability to specify requirements right in meta/execution-environment.yml #634

Closed Andersson007 closed 8 months ago

Andersson007 commented 9 months ago

In https://ansible.readthedocs.io/projects/builder/en/latest/collection_metadata/#collection-level-dependencies is written:

The meta/execution-environment.yml file containing the Python and/or bindep requirements
or referencing other files listing them. The .yaml extension is also valid on this file.

The containing the Python and/or bindep requirements part of the statement is not true. I tried and it throws a type error. It would be nice if it works as it's convenient and it would allow not to pollute collection repos with additional files. For the context, see https://forum.ansible.com/t/collection-inclusion-requirements-add-requirements-txt-and-bindep-txt/2611/8 (thanks @felixfontein for spotting this).

This PR allows to specify dependencies right in meta/execution-environments.yml. Tested locally and it works as expected.

NOTE: if you're OK with the idea and approve it in general, I'll try to fix the unit tests.

Andersson007 commented 9 months ago

ready for review (i'll fix unit tests but I need your approval first)

Shrews commented 9 months ago

After some internal discussion, maybe we can relax the strict schema validation suggestion a bit (though it would be nice to catch obvious format errors, I suppose).

Just curious, but how do you see collections dealing with using this new EE format but still trying to support folks stuck using an older version of ansible-builder that does not support it? Would collection authors just force folks to use older versions of the collection?

felixfontein commented 9 months ago

Two thoughts of mine:

  1. If this is added to ansible-builder now, I would wait a few years before using it anywhere, to make sure that most users (hopefully) have an ansible-builder version supporting this by then.
  2. I'm not sure what older ansible-builder versions say when trying to build an EE with such a collection. Do they ignore the dependencies, or do they fail? What's the error message?
  3. It might be a good idea to have versioning (similar to version: <int> in the EE definition), so that ansible-builder can give a nicer error message when encountering EE metadata it doesn't yet support. But even if this is added now, it again takes years so that most ansible-builder installations have this code included...
felixfontein commented 8 months ago

(Some strange definition of "two" I have...)

Andersson007 commented 8 months ago

@Shrews @felixfontein thanks a lot for the feedback!

Also worth mentioning is that the file naming execution-environment.yml and meta/execution-environment.yml can be confusing and their fields names kinda intersect which makes it even more confusing. That's why someone can expect them work similarly in terms of options to pass the values as path or list.

However, with that doc PR merged yesterday (thank you once more for reviewing) and taking your concerns into consideration, I think I'll just close the PR in order to keep things safe for users. I could open an issue instead, let me know. Thanks