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

hashi_vault collection - add support for python requirements discovery in ansible-builder #105

Merged
merged 3 commits into from
Jul 14, 2021

Conversation

erinn
Copy link
Contributor

@erinn erinn commented Jul 9, 2021

SUMMARY

Allows ansible-builder to automatically discover python library requirements for execution environments.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

None

ADDITIONAL INFORMATION

ansible-builder documentation for collections.. This work should not have any effect on anything else, but simply makes it easier to build execution environments for ansible tower/awx.

Allows ansible-builder to automatically discover python library requirements for execution environments.
@briantist briantist self-assigned this Jul 9, 2021
@briantist briantist added the enhancement New feature or request label Jul 9, 2021
@briantist
Copy link
Collaborator

Hello again @erinn ! I'm going to take a closer look into builder and other collections that publish a requirements.txt in the root, just to ensure I'm not missing any details, but it seems like a good idea. We'll likely want to reuse tests/integration/requirements.txt which properly limits the compatible versions of hvac based on python version.

Additionally, I need to update that as well re: hvac dropping support for python 3.5 in v1.0.0 (as announced just yesterday), but this PR doesn't need to wait for that.

I'd also like to see a changelog fragment for this, so that other builder/AWX users such as yourself will see it in release notes.

Thank you!

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #105 (1ce020f) into main (98dd60c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #105   +/-   ##
=======================================
  Coverage   83.52%   83.52%           
=======================================
  Files          16       16           
  Lines         892      892           
  Branches       87       87           
=======================================
  Hits          745      745           
  Misses        129      129           
  Partials       18       18           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98dd60c...1ce020f. Read the comment docs.

@erinn
Copy link
Contributor Author

erinn commented Jul 9, 2021

azure.azcollections is one example that uses the meta/execution-environments.yml file, vmware_rest is another and they use requirements.txt just for reference. I'll add a fragment.

I can't of course say for sure what is correct, but it seems to be working for some other folks.

@briantist
Copy link
Collaborator

0001-prepare-builder-requirements-CI-changelog.patch.txt

Hey @erinn thanks for your patience, I've looked into this a little bit, and I've made some changes. I was not able to push them into your branch (you may not have ticked the box to allow maintainers to make changes when you created the PR), so I'm including a .patch file you can use to apply the changes to your branch and push it. Let me know what you think.


Summary

  • I've created a meta/ansible-builder.yml file to explicitly point to a python requirements file (a new file, meta/ee-requirements.txt)
  • I originally wanted to point it directly at tests/integration/requirements.txt however I found that ansible-builder does not properly handle the ; python_version constraints, and instead just combines all of them together, which will not be good. So I opted to create a separate file for now that will need to be independently maintained, and it assumes that the python version is always 3.6 or higher.
  • I've added a new GitHub Actions workflow that attempts to validate that the requirements are at least being read properly.
  • I added a changelog fragment.

attachment contents:

From 0db74699dd0c2704ad177b7ff9c9a84608519556 Mon Sep 17 00:00:00 2001
From: Brian Scholer <1260690+briantist@users.noreply.github.com>
Date: Sun, 11 Jul 2021 13:39:30 -0400
Subject: [PATCH] prepare builder requirements, CI, changelog

---
 .github/workflows/ansible-builder.yml         | 40 +++++++++++++++++++
 .../fragments/105-support-ansible-builder.yml |  3 ++
 meta/ee-requirements.txt                      |  4 ++
 meta/execution-environment.yml                |  4 ++
 requirements.txt                              |  1 -
 5 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 .github/workflows/ansible-builder.yml
 create mode 100644 changelogs/fragments/105-support-ansible-builder.yml
 create mode 100644 meta/ee-requirements.txt
 create mode 100644 meta/execution-environment.yml
 delete mode 100644 requirements.txt

diff --git a/.github/workflows/ansible-builder.yml b/.github/workflows/ansible-builder.yml
new file mode 100644
index 0000000..5213ecf
--- /dev/null
+++ b/.github/workflows/ansible-builder.yml
@@ -0,0 +1,40 @@
+---
+name: ansible-builder
+on:
+  push:
+    paths:
+      - 'meta/execution-environment.yml'
+      - 'meta/ee-requirements.txt'
+  pull_request:
+    paths:
+      - 'meta/execution-environment.yml'
+      - 'meta/ee-requirements.txt'
+  schedule:
+    - cron: '0 13 * * *'
+
+env:
+  NAMESPACE: community
+  COLLECTION_NAME: hashi_vault
+
+jobs:
+  builder:
+    name: ansible-builder requirements
+    runs-on: ubuntu-latest
+    steps:
+      - name: Check out code
+        uses: actions/checkout@v2
+        with:
+          path: ansible_collections/${{ env.NAMESPACE }}/${{ env.COLLECTION_NAME }}
+
+      - name: Set up Python
+        uses: actions/setup-python@v2
+        with:
+          python-version: 3.9
+
+      - name: Install ansible-builder
+        run: pip install ansible-builder
+
+      # this is kind of a naive check, since we aren't comparing the output to anything to verify
+      # so the only we'll catch with this is an egregious error that causes builder to exit nonzero
+      - name: Verify Requirements
+        run: ansible-builder introspect --sanitize .
diff --git a/changelogs/fragments/105-support-ansible-builder.yml b/changelogs/fragments/105-support-ansible-builder.yml
new file mode 100644
index 0000000..431e4c8
--- /dev/null
+++ b/changelogs/fragments/105-support-ansible-builder.yml
@@ -0,0 +1,3 @@
+---
+minor_changes:
+  - hashi_vault collection - add ``execution-environment.yml`` and a python requirements file to better support ``ansible-builder`` (https://github.com/ansible-collections/community.hashi_vault/pull/105).
diff --git a/meta/ee-requirements.txt b/meta/ee-requirements.txt
new file mode 100644
index 0000000..a9d32a7
--- /dev/null
+++ b/meta/ee-requirements.txt
@@ -0,0 +1,4 @@
+# ansible-builder doesn't seem to properly handle "; python_version" type of constraints
+# requirements here are assuming python 3.6 or higher
+hvac >=0.10.6
+urllib3 >= 1.15
diff --git a/meta/execution-environment.yml b/meta/execution-environment.yml
new file mode 100644
index 0000000..c899493
--- /dev/null
+++ b/meta/execution-environment.yml
@@ -0,0 +1,4 @@
+---
+version: 1
+dependencies:
+  python: meta/ee-requirements.txt
diff --git a/requirements.txt b/requirements.txt
deleted file mode 100644
index 3e6a595..0000000
--- a/requirements.txt
+++ /dev/null
@@ -1 +0,0 @@
-hvac
-- 
2.31.1

@briantist briantist added this to the v1.3.2 milestone Jul 11, 2021
@erinn
Copy link
Contributor Author

erinn commented Jul 13, 2021

Hmm interesting, the option is checked to allow edits, so I am not sure what is going on there.

What you've got looks good, I can pull it in or we can try to figure out this 'Allow edits by maintainers' thing.

@briantist
Copy link
Collaborator

I most likely did something wrong then 😅 but yeah go ahead and pull it in whichever way is easiest. Thanks!

@briantist briantist changed the title Add requirements.txt hashi_vault collection - add support for python requirements discovery in ansible-builder Jul 13, 2021
@briantist
Copy link
Collaborator

heh confirmed it was me who did not push correctly 🤦 just added a small change to add in the AWS requirements after some feedback in IRC (thanks @tadeboro )

Comment on lines +1 to +7
# ansible-builder doesn't seem to properly handle "; python_version" type of constraints
# requirements here are assuming python 3.6 or higher
hvac >=0.10.6
urllib3 >= 1.15

boto3 # these are only needed if inferring AWS credentials or
botocore # using a boto profile; including for completeness

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why these requirements are not in the requirements.txt file at the collection's root? This way, we do not need the meta/execution-environment.yml file at all since the requirements.txt file is automatically used by ansible-builder as a source of dependencies.

Copy link
Collaborator

@briantist briantist Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tadeboro that's what @erinn had originally proposed; there currently isn't one in the root. I wanted to point at the one used by the integration/unit tests since it already existed, but I found that builder doesn't handle constraints correctly, like this:

hvac >= 0.10.6, != 0.10.12, != 0.10.13, < 1.0.0 ; python_version == '2.7' # bugs in 0.10.12 and 0.10.13 prevent it from working in Python 2
hvac >= 0.10.6 ; python_version >= '3.5'

Resulted in a single line like this:

hvac >=0.10.6, >= 0.10.6, != 0.10.12, != 0.10.13, < 1.0.0 

So I opted to make a separate one for this purpose for the time being that assumes py >= 3.6

edit to add: (I may revisit that in v2 of the collection when py2 & 3.5 will be dropped)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, EEs use Python 3.8, so those markers are usually not needed. But maybe builder should learn how to deal with them? Anyhow, this PR looks good and I think it is ready to go.

Copy link
Collaborator

@briantist briantist Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tadeboro so interestingly enough, I suspected it might be the case that some standard version or range of versions was used for EE's, and I tried to verify it but could not find it in the docs. If you happen to know where that is, I would really appreciate a link!

In any case, thanks again for reviewing.

@briantist
Copy link
Collaborator

@erinn thanks for bringing this to my attention, I'll be looking to ensure the collection is EE-ready and supported, please don't hesitate to raise any issues you see with that.

I have some other changes I'm planning to wrap up soon that will be released with this change in 1.3.2, so I hope to maybe release that by this weekend or so.

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

Successfully merging this pull request may close these issues.

None yet

3 participants