-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
monthly-check: improve username validation workflow #19151
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
base: main
Are you sure you want to change the base?
Conversation
| - name: Restore lychee cache | ||
| id: restore-cache | ||
| uses: actions/cache/restore@v4 | ||
| with: | ||
| path: .lycheecache | ||
| key: cache-lychee-${{ github.sha }} | ||
| restore-keys: cache-lychee- |
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 removed cache read/write steps because the default cache duration is 1 day while we're running this only once a month, so the cace would be expired anyway. Besides, we don't want to cache results, since indeed they may change between runs.
c17ad30 to
9f056fc
Compare
| id: last-issue # Step ID to reference later | ||
| with: | ||
| state: open | ||
| state: all |
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.
There's no need to only update an existing issue if it is open; so I changed this to search for closed ones too, and modified the last step in the workflow to reopen the closed issue if needed.
.github/workflows/monthly-check.yml
Outdated
| --no-progress | ||
| --max-concurrency 25 | ||
| usernames.txt | ||
| --output lychee_report.txt |
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 explicitly defined the output filename here, rather than let the action use the default one, since it's referenced further down in the workflow, and this way it is a bit clearer where it comes from.
| - name: Update last report open issue created | ||
| if: ${{ env.lychee_exit_code != 0 }} | ||
| - name: Create or update username validation issue if there are errors | ||
| if: steps.lychee.outcome == 'failure' |
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.
Used the standard outcome field of GitHub Actions steps rather than relying on an a custom environment variable whose definition is hidden (and which is not even being used by the lychee action anymore).
Refactor the monthly username validation workflow for improved clarity and maintainability. Also update lychee.md to the link to the CLI documentation page in the docs site.
9f056fc to
330d7c2
Compare
|
(I wanted to also rename the file from |
|
Btw, the current workflow doesn't seem to be working correctly: the last run did fail to load some of the userpages (though for reasons other than them actually being invalid), but skipped the issue update steps, for reasons that are not clear to me. Screenshot of the job log/output for future reference, since they will eventually be deleted:
|
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.
Looks cool, and definitely improves maintainability here! Just thought I'd drop some comments while I'm thinking about it haha
| workflow_dispatch: | ||
| schedule: | ||
| - cron: "0 0 1 * *" | ||
| - cron: "0 0 1 * *" # Run on the 1st of every month at midnight UTC |
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.
Offsetting this to a random value would be better practice, as if everyone does the same thing GitHub servers would be overwhelmed at the same time every month.
Hence perhaps do e.g. 1:24am or smth?
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 have a strong opinion either way. I'll wait for others to chime in.
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.
Looking at the last runs, it seems it is some kind of "random" already: https://github.com/tldr-pages/tldr/actions/workflows/monthly-check.yml?query=event%3Aschedule
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.
mmmm true
it's just the sysadmin in me speaking haha
either way this is just a minor thing really
Could you test if this PR fixed the issue? |
| - name: Extract GitHub usernames for validation | ||
| run: | | ||
| { | ||
| grep -oP '@[\w-]+' .github/CODEOWNERS |
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.
| grep -oP '@[\w-]+' .github/CODEOWNERS | |
| grep -oP '@[\w-]+' .github/CODEOWNERS |
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 did it that way to make it more obvious that both patterns are looking for the same thing, with the only difference being the ** prefix in the second one. Do you think it makes it more confusing than helpful?
Sure. How can I do that test? Maybe temporarily changing the triggering conditions of the workflow so it executes in this PR? |
We do have |
|
Thanks! That seems to have done the trick: https://github.com/tldr-pages/tldr/actions/runs/19003751429/job/54274077842 I think I might change the action to add a comment to the issue instead of modifying the existing opening comment, to make the historical progression easier to follow. |
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.
Cool!
(take my time suggestion or leave it, it's p minor)
I see we still have the issue about running into a 403 for 10 valid users. Are you able to fix that as well? |


Refactor the monthly username validation workflow for improved clarity and maintainability.
Also update lychee.md to the link to the CLI documentation page in the docs site.