-
Notifications
You must be signed in to change notification settings - Fork 89
fix download-artifact script #1647
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 download-artifact script #1647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @dhchandw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request resolves an issue with the download-artifact script by ensuring that GitHub API calls are consistently authenticated using the provided token. It also adjusts the ESLint configuration to exclude a specific TypeScript file from linting, streamlining the development process.
Highlights
- Authentication Fixes: The
download-artifactscript has been updated to correctly pass thegithubTokenin the authorization headers for bothaxiosandOctokitrequests when interacting with the GitHub API. This addresses issues where the token might not have been properly utilized for authentication. - ESLint Configuration Update: The
.eslintrc.jsconfiguration has been modified to ignore thesrc-script/download-artifact.tsfile, preventing linting errors for this specific TypeScript file.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes an issue with downloading artifacts by correctly adding authentication tokens to GitHub API requests for both axios and octokit. The changes appear to correctly address unauthenticated API access issues. I have one suggestion to refactor the octokit authentication to be more idiomatic and to remove a potentially confusing code comment. The change to ignore the download script in the ESLint configuration is also appropriate for a standalone script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally having a file with a same name and a .js and .ts ending sounds like an absolutely terrible idea. Why are we doing this?
Will the TS not simply overwrite the JS once it transpiles?
47cbbe0 to
4d1cb78
Compare
|
@tecimovic deleted and git-ignoring the transpiled file. |
|
@dhchandw it says "fix download artifact script" but the script is deleted in your PR |
|
I see there are two of the same script? |
4d1cb78 to
c08d53a
Compare
@paulr34 The typescript file is the script that my PR is talking about, the javascript file you see is just the transpiled file. |
c08d53a to
e970564
Compare
| "scripts": { | ||
| "download-artifact": "npx ts-node src-script/download-artifact.ts", | ||
| "pkg:download-artifact": "npx pkg src-script/download-artifact.js -t node18-linux-x64,node18-linux-arm64,node18-macos-x64,node18-win-x64 --output dist/download-artifact --compress GZip", | ||
| "pkg:download-artifact": "npx pkg src-script/download-artifact.js -t node16-linux-x64,node16-linux-arm64,node16-macos-x64,node16-win-x64 --output dist/download-artifact --compress GZip", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the node versions being downgraded? we should be upgrading if anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packaging with node18 is slowing down the performance for this particular artifact, maybe because of the packages used. We are phasing this out so didn't bother to dig deeper into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sue what slowing down performance means here. Educate me later on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give Node 20 a try too here
| .clinic | ||
|
|
||
| # Compiled JavaScript output from TypeScript source | ||
| src-script/download-artifact.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will if somebody compiles the type script code, we don't want them to commit that.
| owner, | ||
| repo | ||
| repo, | ||
| headers: { authorization: `token ${githubToken}` } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sends the Github token as the Github API expects it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the api change with no backwards compatibility?
| const headers = options.githubToken | ||
| ? { Authorization: `token ${options.githubToken}` } | ||
| : undefined | ||
| let resp = await axios.get(url, { headers }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sends the Github token for the API if available, this call is made even if the Github token is not passed in (downloading from Artifactory)
| "scripts": { | ||
| "download-artifact": "npx ts-node src-script/download-artifact.ts", | ||
| "pkg:download-artifact": "npx pkg src-script/download-artifact.js -t node18-linux-x64,node18-linux-arm64,node18-macos-x64,node18-win-x64 --output dist/download-artifact --compress GZip", | ||
| "pkg:download-artifact": "npx pkg src-script/download-artifact.js -t node16-linux-x64,node16-linux-arm64,node16-macos-x64,node16-win-x64 --output dist/download-artifact --compress GZip", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give Node 20 a try too here
- package download-artifact using node 16 (better performance) - add brief comments to download-artifact.ts to satisy linting rules
e970564 to
a952afd
Compare
ZAPP-1662