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

Allow for top-level EE denylist #207

Closed
pabelanger opened this issue Apr 20, 2021 · 3 comments · Fixed by #664
Closed

Allow for top-level EE denylist #207

pabelanger opened this issue Apr 20, 2021 · 3 comments · Fixed by #664
Labels
enhancement New feature or request question Further information is requested state:blocked This Issue or PR is blocked

Comments

@pabelanger
Copy link
Contributor

There are some cases, where we need to override the requirements.txt files and black file packages. There are various reasons for this.

In fact, we do this already internally for ansible-builder, however the list is hard-coded.

I propose we move this logic to an external file, and allow the execution-environment.yaml file to better control it.

@AlanCoding AlanCoding added the enhancement New feature or request label May 26, 2021
@AlanCoding
Copy link
Member

I propose we move this logic to an external file

I don't think that you're proposing that the default behavior will change. So some way or the other, we still have to ship the existing ignore list. Saying that it should be in a file, my best solution would be to package it as a data file:

https://packaging.python.org/guides/distributing-packages-using-setuptools/#data-files

Doing this exposes us to some hazards... but it's probably fine.

But we aren't using those old setuptools options, so it might look like this:

https://setuptools.readthedocs.io/en/latest/userguide/declarative_config.html

[options.data_files]
data = ansible_builder/data/ignore_python.txt

Talking about adding that to the setup.cfg.

Now, assume that we did this. It's not the main code path of ansible-builder build that uses this. It's ansible-builder introspect that would need this file. That's fine when it's using the default file. Since we have a full install in the ansible-builder image, it can figure it out without a problem. However, if the user customizes the file, then that's information outside of the container that we have to somehow get into the container. That would involve an ADD layer, and adding a new option to the introspect command.

All that said, I don't know if allowing this as a separate file is worth it compared to adding a list in the definition for include / exclude libraries.

This is one of those issues that definitely looks easy, but I think it has a lot of gotchas with it. We could easily mess it up.

@AlanCoding
Copy link
Member

After looking over #228 I'm majorly hesitating on this idea.

We need a better handle on what categories of requirements are involved. If it basically comes down to test requirements, then I want to know if people want to build an EE for testing or if they really want a test library in EEs with their collection.

The issue is a real problem, but this may be the wrong solution.

@pabelanger
Copy link
Contributor Author

bumping this up again as we are migrating to python39, from python38. Today, there is no way to denylist collections requirements. In this case, underlying layers have migrated to python39. We can pass in our own bindep.txt file with python39 references, however collections will be hard coded to python38.

The only option today, is ask a collection owner to release a new version with the updated references.

@Shrews Shrews linked a pull request Apr 12, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested state:blocked This Issue or PR is blocked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants