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

Add EE support #440

Merged
merged 6 commits into from
May 3, 2022
Merged

Add EE support #440

merged 6 commits into from
May 3, 2022

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Try to make this collection EE ready, with tests. Similar to ansible-collections/community.dns#93.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

collection

@felixfontein
Copy link
Contributor Author

I didn't expect the cryptography coming with whatever RHEL the EE is based on to be that old (Oct 2019) :)

@felixfontein
Copy link
Contributor Author

Ok, I think we have two choices:

  1. Install cryptography and PyOpenSSL from the system repos. These tend to be old (cryptography 2.8) and restricted (FIPS mode).
  2. Install cryptography and PyOpenSSL from PyPi. We get the latest versions, but we do not use the 'official' support stuff by RedHat.

What do you think?

@felixfontein
Copy link
Contributor Author

One advantage of using the system libraries is that you can still force-install the PyPi versions over these if you want.

@felixfontein felixfontein mentioned this pull request Apr 16, 2022
@felixfontein
Copy link
Contributor Author

After thinking about this for a longer time, I'd personally go with 1 - after all you can still explicitly force newer versions of cryptography and PyOpenSSL in the EE if you want to.

@felixfontein
Copy link
Contributor Author

I switched back to using system packages.

@felixfontein felixfontein changed the title [WIP] Add EE support Add EE support May 2, 2022
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

some small questions, but looks good to me!

Comment on lines +8 to +9
tests/ee/roles/smoke/library/smoke_ipaddress.py shebang
tests/ee/roles/smoke/library/smoke_pyyaml.py shebang
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you see my comment in a previous one of these?
I don't know the reason why (maybe it's because they are in an integration target? or a setup_ target specifically?), but I do not have any shebang errors on modules I created for tests:
https://github.com/ansible-collections/community.hashi_vault/tree/main/tests/integration/targets/setup_vault_test_plugins/library
(some have a shebang, I think most don't, they all work)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that this isn't in tests/integration/targets/.../library/ or plugins/modules/, but some other directory. The sanity check treats these directories specially: https://github.com/ansible/ansible/blob/devel/test/lib/ansible_test/_util/controller/sanity/code-smell/shebang.py#L84

name: Build and test EE (Ⓐ${{ matrix.runner_tag }})
strategy:
matrix:
runner_tag:
Copy link
Contributor

Choose a reason for hiding this comment

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

do they not have stable-2.13 yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't (https://quay.io/repository/ansible/ansible-runner?tab=tags&tag=latest). I guess it will show up once 2.13.0 has been released.

@felixfontein felixfontein merged commit 640bdbc into ansible-collections:main May 3, 2022
@felixfontein felixfontein deleted the ee branch May 3, 2022 17:23
@felixfontein
Copy link
Contributor Author

@briantist thanks a lot for reviewing this!

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