-
Notifications
You must be signed in to change notification settings - Fork 97
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
Do not sanitize user python requirements #336
Conversation
Likely need an option to |
docs/collection_metadata.rst
Outdated
be excluded. For instance, these Python requirements will *always* be excluded | ||
since they should be a part of execution environment definition itself: | ||
|
||
- ansible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this depends on the version of ansible.
After 2.9, the ansible package isn't strictly necessary and this is reflected in the published images for ansible-runner, for example:
# for version in 2.12 2.11 2.10 2.9
> do
> podman run -it quay.io/ansible/ansible-runner:stable-${version}-devel pip freeze | grep ansible
> done
ansible-core @ file:///output/wheels/ansible_core-2.12.1.post0-py3-none-any.whl
ansible-runner @ file:///output/wheels/ansible_runner-2.1.2.dev2-py3-none-any.whl
ansible-core @ file:///output/wheels/ansible_core-2.11.7.post0-py3-none-any.whl
ansible-runner @ file:///output/wheels/ansible_runner-2.1.2.dev2-py3-none-any.whl
ansible-base @ file:///output/wheels/ansible_base-2.10.16.post0-py3-none-any.whl
ansible-runner @ file:///output/wheels/ansible_runner-2.1.1.dev14-py3-none-any.whl
ansible @ file:///output/wheels/ansible-2.9.27.post0-py3-none-any.whl
ansible-runner @ file:///output/wheels/ansible_runner-2.1.2.dev2-py3-none-any.whl
After 2.10, the ansible
package on pypi moved to a model where it only includes a set of collections.
My use case was to mirror the UX of installing 'ansible' from PyPI and then bumped into that issue (thanks for pointing me here)
I don't mind too much because there's simple workarounds like mentioned in that issue in addition to installing collections "manually" with a galaxy requirements.yml.
I could have missed it but it would be great to print a message that says a package was "sanitized" to avoid the effect of surprise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have missed it but it would be great to print a message that says a package was "sanitized" to avoid the effect of surprise.
I think that was attempted as a comment in the resultant requirements file, but with subsequent refactoring with the assemble script and other stuff, I think that file may wind up getting trashed, as more things were moved inside of ephemeral build stages.
logger.debug(f'# Excluding requirement {req.name} from {req.collections}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlanCoding would it make sense to bump the verbosity of that from debug to warning, perhaps ?
ansible_builder/requirements.py
Outdated
@@ -43,7 +54,7 @@ def sanitize_requirements(collection_py_reqs): | |||
# removal of unwanted packages | |||
sanitized = [] | |||
for req in consolidated: | |||
if req.name and req.name.lower() in EXCLUDE_REQUIREMENTS: | |||
if (req.name and (req.name.lower() in EXCLUDE_ALWAYS_REQUIREMENTS or (req.name.lower() in EXCLUDE_OPTIONAL_REQUIREMENTS and exclude))): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me put forward my alternative suggestion here - instead of documenting a new thing, we could avoid sanitizing anything from the user's requirements.
I find it hazardous to turn off sanitization of a collection's dependencies. I also don't see why we should ever sanitize the user's requirements. This would be much easier for documentation, probably just a mention in the --sanitize option help text.
To implement this, it might be adding something like and "user" not in req.collections
Just an idea. I'm always in favor of adding less configuration options if it will accomplish the same goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, I have to consider this some more, but I sort of like this solution at first read. I think we would still want to sanitize the user requirements to remove duplicates, but skip the part where we remove the excluded packages.
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, thanks for incorporating the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Since this is a bit of a behavior change, I don't think I'm going to backport this to |
Fixes #334
User Python requirements, specified via the
--user-pip
CLI option, will have duplicates removed, but will not go through the Python package exclusion process.