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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

lewismiddleton
Copy link

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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check-before-release PR will be looked at again shortly before release and merged if possible. 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