Rewrite documentation URL mapping to use the TOC #58
Conversation
ekohl
changed the title
There was a problem hiding this comment.
The CI jobs needs to be updated to reflect this
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
from
4b5b68a
to
d64fff5
Compare
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
2 times, most recently
from
104d3ab
to
dbd265f
Compare
There was a problem hiding this comment.
@ShimShtein this builds on your TOC work. The aim is to reduce maintenance burden: it makes PRs like #45 not needed, meaning there's a greater change it'll "just work".
| # An upstream chapter mapping, in case downstream another chapter should be | ||
| # used. The top level key is the upstream guide name where the value is a | ||
| # mapping of upstream chapter to downstream mapping | ||
| DOCS_GUIDE_CHAPTER_MAPPING = { |
There was a problem hiding this comment.
I think it's questionable if we really want this, but I kept it in since we had this.
| DOCS_GUIDE_CHAPTER_MAPPING.each do |guide, chapters| | ||
| chapters.keys.each do |chapter| | ||
| url = ForemanThemeSatellite::Documentation.docs_url(guide, chapter) | ||
| failed["/#{guide}/#{chapter}"] = url if url.include?('/html-single/') |
There was a problem hiding this comment.
Not sure if this reports correctly.
There was a problem hiding this comment.
It will fail if the chapter is not recognized. But what about non-recognized guide? Is it possible to have one?
There was a problem hiding this comment.
The html-single code path is the unrecognized guide. I admit it's very indirect. Note that it can only be reached if it's not found in DOCS_GUIDE_PAGES (which is the TOC).
There was a problem hiding this comment.
Let me rephrase: If I have a link without a chapter: just for the guide. It will not get into the if chapter block in the docs_url method, and will return <root>/non-existing-guide. Which is an error too, if I read the code correctly.
There was a problem hiding this comment.
There we assume guide.downcase returns the correct result, but you're right that we actually have a list of guides and can check. And I'll go through the list of current guides and add mappings
There was a problem hiding this comment.
I updated the mapping based on https://github.com/theforeman/foreman-documentation/blob/master/guides/upstream_filename_to_satellite_link.json but when verifying I found some mismatches: theforeman/foreman-documentation#3181
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
3 times, most recently
from
17f09d8
to
a257851
Compare
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
2 times, most recently
from
37095a9
to
52a5c42
Compare
There was a problem hiding this comment.
I like the idea of using the TOC, although it's used here only for validation.
Maybe we should extend the idea to have an upstream-to-downstream mapping file, that would be maintained automatically, so we will be sure the chapter actually exists.
The idea is to have downstream_toc.json <-> upstream_toc.json connection. The algorithm for automatic transformation would be the same as you have pointed out here plus the manual overrides. Then once one of the TOC files gets updated, we can make sure we still have valid connection.
The idea is to ensure each upstream TOC entry has an equivalent downstream: either by automatic guessing (like the algo you have suggested in this PR) or by manual override (with a dictionary). This is much easier that try to hunt down all references to documentation in the code.
WDYT?
| desc 'Validate documentation links against given TOC file' | ||
| task :validate_docs do | ||
| toc_file = ENV['TOC'] | ||
| unless toc_file | ||
| toc_file = File.expand_path('../../test/fixtures/toc.json', __dir__) | ||
| toc_file = File.expand_path('../foreman_theme_satellite/documentation-toc.json', __dir__) |
There was a problem hiding this comment.
This will require a change to the downstream CI
| DOCS_GUIDE_CHAPTER_MAPPING.each do |guide, chapters| | ||
| chapters.keys.each do |chapter| | ||
| url = ForemanThemeSatellite::Documentation.docs_url(guide, chapter) | ||
| failed["/#{guide}/#{chapter}"] = url if url.include?('/html-single/') |
There was a problem hiding this comment.
It will fail if the chapter is not recognized. But what about non-recognized guide? Is it possible to have one?
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
3 times, most recently
from
5408d68
to
6461f1b
Compare
| DOCS_GUIDE_CHAPTER_MAPPING.each do |guide, chapters| | ||
| chapters.keys.each do |chapter| | ||
| url = ForemanThemeSatellite::Documentation.docs_url(guide, chapter) | ||
| failed["/#{guide}/#{chapter}"] = url if url.include?('/html-single/') |
There was a problem hiding this comment.
Let me rephrase: If I have a link without a chapter: just for the guide. It will not get into the if chapter block in the docs_url method, and will return <root>/non-existing-guide. Which is an error too, if I read the code correctly.
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
from
6461f1b
to
f448e82
Compare
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
from
f448e82
to
c8e3850
Compare
| def test_docs_redirect_unknown_chapter | ||
| get "/links/docs/Managing_Hosts" | ||
|
|
||
| assert_redirected_to "https://docs.redhat.com/documentation/en-us/red_hat_satellite/#{ForemanThemeSatellite.documentation_version}/html/managing_hosts/" | ||
| end | ||
|
|
||
| def test_many |
There was a problem hiding this comment.
I'm not sure if we want to keep this, but it was a simple way for me to test if the old DOCS_GUIDES_LINKS were properly converted
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
from
c8e3850
to
0406fef
Compare
Upstream builds various guides and each guide is built as a single large page. That page has many chapters. Technically every guide is also built into 3 flavors (foreman-deb, foreman-el, katello). Downstream builds various guides, which mostly come from upstream. Most guides are a simple translation where they're downcased, but some are rewritten to something else. A bigger difference is that each guide is built into multiple pages. This means we need to find the right page containing the chapter. The Table of Content contains a complete mapping with this information, so it can be looked up dynamically. If this still can't be found, we use the html-single where everything is built into a single page. We then just hope it exists. Otherwise the browser will just show the whole page. This is an unexpected case, so a warning is logged. The result is that a lot less maintenance is needed, because in most cases it will "just work". A downside is that it becomes hard to verify links are correct. It becomes needed to crawl through all of the source code for any documentation links, similar to how we crawl through it for translations.
ekohl
force-pushed
the
use-documentation-toc-at-runtime
branch
from
0406fef
to
4dfad0b
Compare
Upstream builds various guides and each guide is built as a single large page. That page has many chapters. Technically every guide is also built into 3 flavors (foreman-deb, foreman-el, katello).
Downstream builds various guides, which mostly come from upstream. Most guides are a simple translation where they're downcased, but some are rewritten to something else.
A bigger difference is that each guide is built into multiple pages. This means we need to find the right page containing the chapter. The Table of Content contains a complete mapping with this information, so it can be looked up dynamically.
If this still can't be found, we use the html-single where everything is built into a single page. We then just hope it exists. Otherwise the browser will just show the whole page. This is an unexpected case, so a warning is logged.
The result is that a lot less maintenance is needed, because in most cases it will "just work". A downside is that it becomes hard to verify links are correct. It becomes needed to crawl through all of the source code for any documentation links, similar to how we crawl through it for translations.
Includes #59 and #60.