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

Rzfeeser patch 1 #523

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

rzfeeser
Copy link

@rzfeeser rzfeeser commented Dec 17, 2023

Description

Updated requirements.txt in the root of the repo to include entries and hashes for cryptography and pyopenssl. These are dependencies of panos-upgrade-assurance. Attempting to use the previous requirements file would fail as it would attempt to install cryptography and pyopenssl, and find no entry for these libraries or their hashes. In hash mode, pip is very clear that all installed packages must have hashes.

The file requirements.yml was also added. This file documents any and all Ansible collections this collection depends on. Even mention of itself is important for using Dockerfile or ansible-builder to create stable execution spaces.

Tested using a fresh install of Ubuntu 22.04 LTS, ansible [core 2.16.1], python version = 3.10.12, and jinja version = 3.0.3. Was able to successfuly run the panos_facts module against panos_VM.

Updated README.md to include instructions on how to use requirements.txt with pip.

Motivation and Context

Previously, attempting to use requirements.txt with pip would fail. For the purposes of a creating a stable Ansible environment for running playbooks, it is necessary to correctly document requirements.txt so that it may be used in conjunctions with Dockerfile or ansible-builder to create a container space for execution purposes.

The file requirements.yml was also added. This file documents any and all Ansible collections this collection depends on. Even mention of paloaltonetwork.panos is important for using Dockerfile or ansible-builder to create stable execution spaces.

Literature regarding proper documentation of dependencies in Ansible may be found at the following:
https://ansible.readthedocs.io/projects/builder/en/stable/collection_metadata/

How Has This Been Tested?

Tested using a fresh install of Ubuntu 22.04 LTS, ansible [core 2.16.1], python version = 3.10.12, and jinja version = 3.0.3. Was able to successfully run the panos_facts module against panos_VM.

The command ansible-test sanity --local is also still passing, however, no changes made by this commit would cause it to fail. This is mostly documentation and version control stability.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

best practice is to have a requirements.yml describing the collection(s) to be installed, in this case, just paloaltonetworks.panos
attempting to use the `requirements.txt` would fail as it did not have entries for `cryptography` and `pyopenssl`. pip is clear that in hash mode all installs must include hashes. As `panos-upgrade-assurance` has dependencies on these two files, they should be included.
Updating README.md to instruct how to utilize `requirements.txt`
@alperenkose
Copy link
Collaborator

@rzfeeser requirements.yml is to be used with playbooks/roles that need to use the collection. It's not related with this collection's dependencies. And we plan to update and maintain requirements.txt without hashes in a near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants