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

scripts/g.extension: make GitHub REST API server rate limit error message more verbose #2193

Conversation

tmszi
Copy link
Member

@tmszi tmszi commented Feb 12, 2022

Making GitHub REST API server rate limit error message more verbose with date time when rate limit will be reset.

Addon installation via g.extension module depend on make request/response to GitHub REST API server (which has limitation), to obtain addons paths (create addons_paths.json inside ~/.grass8/addons/ dir), required for addon path detection, for making addon source code URL and source code history URL, which appears on the man page. An additional request/response is required to obtain the last commit for the addon, which appears on the man page as commit and commit date. Fixes #2186 issue, when addons_paths.json doesn't exists and GitHub REST API server rate limit is already exceeded.

How to test this PR.

  1. Check if addons_paths.json file exists
GRASS nc_basic_spm_grass7/PERMANENT:~ > file .grass8/addons/addons_paths.json
.grass8/addons/addons_paths.json: JSON data
  1. Exceed GitHub REST API rate limit
tomas@gentoo-gnu-linux:~$ for i in {1..61}; do curl -s -o /dev/null -D - https://api.github.com/repos/OSGeo/grass-addons/git/trees/grass8?recursive=1 | grep "x-ratelimit-remaining" ; done

Scenarion A:

  1. Rename existed addons_paths.json file
tomas@gentoo-gnu-linux:~$ mv ~/.grass8/addons/addons_paths.json ~/.grass8/addons/addons_paths.json.bckp
GRASS nc_basic_spm_grass7/PERMANENT:~ > g.extension -j
ERROR: The download of the JSON file with add-ons paths from the GitHub
       REST API server wasn't successful, rate limit exceeded, 60 requests
       per hour per your computer IP address. You can try it again on
       Saturday Feb 12 14:57:22 2022.
GRASS nc_basic_spm_grass7/PERMANENT:~ > g.extension i.sentinel
ERROR: The download of the JSON file with add-ons paths from the GitHub
       REST API server wasn't successful, rate limit exceeded, 60 requests
       per hour per your computer IP address. The previous downloaded JSON
       file will not be used, because does not exists. You can try it again
       on Saturday Feb 12 14:57:22 2022.

Scenarion B:

  1. Rename back existed addons_paths.json file
tomas@gentoo-gnu-linux:~$ mv ~/.grass8/addons/addons_paths.json.bckp ~/.grass8/addons/addons_paths.json 
GRASS nc_basic_spm_grass7/PERMANENT:~ > g.extension -j
ERROR: The download of the JSON file with add-ons paths from the GitHub
         REST API server wasn't successful, rate limit exceeded, 60
         requests per hour per your computer IP address. You can try it
         again on Saturday Feb 12 14:57:22 2022.
GRASS nc_basic_spm_grass7/PERMANENT:~ > g.extension i.sentinel
WARNING: The download of the JSON file with add-ons paths from the GitHub
         REST API server wasn't successful, rate limit exceeded, 60
         requests per hour per your computer IP address. The previous
         downloaded JSON file will be used or you can try it again on
         Saturday Feb 12 14:57:22 2022.
Fetching <i.sentinel> from GRASS GIS Addons repository (be patient)...
Compiling...
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.coverage> addon html manual page or you can try
         it again on Saturday Feb 12 14:57:22 2022
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.download> addon html manual page or you can try
         it again on Saturday Feb 12 14:57:22 2022
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.import> addon html manual page or you can try
         it again on Saturday Feb 12 14:57:22 2022
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.mask> addon html manual page or you can try it
         again on Saturday Feb 12 14:57:22 2022
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.parallel.download> addon html manual page or
         you can try it again on Saturday Feb 12 14:57:22 2022
WARNING: The download of the commit from the GitHub REST API server wasn't
         successful, rate limit exceeded, 60 requests per hour per your
         computer IP address. Commit and commit date will not be included
         in the <i.sentinel.preproc> addon html manual page or you can try
         it again on Saturday Feb 12 14:57:22 2022
Installing...
Updating extensions metadata file...
Updating extension modules metadata file...
Installation of <i.sentinel> successfully finished

@tmszi tmszi added bug Something isn't working enhancement New feature or request backport_needed labels Feb 12, 2022
@tmszi tmszi requested review from ninsbl and neteler February 12, 2022 14:05
@tmszi tmszi added this to the 8.0.1 milestone Feb 13, 2022
@ninsbl
Copy link
Member

ninsbl commented Feb 14, 2022

If I understood comments by @lucadelu and @petrasovaa correctly, they are sceptical of using the API in general. And if use of the API impedes installation of AddOns (i.e. requires users to wait before they can install more addons) or leads to inconsistent behavior, then I fully agree that we should not use it and rather look for other solutions.

We have three relevant use cases as far as I can see:

  1. offline installation from local code in git repo
  2. offline installation from local code in tar file (without git features)
  3. installation with web access

And in all three cases we should make sure that the user does not hit the 60 requests limit or is not able to proceed because of the lack of internet connection.

So, your approach with generating a JSON file with commits and commit dates in a github action (#2140) seems like a good use of Github specific features that (similar to the unit tests) could be adjusted if we should decide to move e.g. from github to gitlab if we feel that is necessary one day. Unfortuntely, also download of artifacts is limited to 60 requests. So, maybe the JSON files could be published as GIST, like e.g. here: https://github.com/marketplace/actions/deploy-to-gist so we can circumvent the API?

The advantage with using git specific features here is that anybody with repo access can contribute to maintenance...

However, for ofline usage, the JSON file either needs to be part of the published code or not be required at all, so mkhtml should likely behave as follows:

  1. check for availability of a JSON file, if that is not the case
  2. try to get the log from a local git command, and if that does not work either
  3. leave the commit from the manual page and maybe add the date from the file statistics...

@tmszi
Copy link
Member Author

tmszi commented Feb 14, 2022

If I understood comments by @lucadelu and @petrasovaa correctly, they are sceptical of using the API in general. And if use of the API impedes installation of AddOns (i.e. requires users to wait before they can install more addons) or leads to inconsistent behavior, then I fully agree that we should not use it and rather look for other solutions.

In general, this is a good solution (using the remote REST API, if we have full control of it) given the options we have and this PR improves it. All request/response exceptions (HTTPError, URLError) associated with the GitHub REST API call are handled correctly, even during compilation (see explanation diagram).

Only marginal case remain problematic.

In the case of g.extension module, there are multiple places where the remote GitHub REST API is called, it's not just when generating a manual page, such as the source code URL, source code history URL, commit, and commit date.

Unfortuntely, also download of artifacts is limited to 60 requests. So, maybe the JSON files could be published as GIST, like e.g. here: https://github.com/marketplace/actions/deploy-to-gist so we can circumvent the API?

When we want make new release (source code without Git), person which make this release via GitHub page, first download pgms_with_last_commit.json file from GitHub workflow pgms-with-last-commit-patch-file artifact to him computer and than upload as attach into process of making GitHub GRASS GIS release. File will be part of archive e.g. https://github.com/OSGeo/grass/archive/refs/tags/8.0.1RC1.tar.gz.

However, for ofline usage, the JSON file either needs to be part of the published code or not be required at all, so mkhtml should likely behave as follows:

Yes it will, see my comment above.

@tmszi tmszi modified the milestones: 8.0.1, 8.2.0 Feb 15, 2022
@wenzeslaus wenzeslaus modified the milestones: 8.2.0, 8.2.1 Apr 26, 2022
@neteler neteler modified the milestones: 8.2.1, 8.2.2 Jan 22, 2023
@wenzeslaus wenzeslaus modified the milestones: 8.2.2, 8.3.1 Jun 6, 2023
@tmszi
Copy link
Member Author

tmszi commented Sep 27, 2023

Using GitHub REST API was replaced with Git program in major 8.3 version.

@tmszi tmszi closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem installing addons
4 participants