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

First stab at writing a central utils function for GitHub API calls. #1499

Merged
merged 13 commits into from
May 11, 2022

Conversation

ewels
Copy link
Member

@ewels ewels commented Mar 29, 2022

Uses authentication from local gh CLI or a supplied bearer token if found.

Done to address #1496

Bit scared it'll break something, so ideally needs extensive testing.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@codecov

This comment was marked as resolved.

@ewels
Copy link
Member Author

ewels commented Mar 30, 2022

Might be worth looking into doing API requests using a shared Session: https://docs.python-requests.org/en/master/user/advanced/

See also: https://requests-cache.readthedocs.io/en/stable/user_guide/general.html#sessions

Could then just define the auth once for the whole run, and might be faster when we're doing a lot of calls to the GitHub API.

@ewels ewels marked this pull request as ready for review April 1, 2022 09:05
@ewels ewels added the WIP Work in progress label Apr 1, 2022
@ewels ewels marked this pull request as draft April 1, 2022 12:06
Uses authentication from local gh CLI or a supplied bearer token if found.

Done to address nf-core#1496

Bit scared it'll break something, so ideally needs extensive testing.
Also removed function prefix from calls that were in the same file
nf_core/utils.py Show resolved Hide resolved
@ewels

This comment was marked as resolved.

@ewels ewels removed the WIP Work in progress label May 10, 2022
@ewels
Copy link
Member Author

ewels commented May 10, 2022

Might be worth looking into doing API requests using a shared Session

Leave this for another day I think. Especially as we may rewrite how the modules code works, so drop the number of GitHub API calls that we're doing anyway.

@ewels ewels marked this pull request as ready for review May 10, 2022 08:18
@maxulysse
Copy link
Member

to install locally
pip3 install --upgrade --force-reinstall git+https://github.com/ewels/nf-core-tools.git@actions-auth-api

maxulysse added a commit to maxulysse/nf-core_sarek that referenced this pull request May 10, 2022
@maxulysse
Copy link
Member

Works locally.
And on GHA:
https://github.com/nf-core/sarek/runs/6368068853?check_suite_focus=true

(lint fails, but we're not hitting limit of API calls anymore)

Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

<3

tests/test_sync.py Outdated Show resolved Hide resolved
tests/test_sync.py Outdated Show resolved Hide resolved
nf_core/sync.py Outdated Show resolved Hide resolved
ewels and others added 2 commits May 11, 2022 15:03
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
@ewels ewels merged commit be0993c into nf-core:dev May 11, 2022
@ewels ewels deleted the actions-auth-api branch May 11, 2022 17:41
@ewels
Copy link
Member Author

ewels commented May 11, 2022

Thank you for the help @mirpedrol!

This pull request was closed.
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.

4 participants