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

Replace octonode with octokit #383

Closed
liammulh opened this issue Mar 5, 2023 · 8 comments
Closed

Replace octonode with octokit #383

liammulh opened this issue Mar 5, 2023 · 8 comments

Comments

@liammulh
Copy link
Member

liammulh commented Mar 5, 2023

In Rosetta 1.0, we were using a library called octonode for interacting with GitHub's API. It's possible that Rosetta 1.0 was written prior to the release of octokit, which is GitHub's official library for interacting with GitHub's API.

We continued using octonode in Rosetta 2.0 because it was easier than switching to something new. (We were already making a lot of changes in the transition from 1.0 to 2.0.)

I don't think this should be a high priority because octonode works (and if it ain't broke don't fix it), but there are a few advantages I see to using octokit instead of octonode:

  1. more popular, better support generally
  2. better IDE support (octonode has async methods, but Webstorm doesn't recognize them)
  3. better response objects (i.e. response objects that have the HTTP response status code, see long term storage of pl/energy-skate-park failed #376 (comment))
@liammulh
Copy link
Member Author

liammulh commented Apr 4, 2023

Octonode is now introducing moderate security vulnerabilities. npm audit yields:

# npm audit report

request  *
Severity: moderate
Server-Side Request Forgery in Request - https://github.com/advisories/GHSA-p8p7-x288-28g6
No fix available
node_modules/request
  octonode  *
  Depends on vulnerable versions of request
  node_modules/octonode

2 moderate severity vulnerabilities

Some issues need review, and may require choosing
a different dependency.

liammulh added a commit that referenced this issue Apr 7, 2023
Use Octokit instead of making Axios requests to
raw.githubusercontent.com.

See #383.
liammulh added a commit to phetsims/babel that referenced this issue Apr 10, 2023
Added as part of a test for phetsims/rosetta#383.
liammulh added a commit to phetsims/babel that referenced this issue Apr 10, 2023
Added as part of test for phetsims/rosetta#383.
liammulh added a commit that referenced this issue Apr 10, 2023
liammulh added a commit to phetsims/babel that referenced this issue Apr 10, 2023
liammulh added a commit to phetsims/babel that referenced this issue Apr 10, 2023
liammulh added a commit that referenced this issue Apr 10, 2023
@liammulh
Copy link
Member Author

In the above commits, I added a class LongTermStorage to interface with long-term storage, which is GitHub, as of this writing. This class has a get method and a store method. I made it so that Rosetta now uses these methods instead of issuing Axios requests to raw.githubusercontent.com.

We also get string files from GitHub that are in the sim repos. In the above commits, I added a module getStringFile that uses Octokit instead of issuing a request to raw.githubusercontent.com, except in one place: getEnglishStringKeysAndValues. It's kind of a special module in that the other modules that are getting the string file on the master branch, but the getEnglishStringKeysAndValues module needs a specific SHA.

Before we can do more testing, I need to use Octokit for getEnglishStringKeysAndValues. I need to refactor getStringFile to accept a ref as the second parameter, I think.

@liammulh
Copy link
Member Author

Now that we are using Octokit, the data we get back is what's currently on GitHub. (raw.githubusercontent.com caches requests for about five minutes.)

This means we can close #316, and we might be able to reload the page after a user submits a translation in order to fix #386.

@liammulh
Copy link
Member Author

I think we can also close #376 once this fix is deployed.

@liammulh
Copy link
Member Author

Don't forget to close #343 when this fix is deployed.

liammulh added a commit that referenced this issue Apr 11, 2023
Instead of issuing a request to raw.githubusercontent.com.

For #383.
@liammulh
Copy link
Member Author

Before we can do more testing, I need to use Octokit for getEnglishStringKeysAndValues. I need to refactor getStringFile to accept a ref as the second parameter, I think.

Done in 7a47a60.

liammulh added a commit that referenced this issue Apr 12, 2023
@liammulh
Copy link
Member Author

Deployed in 6021626 (2.0.7).

@liammulh
Copy link
Member Author

liammulh commented May 4, 2023

This is done. Closing.

@liammulh liammulh closed this as completed May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant