-
-
Couldn't load subscription status.
- Fork 4
feat: create update-readme.yml
#12
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
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.
If it's okay, could this PR include only a ## Sponsors heading for now, so we can verify the update-readme workflow is working correctly in the workflows repository?
Yes |
.github/workflows/update-readme.yml
Outdated
| node-version: "lts/*" | ||
|
|
||
| - name: Update README with latest sponsor data | ||
| run: | |
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 really don't like having this JavaScript in the YAML file. It makes it difficult to maintain and debug when something goes wrong. Can we move this into an external JavaScript file?
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've explored some alternatives and run several tests, but getting an external JavaScript file to work in a reusable workflow requires a lot of additional setup.
For example, the recommended way to run external JavaScript is to create a "JavaScript Action" following the GitHub guide: https://docs.github.com/en/actions/tutorials/create-actions/create-a-javascript-action (Most of the current Actions using JavaScript in the GitHub Actions Marketplace follow this approach.)
However, this approach requires quite a bit of setup. It requires installing npm packages such as @actions/core and @actions/github, and a bundler like Webpack or Rollup. I looked for alternatives, but a reusable JavaScript Action can't be implemented without these setups.
I think this approach matches what you mentioned in #6 (comment) and would add additional overhead for very little gain.
Another possible approach would be to avoid consolidating tools/commit-readme.sh and tools/update-readme.js into the workflow and instead keep them as external scripts that live in each repository as they do now. However, this would require every repository to include tools/commit-readme.sh and tools/update-readme.js. (This is what I was trying to avoid when suggesting #6.)
Yes, I agree that inline JavaScript makes debugging and maintenance harder, but the actual implementation is only about 20 lines, so given the overhead required to get external JavaScript working, inline JavaScript seems the simplest way to achieve the goal.
If debugging is the main concern, I can have the workflow log as much debugging information as possible. I think adding more logging to the workflow would help alleviate concerns if something goes wrong.
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 don't understand. Can't you just create a JavaScript file in this repository and have the workflow execute 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.
Can't you just create a JavaScript file in this repository and have the workflow execute it?
Adding a JavaScript file named tools/update-readme.js (or similar) to this eslint/workflows repository and updating the YAML script as shown below will not work in other repositories.
This is because, for example, if we run this reusable workflow in the eslint/json repository, the workflow runs in that repository. The default becomes eslint/json, not eslint/workflows.
So, if we check out the repository using actions/checkout@v5, only the eslint/json repository is cloned into the GitHub Actions runner. Therefore, the tools/update-readme.js file in the eslint/workflows repository is not accessible, since files from eslint/workflows are not cloned into the runner.
If inline JavaScript is indeed a problem (I'm not sure there's a way around it), I'll try to find a simpler way to implement this workflow. If I can't find any alternatives, I'll let you know in a comment.
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.
Could we have an external JavaScript file in this repo, and in the workflow use curl to download it and then execute 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.
I've updated the workflow to use curl in e7ab9a7.
I tested it on my personal repository and found that it works as expected:
8fe1fc8 to
e7ab9a7
Compare
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.
LGTM. leaving it open for @nzakas to re-review before merging.
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.
LGTM.
Prerequisites checklist
What is the purpose of this pull request?
Overview
In this PR, I created the
update-readme.ymlworkflow.I inlined and consolidated the logic that was originally in
tools/commit-readme.sh,tools/update-readme.js, and.github/workflows/update-readme.ymlin each repository.The original implementation used the
gotpackage to fetch data, but the workflow now runs on Node.js LTS and the nativefetchAPI is available, so thegotpackage is no longer needed.Support both multirepo and monorepo setups
This workflow works well for both multirepo and monorepo setups.
According to #4, 7 repositories use the
update-readmescript.There are two types of the
update-readmescript: one updates only the root-levelREADME.md(multirepo), and the other updates allREADME.mdfiles under thepackagesdirectory as well as the root-levelREADME.md(monorepo).I've taken these two types of
update-readmescript into account.Multirepo
Monorepo
Exception
eslint/eslintrepository is an exception, as it uses a slightly different form of theupdate-readmescript.How did you test this change?
I've tested the workflow in my personal repository ( https://github.com/lumirlumir/test-update-readme ) with my Personal Access Token and confirmed it works correctly for both multirepo and monorepo setups.
Multirepo test example
https://github.com/lumirlumir/test-update-readme/commit/c0663488a3e2b1b4a756f9cbe9961cc27e87c462
https://github.com/lumirlumir/test-update-readme/actions/runs/17358680977/job/49275514381
Monorepo test example
https://github.com/lumirlumir/test-update-readme/commit/c14102ea1063121e6d34b12ed17ad00a46ada303
https://github.com/lumirlumir/test-update-readme/actions/runs/17380613326/job/49337017695
What changes did you make? (Give an overview)
In this PR, I created the
update-readme.ymlworkflow.Related Issues
Ref: #4
Fixes: #6
Is there anything you'd like reviewers to focus on?
There is a prerequisite for this workflow to function as expected:
WORKFLOW_PUSH_BOT_TOKENsecret set at the organization level?