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

fix(ci): recycle the weekly spelling check issue #35653

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Aug 31, 2024

Recently, we implemented a weekly spelling check bot. It creates an issue every Monday to report typos and new words. We do not wish to flood the repo with such issues over a long period of time.

The PR updates the bot workflow to reuse the existing(35634) issue. The new code does so by updating the issue's opening comment and reopening the issue.

Testing

I tested the code in my fork.

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner August 31, 2024 02:48
@github-actions github-actions bot added system [PR only] Infrastructure and configuration for the project size/s [PR only] 6-50 LoC changed labels Aug 31, 2024
@Josh-Cena
Copy link
Member

If we do this we should be able to run this far more often—what about once per day? Or even once after each new commit?

@OnkarRuikar
Copy link
Contributor Author

If we do this, we should be able to run this far more often—what about once per day? Or even once after each new commit?

Once a day sounds good because content goes live at 24-hour frequency. So whether you fix typos after every commit or once a day is the same thing. If we could run this 2-3 hours before and fix typos before the prod deployment, that would be sufficient.

@Josh-Cena
Copy link
Member

I don't think we should assume anyone could get to typos at any time. Thinking about it, I think weekly is fine since we don't want to encourage people to send typo fixes every day.

@Josh-Cena Josh-Cena requested review from bsmth and removed request for a team September 2, 2024 03:49
@OnkarRuikar
Copy link
Contributor Author

Also, we hardly get 3-4 typos per week.

@Josh-Cena
Copy link
Member

I think in this PR:

  • We should in the issue body indicate the last time it was updated (this is also accessible via the edit history but it's less apparent, and also may not change if nothing had changed in the past week).
  • If the output is empty, we should update the issue to say "all clear" and also indicate the time.
  • We should probably parse the output and generate a Markdown list instead (as I suggested on Slack), but understood if you want to defer this to a future PR.

@OnkarRuikar
Copy link
Contributor Author

The current workflow would work like this:

  1. Wait till Monday.
  2. The bot runs.
  3. If no typos found do nothing, the issue remains closed.
  4. If typos found, update the issue and if it is closed reopen it.
  5. If someone submits a PR tagging the issue.
  6. After the PR is merged the issue automatically closes.

Note that there is no way to block the issue from being closed. If someone tags the issue in PR then the issue will be closed after the PR is merged.


Working on the timestamp and making files linkable in a list.

(this is also accessible via the edit history but it's less apparent, and also may not change if nothing had changed in the past week).
If the output is empty, we should update the issue to say "all clear" and also indicate the time.

We don't have to do this, as the closed issue will be the indicator of "all clear."

@Josh-Cena
Copy link
Member

Josh-Cena commented Sep 2, 2024

I don't think we should make people ever close that issue. Closing and reopening generates a lot of noise in the history and makes it barely scrollable. I think we should keep it like a dashboard. Think of something like the Renovate dependency dashboard.

@OnkarRuikar
Copy link
Contributor Author

OnkarRuikar commented Sep 2, 2024

I don't think we should make people ever close that issue.

If we run the workflow weekly, then how would you mark the issue as addressed? Also, autoclosing from PR is the best mechanism to tell other contributors that this has been fixed, so there is no need to work on it.

In a year we'll have less than 48 open-close logs under the issue. Next year we'll create a new issue to track this.

@Josh-Cena
Copy link
Member

I meant: the issue may be accidentally closed. That's fine. But we shouldn't include "close the issue every week" as part of the workflow.

then how would you mark the issue as addressed?

By having a PR called "20240831 typo fixes" that references the issue, so when you are on the issue you see what was the last typo fix PR and whether it's merged.

Next year we'll create a new issue to track this.

I think that's quite arbitrary and we'd rather keep the issue evergreen.

@OnkarRuikar
Copy link
Contributor Author

By having a PR called "20240831 typo fixes" that references the issue, so when you are on the issue you see what was the last typo fix PR and whether it's merged.

So you mean to use #12345678 but not fixes #12345678 in PR OP comment?
This is not foolproof, though. It burdens the reviewer to correct the PR title and remove linking to the issue. Also, by the time "20240831 typo fixes" gets merged a new contributor could submit "fix typos" PR.

@Josh-Cena
Copy link
Member

You have a point, but then IMO recycling the issue doesn't provide real benefits over new issues every week, if we are going to have this kind of useless history piling up anyway. The goal was to (a) most importantly, reduce our footprint generated on GitHub (b) change the labels (c) know what the latest status is. None of these really necessitate a recycled issue.

@OnkarRuikar OnkarRuikar marked this pull request as draft September 4, 2024 02:07
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Sep 4, 2024
@OnkarRuikar OnkarRuikar marked this pull request as ready for review September 4, 2024 08:08
@OnkarRuikar OnkarRuikar requested a review from a team as a code owner September 4, 2024 08:08
@OnkarRuikar
Copy link
Contributor Author

I've added the following two features:

scripts/linkify-logs.js Outdated Show resolved Hide resolved
@bsmth
Copy link
Member

bsmth commented Sep 6, 2024

IMO recycling the issue doesn't provide real benefits over new issues every week

I can see this being the case if the issue becomes very noisy. Aside from opening/closing, linking the same issue from PRs or other issues will also contribute to noise in the issue history.

That said, I'm not against trying this PR out and we can course-correct if we don't like it.

edit: Onkar's test issue is a good example of what it looks like with a lot of open/closing activity: OnkarRuikar#29

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on trying this out. There's a comment about line and colnumber in the logs, but it's not blocking.

@OnkarRuikar
Copy link
Contributor Author

I can see this being the case if the issue becomes very noisy. Aside from opening/closing, linking the same issue from PRs or other issues will also contribute to noise in the issue history.

We are interested in only the opening comment. In a year, we will get less than 12x4 open-close logs. We have way bigger PR pages with tons of review comments. At the beginning of the next year, we'll start fresh by using a new issue number. 😀

@bsmth bsmth merged commit 6b45b10 into mdn:main Sep 6, 2024
8 of 9 checks passed
@OnkarRuikar OnkarRuikar deleted the ci_recycle_issue branch September 6, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/m [PR only] 51-500 LoC changed system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants