Skip to content

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 <jmeridth@gmail.com>
@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
Signed-off-by: Zack Koppert <zkoppert@github.com>
ignore_users,
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
@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.

3 participants