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 module pacman_key #778

Merged
merged 32 commits into from
Jun 4, 2021
Merged

add module pacman_key #778

merged 32 commits into from
Jun 4, 2021

Conversation

grawlinson
Copy link
Contributor

@grawlinson grawlinson commented Aug 15, 2020

SUMMARY

Manage pacman's keyring of trusted keys with pacman-key. Similar to the apt_key & rpm_key modules. Useful when adding additional repositories that have signed packages/databases.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

pacman_key

EDIT: This is originally from this PR on ansible/ansible.

@ansibullbot

This comment has been minimized.

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Please note that all new modules need to have tests. Since we do not have Arch Linux nodes in CI, unit tests are probably best in this case.

plugins/modules/packaging/os/pacman_key.py Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
@grawlinson
Copy link
Contributor Author

@felixfontein Thanks for the exceptional feedback! I'll implement your suggestions when I have the time.

@grawlinson
Copy link
Contributor Author

I'm not having any luck implementing unit tests due to this occurring.

$ ansible-test
ERROR: The current working directory must be at or below:

 - an Ansible collection: {...}/ansible_collections/{namespace}/{collection}/

The documentation simply states to run the tests in docker containers with ansible-test --docker. Is there something in the documentation that I'm missing?

@felixfontein
Copy link
Collaborator

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Oct 21, 2020
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@grawlinson thanks for the new module!
To have unit or integration tests would be good

plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

@grawlinson are you still working on this?

needs_info

@ansibullbot ansibullbot added needs_info This issue requires further information. Please answer any outstanding questions and removed community_review labels Feb 26, 2021
@grawlinson
Copy link
Contributor Author

Since the last few messages, I've got a new laptop so this hasn't been a pressing priority.

I'll hop on IRC and ask a few questions in the next week or so.

@ansibullbot ansibullbot added community_review and removed needs_info This issue requires further information. Please answer any outstanding questions labels Feb 26, 2021
@russoz
Copy link
Collaborator

russoz commented Apr 18, 2021

@grawlinson any updates on this?

@russoz
Copy link
Collaborator

russoz commented Apr 18, 2021

-label needs_triage

tests/unit/plugins/modules/packaging/os/test_pacman_key.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/packaging/os/test_pacman_key.py Outdated Show resolved Hide resolved
tests/unit/plugins/modules/packaging/os/test_pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

Please note that community.general will be released next Tuesday. Would be great if we can get this PR merged for that :)

a full length (40 characters) key ID is equivalent to the fingerprint.
@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 3, 2021
@grawlinson
Copy link
Contributor Author

I think module test coverage is at ~90% at the moment. Is there a threshold that new modules need?

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Jun 4, 2021
@russoz
Copy link
Collaborator

russoz commented Jun 4, 2021

I think module test coverage is at ~90% at the moment. Is there a threshold that new modules need?

Not that I am aware @grawlinson . (Maybe we should, but to establish any threshold without adding test coverage to all modules (many are at 0% today) wouldn't feel right, IMO).

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Looking good to me! Thanks @grawlinson for your contribution! 😁

@ansibullbot ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 4, 2021
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Something I missed before. Besides that, looks good!

plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman_key.py Outdated Show resolved Hide resolved
Co-authored-by: Felix Fontein <felix@fontein.de>
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 4, 2021
@felixfontein felixfontein merged commit 5ddf004 into ansible-collections:main Jun 4, 2021
@patchback
Copy link

patchback bot commented Jun 4, 2021

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/5ddf0041ecc733ed6f1f6ab938af584683c6e862/pr-778

Backported as #2704

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jun 4, 2021
* add module pacman_key

* add symlink and fix documentation for pacman_key

* documentation fix for pacman_key

* improve logic around user input

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Improve parameter checking

required_one_of=[] is neat.

Co-authored-by: Alexei Znamensky

* Revert "Improve parameter checking"

This reverts commit 044b0cb.

* Simplify a bunch of code.

* fix typos pointed out by yan12125

* replaced manual checks with required-if invocation

* added default keyring to documentation

* some initial tests

* updated metadata

* refactored to make sanity tests pass

* refactor to make sanity tests pass ... part deux

* refactor: simplify run_command invocations

* test: cover check-mode and some normal operation

* docs: fix grammatical errors

* rip out fingerprint code

a full length (40 characters) key ID is equivalent to the fingerprint.

* refactor tests, add a couple more

* test: added testcase for method: data

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* docs: correct yaml boolean type

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 5ddf004)
@felixfontein
Copy link
Collaborator

@grawlinson thanks a lot for your contribution!
@Andersson007 @russoz @yan12125 thanks for reviewing etc.!

felixfontein pushed a commit that referenced this pull request Jun 4, 2021
* add module pacman_key

* add symlink and fix documentation for pacman_key

* documentation fix for pacman_key

* improve logic around user input

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>

* Improve parameter checking

required_one_of=[] is neat.

Co-authored-by: Alexei Znamensky

* Revert "Improve parameter checking"

This reverts commit 044b0cb.

* Simplify a bunch of code.

* fix typos pointed out by yan12125

* replaced manual checks with required-if invocation

* added default keyring to documentation

* some initial tests

* updated metadata

* refactored to make sanity tests pass

* refactor to make sanity tests pass ... part deux

* refactor: simplify run_command invocations

* test: cover check-mode and some normal operation

* docs: fix grammatical errors

* rip out fingerprint code

a full length (40 characters) key ID is equivalent to the fingerprint.

* refactor tests, add a couple more

* test: added testcase for method: data

* Update plugins/modules/packaging/os/pacman_key.py

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

* docs: correct yaml boolean type

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 5ddf004)

Co-authored-by: George Rawlinson <george@rawlinson.net.nz>
@djmattyg007
Copy link

Wow, nice work and thank you for persevering <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI new_contributor Help guide this first time contributor new_module New module new_plugin New plugin os packaging plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants