-
Notifications
You must be signed in to change notification settings - Fork 70
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
Prefer node16 action to support macOS and Windows runners #36
Conversation
66a4d09
to
213b3ec
Compare
Implemented the same functionality as fetch_github_asset.sh in index.ts and made this action node16 action. By this change, macOS and Windows runners on GitHub Actions will be supported.
Thanks for your contribution, it's really significant. I hadn't given any thought to supporting Windows and MacOS runners. That sounds great for sure! However, it does have significant implications as it changes the language of this repo from bash scripting to Typescript. I'm actually more familiar with TS than bash but the change needs to be reviewed a bit more closely as it's, essentially, a full rewrite. It may have different implications for other contributors. Please bear with me while I go through it. Given personal circumstances, it may take me over a week. |
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.
I just have 1 question really, but it looks pretty good.
I will do some refactoring later on and finally, add unit tests! Once it's merged.
"@typescript-eslint" | ||
], | ||
"rules": { | ||
"sort-keys": "error", |
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.
Once this is merged, I may make some changes to the eslint config and potentially add prettier.
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.
Please feel free, I don't have any strong opinions about that. These settings are just what I always use in my personal projects 😆
} | ||
|
||
const MAX_RETRY = 5 | ||
const RETRY_INTERVAL = 1000 |
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.
In the future we may want to replace the manual retry implementation with something like async-retry
.
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.
Great idea!
const file = core.getInput('file', { required: true }) | ||
const target = inputTarget === '' ? file : inputTarget | ||
|
||
const octokit = github.getOctokit(token) |
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 respects the defaulting behaviour here, right? https://github.com/dsaltares/fetch-gh-release-asset/pull/36/files#diff-fab4d7fb461bc6fbe9587f6c03fff98102b1c744145edcf2a993f2ff7cb05a0dR22.
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.
I intended to implement the same behavior as the bash script.
@umireon It does look like the integration test is failing, would you be able to look into it please? Thanks! |
@dsaltares It seems that the GitHub authorization mechanism is broken now. Please re-run the test later. |
The existing Docker action is also broken now. The problem should be in the GitHub infrastructure. |
@umireon GH's status page is green https://www.githubstatus.com and the error is related to API rate limiting. |
@dsaltares It hits the API limit due to the requests are not authorized against the GitHub API despite the correct Authorization header being present. |
@dsaltares I've created a support ticket to GitHub. Please wait for the response from GitHub support. |
Still no response from GitHub. I had created the ticket from the paid organization so the response would be soon. |
@dsaltares I talked with GitHub Support and managed to fix the problems. |
Great stuff, thanks @umireon! |
@guilhermeblanco apologies, can you open an issue for this? In the meantime, can you pin your pipeline to the latest release instead of |
Done. #38 |
Prefer node16 action to support macOS and Windows runners
Implemented the same functionality as fetch_github_asset.sh in index.ts and made this action node16 action.
By this change, macOS and Windows runners on GitHub Actions will be supported.