Skip to content

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Jul 8, 2019

Ran script from MS Docs Authoring Pack.

  • Deletes redirect files and places them in redirect.json
  • Manually replaced https://docs.microsoft.com/
  • Manually fixed relative redirect URLs that didn't resolve
  • Manually replaced old powerbi documentation urls

Ran script from MS Docs Authoring Pack.
- Deletes redirect files and places them in redirect.json
- Manually replaced https://docs.microsoft.com/
- Manually fixed relative redirect URLs that didn't resolve
- Manually replaced old powerbi documentation urls
@nschonni nschonni force-pushed the chore--Generate-master-redirect-file branch from 585c38d to 1a3e783 Compare July 8, 2019 07:22
@ktoliver
Copy link
Contributor

ktoliver commented Jul 8, 2019

@craigg-msft @MikeRayMSFT @rothja @MightyPen - Could you review the PR? If you accept the submission, PR reviewers can move the commit to the private repo for validation and staging. Thanks.

@ktoliver ktoliver added the aq-pr-triaged tracking label for the PR review team label Jul 8, 2019
@MightyPen
Copy link
Contributor

I will examine this public PR 2452.

@MightyPen
Copy link
Contributor

@nschonni Hi Nick, If we accept this PR 2452, what behavior do you expect for a case where a person attempts to browse directly to the following Https URL?:

Currently the system default to assuming the whole URL targets the file named 'index', as in:

Logically the system could continue this same default, and thereby continue by invoking the .op redirection entry for 'guide/index.md'. But I would need to see Test Results for this first, before Merging this big change into upstream master.
However, PRs created in repo= 'sql-docs' are not built in a test environment (nor built at all).
I could consider pulling this PR into my local area for repo= 'sql-docs-pr' (where we do have test building), but I am unsure how to pull across repositories.

I need to think about this PR 2452.
. .

@nschonni
Copy link
Contributor Author

nschonni commented Jul 9, 2019

I'm not sure if that is the function of the "redirect_document_id": false, maybe it needs to be true for the second to work.
For pulling it into the internal repo, I think @ktoliver can help with that, so it does get build on the review.docs.microsoft.com site.

@MightyPen
Copy link
Contributor

@nschonni Nick, The Diff on the .op redirection file is too big for me to view. I cannot see things like potential problems with redirect_document_id. If I approve this PR 2452, I might be doing so while too blind.

redirect_document_id : true means that the new target should start with all the accumulated pageViews and other data taken from the now-old source address. true causes errors when two or more source addresses are both redirected to the same one new address. In such cases, false is best.
For these specialty index.md files, only false should be used!

Overall, I cannot get comfortable approving this PR. My concerns include the Diff blindness problem.
Another is that I believe a small scale test of this PR idea would be prudent, of this expectation that .op redirection will work by the implicit default to a index.md. I would need to perform that small scale test.

So, I am rejecting and closing this PR 2452.
But I will ping you again after I have performed the small scale test.
Then you will have the option of creating a series of smaller PRs (with false).

Thanks again Nick. Gene.
If you want to generate a series of smaller PR, so that I might be able to eyeball the Diff, then I might approve and welcome this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aq-pr-triaged tracking label for the PR review team do-not-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants