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

github_app_access_token: add support for private key fact #8989

Merged
merged 11 commits into from
Oct 21, 2024

Conversation

lewismiddleton
Copy link
Contributor

SUMMARY

Adds support for specifying the GitHub App private key via an ansible fact instead of a path to a file.

This is useful when you want to generate registration tokens for a remote host but don't want to put secrets on the host.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

github_app_access_token

ADDITIONAL INFORMATION

Adds support for specifying the GitHub App private key via an ansible
fact instead of a path to a file.

This is useful when you want to generate registration tokens for a
remote host but don't want to put secrets on the host.
@ansibullbot

This comment was marked as outdated.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress ci_verified Push fixes to PR branch to re-run CI feature This issue/PR relates to a feature request integration tests/integration lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit labels Oct 7, 2024
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Oct 7, 2024
@lewismiddleton lewismiddleton marked this pull request as ready for review October 7, 2024 11:13
@ansibullbot ansibullbot removed the WIP Work in progress label Oct 7, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch and removed backport-9 Automatically create a backport for the stable-9 branch labels Oct 7, 2024
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.

Hi @lewismiddleton thanks for your contribution!

I got only a couple of comments, the rest LGTM.

changelogs/fragments/8989-github-app-token-from-fact.yml Outdated Show resolved Hide resolved
Comment on lines 94 to 95
if private_key:
return jwk_from_pem(private_key.encode())
Copy link
Collaborator

Choose a reason for hiding this comment

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

If neither are passed by the user, they will get an error trying open the file None or something like that. I believe it would be better to have a validation check telling the user they must pass one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8e928bb

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.

Thanks for your contribution!

plugins/lookup/github_app_access_token.py Outdated Show resolved Hide resolved
plugins/lookup/github_app_access_token.py Show resolved Hide resolved
lewismiddleton and others added 2 commits October 14, 2024 10:28
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
plugins/lookup/github_app_access_token.py Outdated Show resolved Hide resolved
plugins/lookup/github_app_access_token.py Outdated Show resolved Hide resolved
plugins/lookup/github_app_access_token.py Show resolved Hide resolved
plugins/lookup/github_app_access_token.py Outdated Show resolved Hide resolved
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.

Besides that, the change looks good from my POV.

plugins/lookup/github_app_access_token.py Show resolved Hide resolved
Co-authored-by: Felix Fontein <felix@fontein.de>
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.

LGTM

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Oct 21, 2024
@felixfontein felixfontein merged commit 5b3b7a1 into ansible-collections:main Oct 21, 2024
126 checks passed
@felixfontein
Copy link
Collaborator

@lewismiddleton thanks for your contribution!
@russoz thanks for reviewing!

@lewismiddleton lewismiddleton deleted the github-app-token branch October 22, 2024 09:57
Massl123 pushed a commit to Massl123/community.general that referenced this pull request Feb 7, 2025
…llections#8989)

* github_app_access_token: add support for private key fact

Adds support for specifying the GitHub App private key via an ansible
fact instead of a path to a file.

This is useful when you want to generate registration tokens for a
remote host but don't want to put secrets on the host.

* Add license file

* Fix pep8 formatting

* Add changelog fragment

* Run sanity tests on changelog

* Apply suggestions from code review

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>

* Add input validation check

* Add import

* Apply suggestions from code review

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

* Add error for mutually exclusive options

* Update plugins/lookup/github_app_access_token.py

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

---------

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request integration tests/integration lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants