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

Support reading GitHub token from env var #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tplass-ias
Copy link

@tplass-ias tplass-ias commented Jan 14, 2025

Proposed change

I would like to share my config.yaml and update scripts in a git repo, so I need a way to supply the GitHub token outside of config.yaml

This solution felt natural to me, but I would be willing to take a second pass utilizing a credentials file instead. (or any other preferred solution!)

Thanks for this great tool!

How to test the change

I tested manually by exporting the env var and successfully building a db.json with auto-pr pull

Checklist

  • Tests have been added to verify that the new code works (if possible)
  • Documentation has been updated to reflect changes
  • CHANGELOG.md has been updated to reflect changes

@tplass-ias tplass-ias requested a review from a team as a code owner January 14, 2025 19:53
I would like to share my config.yaml and update scripts in a git repo, so I need a way to supply the GitHub token outside of config.yaml

This solution felt natural to me, but I would be willing to take a second pass utilizing a credentials file instead.

Thanks for this great tool!
Copy link
Contributor

@daniddelrio daniddelrio 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 the contribution! If you can also add a test case for it in the /test directory, that would be amazing 🙏 Something like mocking the env var APR_API_KEY in the run_cli util function and testing if the functionality still works afterwards would work

@tplass-ias
Copy link
Author

tplass-ias commented Jan 15, 2025

hey @daniddelrio, I copy+modified test_create_files to work as an e2e test for using the env var for the api key. It already had a mock for create_github_client, which I used to validate the API key from the env var is used.

I had to adjust the feature implementation to play nice with pytest. Since the default value for the Credentials dataclass is set at import time, I needed to replace it with a function to return the value of the APR_API_KEY. This way the default value reflects the value of the env var during execution, and not just at script load time. I think this is inconsequential for actual usage, so I made the change.

Click's CliRunner's env injection doesn't seem to inject the env var at the right time, so I used pytest's monkeypatch feature instead.

Please let me know if you have any other feedback or suggestions :)

Copy link
Contributor

@daniddelrio daniddelrio left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again!

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