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

"yarn content delete" can break redirects #3146

Closed
ddbeck opened this issue Mar 5, 2021 · 1 comment · Fixed by #3147
Closed

"yarn content delete" can break redirects #3146

ddbeck opened this issue Mar 5, 2021 · 1 comment · Fixed by #3147

Comments

@ddbeck
Copy link
Contributor

ddbeck commented Mar 5, 2021

I accidentally broke CI by removing pages in mdn/content#2691, but it was only discovered in mdn/content#2866.

Three things came together to cause this failure:

  1. The README for mdn/content says "Don't edit the _redirects.txt file manually!" (emphasis in the original).
  2. yarn content delete doesn't make the corresponding modifications to _redirects.txt.
  3. The GitHub Action to run yarn content validate-redirects only runs when _redirects.txt is modified.

When I was preparing the original PR, I noticed that the pages I was removing was mentioned in _redirects.txt, but I didn't modify it (point 1). When I ran yarn content delete, it didn't modify the redirects file, so I thought this was intentional (points 1 + 2). Finally, I never knew I had caused a failing error, because validation never ran for my PR (point 3).

My impressions on this:

  1. yarn content delete should do the right thing and update _redirects.txt.
  2. If yarn content delete can't or won't do the right thing, then the content README needs to cover this caveat.
  3. Either way, if it's possible to break redirect validation by modifying files other than _redirects.txt, then it should run for every PR.

Originally posted in mdn/content#2866 (comment):

do you know why this is failing CI? It says (#2866 (checks)):

It’s due to the mglyph and mlabeledtr articles being removed in 44112ad/#2691 (so, heads-up to (heads-up @ddbeck) but some dangling redirects got left behind I don’t know if running yarn content delete causes those to get removed or not (so, heads-up to @mdn/core-yari-content too).

mdn/content#2871 fixes that problem. So once that PR merged, this PR can be rebased against main, and the CI failure should go away.

@fiji-flo
Copy link
Contributor

fiji-flo commented Mar 5, 2021

@ddbeck if you take a quick look at #3147 I'd merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants