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

Add ability to generate TOC file for testing links #2665

Merged
merged 1 commit into from
May 27, 2024

Conversation

ShimShtein
Copy link
Member

This commit adds the ability to generate a TOC file that can be used to test documentation links.
This is done in the theme PR: RedHatSatellite/foreman_theme_satellite#31

Please cherry-pick my commits into:

  • Foreman 3.9/Katello 4.11 (planned Satellite 6.15)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4/6.5 on EL8 only)
  • Foreman 3.2/Katello 4.4 on EL7 & EL8
  • Foreman 3.1/Katello 4.3 on EL7 & EL8 (Satellite 6.11 EL7/8; orcharhino 6.3 on EL7/8)
  • We do not accept PRs for Foreman older than 3.1.

Copy link

github-actions bot commented Jan 11, 2024

The PR preview for f84f0e7 is available at theforeman-foreman-documentation-preview-pr-2665.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

guides/scripts/extract_links_toc.rb Outdated Show resolved Hide resolved
guides/scripts/extract_links_toc.rb Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/scripts/extract_links_toc.rb Outdated Show resolved Hide resolved
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Jan 17, 2024
@maximiliankolb
Copy link
Contributor

Thanks for your contribution. @ShimShtein Can you please update your PR?

@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Feb 20, 2024
@ShimShtein
Copy link
Member Author

ShimShtein commented Feb 20, 2024

Updated the PR, should be OK now.
@ekohl ,

What is this file for? It looks like it's converting files to Satellite file naming, but I don't understand the point. Wouldn't index-satellite.html be correct then?

This file translates book names into the first part of documentation link path. The translation is not trivial, so I can't actually use an algorithm to derive the book name from the link.

Gemfile Outdated Show resolved Hide resolved
guides/scripts/extract_links_toc Show resolved Hide resolved
@ShimShtein
Copy link
Member Author

@maximiliankolb ready for another round

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

Took me a little while to see that this only works for "foreman" as build target.

$ rm -fr guides/build && podman run --rm -v $(pwd):/foreman-documentation:Z tmp_review_2665 make toc
$ wc guides/build/toc.json
  931  1028 43851 guides/build/toc.json
$ cat guides/build/toc.json | jq -e
$ echo $?
0

I've built the container image based on your branch. I generally run rm -fr guides/build && podman run --rm -v $(pwd):/foreman-documentation:Z foreman_documentation make html BUILD=katello locally. With a build target, I get the following error message: scripts/extract_links_toc: warning: argument of top-level return is ignored and the JSON file is empty like {}.

two things: a) it would be nice to have a small section in guides/README.md how it works (maybe just copy my command?; maybe just say "run make toc"?) and why/when someone would want to do this.
b) it would be nice to make this work for all build targets, no?

I cannot comment on the ruby code tbh.

@ShimShtein
Copy link
Member Author

Added a paragraph into the README file. It actually explained why running build targets other than foreman resulted in an empty TOC. To get something in the TOC, we will need to modify the upstream_filename_to_link.json file.

@ShimShtein
Copy link
Member Author

@ekohl I think it's ready for another round.

Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

I've left you corrections for folder names of Satellite guides in 6.15.
They have to be precise, otherwise we would be getting 404s.

FYI The guide titles/folder names may and do differ between Satellite releases!

guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Needs re-review labels Apr 24, 2024
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Apr 25, 2024
@ShimShtein
Copy link
Member Author

@Lennonka Ready for another round

@ShimShtein ShimShtein force-pushed the link_checker branch 2 times, most recently from 896aca8 to 108eb9e Compare April 25, 2024 12:21
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

A couple of suggestions.
I cannot comment on the ruby code.
The rest LGTM.

guides/README.md Outdated Show resolved Hide resolved
guides/README.md Outdated Show resolved Hide resolved
guides/README.md Outdated Show resolved Hide resolved
guides/README.md Outdated Show resolved Hide resolved
guides/README.md Show resolved Hide resolved
@maximiliankolb
Copy link
Contributor

@ekohl Please have a look at the code.

@maximiliankolb maximiliankolb added the Needs tech review Requires a review from the technical perspective label May 16, 2024
Gemfile Outdated Show resolved Hide resolved
guides/common/Makefile Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
guides/upstream_filename_to_link.json Outdated Show resolved Hide resolved
@ShimShtein ShimShtein force-pushed the link_checker branch 3 times, most recently from 681a002 to eaf760d Compare May 22, 2024 13:21
@ShimShtein
Copy link
Member Author

@ekohl Changed the files.

I'd prefer to make it explicit that this if for Satellite.

Would you like to call it upstream_filename_to_satellite_link ?

@ekohl
Copy link
Member

ekohl commented May 22, 2024

Would you like to call it upstream_filename_to_satellite_link ?

Sounds good to me. Mostly raising it because we have 2 downstreams, so it's ambiguous. Satellite is explicit so 👍

guides/common/Makefile Outdated Show resolved Hide resolved
guides/scripts/extract_links_toc Outdated Show resolved Hide resolved
@ShimShtein
Copy link
Member Author

@ekohl renamed the file and left comments on your questions.

guides/Makefile Outdated
@@ -24,3 +24,6 @@ linkchecker-tryer:
find build -type f -name index\*html | xargs linkchecker -r1 -f common/linkchecker.ini --check-extern | ./scripts/linkchecker-tryer

ccutil: $(SUBDIRS)

toc: html
F2L=upstream_filename_to_satellite_link.json ruby scripts/extract_links_toc build/**/index-*.html > build/toc.json
Copy link
Member

Choose a reason for hiding this comment

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

Since you're already doing this for Satellite only, you can limit it to just index-satellite.html. And also use the shebang

Suggested change
F2L=upstream_filename_to_satellite_link.json ruby scripts/extract_links_toc build/**/index-*.html > build/toc.json
F2L=upstream_filename_to_satellite_link.json ./scripts/extract_links_toc build/**/index-satellite.html > build/toc.json

Then for the mapping file you can map File.dirname($filename). In the JSON you'll only have a mapping of directory names:

{
  "Administering_Project": "administering_red_hat_satellite"
}

Then you probably want to change the JSON filename to mention directory instead of filename.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer specifying the file names explicitly. Downstream the files and directories structure looks a bit different.

Once we unify the upstream and downstream - that would be an option.

Comment on lines 16 to 17
"build/Upgrading_Project/index-katello.html": "upgrading_connected_red_hat_satellite_to_6.15",
"build/Upgrading_Project_Disconnected/index-katello.html": "upgrading_disconnected_red_hat_satellite_to_6.15",
Copy link
Member

Choose a reason for hiding this comment

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

If this includes the version in the filename then we'll need to make sure that this is updated during Satellite version bumping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that would make sense. It will be detected on the first run of the tests against the new TOC, since the links to the next version will be missing.

@pr-processor pr-processor bot added the Waiting on contributor Requires an action from the author label May 23, 2024
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels May 23, 2024
@ShimShtein
Copy link
Member Author

@ekohl updated to use the shebang.

guides/Makefile Outdated
@@ -24,3 +24,6 @@ linkchecker-tryer:
find build -type f -name index\*html | xargs linkchecker -r1 -f common/linkchecker.ini --check-extern | ./scripts/linkchecker-tryer

ccutil: $(SUBDIRS)

toc: html
F2L=upstream_filename_to_satellite_link.json ./scripts/extract_links_toc build/**/index-*.html > build/toc.json
Copy link
Member

Choose a reason for hiding this comment

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

I instist you should only read index-satellite.html because that's the only relevant guide. And please also update that in the JSON file.

Suggested change
F2L=upstream_filename_to_satellite_link.json ./scripts/extract_links_toc build/**/index-*.html > build/toc.json
F2L=upstream_filename_to_satellite_link.json ./scripts/extract_links_toc build/**/index-satellite.html > build/toc.json

@ShimShtein
Copy link
Member Author

@ekohl Finally got it working and even without the toc target on the internal layer

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this is the last part, otherwise 👍. Thanks for iterating on it because I now think we have the correct version.

guides/README.md Outdated
which can be used for validating `administering_red_hat_satellite/accessing_server_admin#Logging_in_admin` link.

Note: The translation of book file names to root part of the link path is dependent on [`upstream_filename_to_satellite_link.json`](./upstream_filename_to_satellite_link.json) file.
For example `build/Administering_Project/index-foreman-el.html` would be translated to `.../administering_red_hat_satellite/...` path in the TOC.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example `build/Administering_Project/index-foreman-el.html` would be translated to `.../administering_red_hat_satellite/...` path in the TOC.
For example `build/Administering_Project/index-satellite.html` would be translated to `.../administering_red_hat_satellite/...` path in the TOC.

@ShimShtein
Copy link
Member Author

@ekohl resolved the last issue.

@ekohl ekohl merged commit d7af158 into theforeman:master May 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs tech review Requires a review from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants