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

fix: use GitHub App token when authed with GitHub App #257

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

jmeridth
Copy link
Member

Pull Request

Proposed Changes

Currently we are still trying to use the GH_TOKEN env var when making api/graphql calls even after authenticating with a GitHub App Installation. This change fixes that.

Also fixed a few other things while in here:

  • split authentication to auth.py file (like other actions)
  • fix arguments to the count_comments_per_user function

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing

Reviewer

  • Label as either fix, documentation, enhancement, infrastructure, maintenance, or breaking

@jmeridth jmeridth self-assigned this Apr 29, 2024
@jmeridth jmeridth requested a review from zkoppert as a code owner April 29, 2024 02:37
@github-actions github-actions bot added the fix label Apr 29, 2024
Currently we are still trying to use the GH_TOKEN env var when making api/graphql calls even after authenticating with a GitHub App Installation. This change fixes that.

Also fixed a few other things while in here:

- [x] split authentication to auth.py file (like other actions)
- [x] fix arguments to the count_comments_per_user function
- [x] add `maintenance` to pull request template

Signed-off-by: jmeridth <[email protected]>
@jmeridth jmeridth force-pushed the jm-use-app-token-in-graphql-when-using-github-app branch from c031a96 to 1f04c74 Compare April 29, 2024 02:41
None,
None,
ignore_users,
Copy link
Member

Choose a reason for hiding this comment

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

uh oh. This should have been caught by a test. Added one at 79a3995

Copy link
Member Author

Choose a reason for hiding this comment

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

local linting caught it for me. mypy ftw. excellent idea to add tests. thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Is mypy disabled in our super-linter? Wondering how this status check passed so we could merge.

Copy link
Member

@zkoppert zkoppert left a comment

Choose a reason for hiding this comment

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

:shipit:

@jmeridth jmeridth merged commit fb94346 into main Apr 29, 2024
27 checks passed
@jmeridth jmeridth deleted the jm-use-app-token-in-graphql-when-using-github-app branch April 29, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants