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.

Sign up for GitHub

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

Jump to bottom

feat(bot): auto update links of moved files #31091

Merged
merged 2 commits into from
Dec 20, 2023
Merged

feat(bot): auto update links of moved files #31091

merged 2 commits into from
Dec 20, 2023

Conversation

OnkarRuikar
Copy link
Contributor

@OnkarRuikar OnkarRuikar commented Dec 18, 2023 •
edited
Loading

Every night the bot will create a PR to update all the links of moved files.

  • Note: A demo PR, fix: redirected links #31090, was created using the code and has already been merged. The PR has fixed all the previous redirect issues in the content.

@github-actions github-actions bot added the system [PR only] Infrastructure and configuration for the project label Dec 18, 2023
@Josh-Cena
Copy link
Member

Josh-Cena commented Dec 18, 2023 •
edited
Loading

Can this be part of the Yari CLI instead? It is my opinion that things that can be fixed locally should be fixed locally, instead of waiting for another cleanup PR. Again, PRs and commits are not zero-cost. We have formatting/linting daily fixes, because not everyone authors content locally, but everyone is and should be using the move CLI to move files.

@OnkarRuikar
Copy link
Contributor Author

Can this be part of the Yari CLI instead? It is my opinion that things that can be fixed locally should be fixed locally, instead of waiting for another cleanup PR. Again, PRs and commits are not zero-cost. We have formatting/linting daily fixes, because not everyone authors content locally, but everyone is and should be using the move CLI to move files.

  • I did submit a yari PR 9 months ago which does exactly what you are saying: feature(tool/move): update files pointing to the moved document(s) yari#8493 .
    Multiple review & merge requests were made. Multiple rebases and conflict resolutions also happened. The PRs came up in the last community meeting as well. 😡 In OWD meeting last week we decided to have the implementation as an auto cleanup bot on content side.
    If you could get the yari PR merged then it would be great. Meanwhile there is no harm in keeping this.

@Josh-Cena
Copy link
Member

Ugh, I keep forgetting how painful it is to get things merged in Yari. I feel you. No objection from me, then.

@OnkarRuikar OnkarRuikar force-pushed the update_moved_links_bot branch from da29871 to 053a598 Compare December 19, 2023 12:00
@OnkarRuikar OnkarRuikar marked this pull request as ready for review December 19, 2023 12:00
@OnkarRuikar OnkarRuikar requested review from a team as code owners December 19, 2023 12:00
@OnkarRuikar OnkarRuikar requested review from bsmth and removed request for a team December 19, 2023 12:00
@OnkarRuikar
Copy link
Contributor Author

@teoli2003 ping

teoli2003
teoli2003 suggested changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

One nit.

I think we should then try this, keeping in mind that once Yari's PR will be merged we will undo this. (But this is an annoying problem, so worth solving it in the meantime)

scripts/update-moved-file-links.js Outdated Show resolved Hide resolved
OnkarRuikar and others added 2 commits December 20, 2023 08:17
@OnkarRuikar OnkarRuikar force-pushed the update_moved_links_bot branch from a2225d7 to e7f947e Compare December 20, 2023 02:47
teoli2003
teoli2003 approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@teoli2003 teoli2003 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should go ahead with this:

  • this can't break mdn/content (and the Firefox release date was yesterday, so it can't be a problem there)
  • it is easy to revert in case of problem (the modified files aren't modified every day)
  • it is a band-aid

Let's merge this.

@teoli2003 teoli2003 merged commit 7eda00e into mdn:main Dec 20, 2023
8 of 9 checks passed
@OnkarRuikar OnkarRuikar deleted the update_moved_links_bot branch December 20, 2023 14:49
Tenkir pushed a commit to Tenkir/content that referenced this pull request Dec 22, 2023
@OnkarRuikar @teoli2003 @Tenkir
feat(bot): auto update links of moved files (mdn#31091)
e953084 
* feat(bot): auto update links of moved files

* Update scripts/update-moved-file-links.js

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

---------

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
@OnkarRuikar OnkarRuikar mentioned this pull request Dec 24, 2023
Merged
dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
@OnkarRuikar @teoli2003 @dipikabh
feat(bot): auto update links of moved files (mdn#31091)
696b097 
* feat(bot): auto update links of moved files

* Update scripts/update-moved-file-links.js

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>

---------

Co-authored-by: Jean-Yves Perrier <jypenator@gmail.com>
@caugner caugner mentioned this pull request Jun 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teoli2003 teoli2003 teoli2003 approved these changes

@bsmth bsmth Awaiting requested review from bsmth bsmth is a code owner automatically assigned from mdn/core-yari-content

Assignees
No one assigned
Labels
system [PR only] Infrastructure and configuration for the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OnkarRuikar @Josh-Cena @teoli2003