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

Additonal corrections #3

Merged
merged 7 commits into from
Dec 12, 2018
Merged

Conversation

fscheiner
Copy link
Member

This includes additional corrections.

Please keep this PR open, because:

  • I'd like to discuss some possible additional corrections/modifications before merging things
  • the PR is missing the updated HTML files for now

@fscheiner
Copy link
Member Author

Version numbers

Mat has created {version} and {shortversion} attributes, i.e. vars that are expanded during the transformation process performed by AsciiDoctor from e.g. AsciiDoc to HTML.

I propose to change all occurrences of version numbers related to the GCT as a whole to these attributes respectively due to the following advantages:

  • This would allow us in the future to host documentation for different GCT versions in versioned subdirectories - i..e just like Globus did it.
  • Toolkit version numbers only need to be changed in the rules.mk of the respective versioned subdir

Inter-document links

Inter-document links are currently mainly linking to the HTML version of an AsciiDoc file. As a result, they for example don't work "correctly" in the rendered AsciiDoc files on GitHub. AsciiDoctor allows something similar called "Inter-document Cross References" that will be adapted to the desired target format, i.e. link to HTML files in the HTML output only - IIUC.

But there are some specifics we need to anticipate: E.g. for fragments when relative paths are used for inter-doc. cross references (ICRs) - and I haven't yet found out how to use absolute paths for ICRs - the relative path to the referenced document needs to be chosen based on the location of the AsciiDoc file that includes the fragment and not based on the fragment's location. This makes a problem for fragments that are included in multiple documents which are itself located in different locations.
Actually relative links in such fragments are a problem per se. So maybe a solution would be to not include fragments in multiple documents. I also don't see an advantage in this for "real" documentation, as we virtually replicate information on the website (same content at mutliple locations). Or can you think of a reason why this could be needed? It makes sense for example for the breadcrumb parts though.


What do you think?

@@ -14,7 +14,7 @@ This document provides information for GSI-OpenSSH developers.
The changes to http://www.openssh.org/[OpenSSH] to add GSI support are
limited, because we build on the existing GSSAPI support in OpenSSH for
Kerberos. See the
http://dev.globus.org/wiki/GSI-OpenSSH/Internals[GSI-OpenSSH Internals]
https://web.archive.org/web/20130607001903/http://dev.globus.org:80/wiki/GSI-OpenSSH/Internals[GSI-OpenSSH Internals]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I didn't knew this document was moved to the wiki of GSI-OpenSSH on GitHub.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we allow "rewriting history"? I.e. should I squash the change of the URL in the original commit (not sure if this code comment will then cease to exist) or should I create a new one for this change?

Copy link
Member

Choose a reason for hiding this comment

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

I think if you just commit to your 'corrections' branch, the pull request will get updated automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but we'll have another commit cluttering the history then. If we don't care that'd be the easiest way I think.

Copy link
Member

Choose a reason for hiding this comment

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

Either way is fine with me. Feel free to squash them in your branch (even if this conversation disappears as a result) or leave it as two commits.
We shouldn't squash in the actual gct-docs repo though.

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'm OK with that.

@msalle
Copy link
Member

msalle commented Aug 30, 2018

Concerning the fragments / links, would it work to start the links with a / i.e. to make them absolute paths? See e.g. asciidoctor issue #844
And if so, can't we then restrict fragments to only use absolute links?
Note/disclaimer: I'm absolutely not an expert in AsciiDoc(tor)

@fscheiner
Copy link
Member Author

fscheiner commented Aug 30, 2018

Me neither, I'm just testing various options. At least on GitHub the "start with a slash" method seems to work, see https://github.com/fscheiner/gct-docs/blob/c8fa6589dcaf3ddd3678f69d45806123dc087e34/myproxy/Cred_Mgmt_MyProxy_Usage_Statistics_Frag.adoc (here7). It didn't work for me locally, but I viewed the HTML files via a pseudo GVfs mount. And I don't know how it will look when rendered via GitHub pages.
I also tried to use a variable, but GitHub doesn't really include fragment AsciiDoc files but just links them, so the value of the variable (EDIT: defined in https://raw.githubusercontent.com/fscheiner/gct-docs/c8fa6589dcaf3ddd3678f69d45806123dc087e34/myproxy/admin/index.adoc) is not known when the fragment is shown for itself.

@fscheiner
Copy link
Member Author

Looks like the "start with a slash" method does not work when the HTML files are served from GitHub pages, see https://fscheiner.github.io/gct-docs/myproxy/admin/index.html#myproxy-usage (does serve the master branch which in my fork includes the test-xref modifications mentioned above). Most likely due to the anchor character preceding the path of the cross reference. :-/

@msalle
Copy link
Member

msalle commented Aug 31, 2018

Hmm, I think it interprets them as anchor names, not as paths. From asciidoc-reference-check (under supported-anchors-and-references) I would say you either need to add .adoc or perhaps use link instead of xref.
Perhaps http://discuss.asciidoctor.org/Re-Link-versus-XRef-td327.html is also of any help?

@fscheiner
Copy link
Member Author

I did some testing and had some further reading, thanks for the links. To me it now looks like the asciidoctor documentation I referenced earlier is not correct in regard to inter-document cross references, because all tests I did with both forms (<<[...]>> and xref:[...]) didn't behave as documented - or let's say not as expected by me. Just compare the raw file and the corresponding HTML output.

With the {outfilesuffix} variable as "suffix" to file names without file type suffix and a variable defined in the AsciiDoc file that includes a fragment pointing to the document root (as relative path, absolute paths don't seem to work), the link macro will work from fragments and allow to link to other documents and/or anchors in other documents for the HTML output served by GitHub pages with correct file type suffix.
It doesn't work when viewing the AsciiDoc files itself on GitHub (see https://github.com/fscheiner/gct-docs/blob/master/myproxy/Cred_Mgmt_MyProxy_Usage_Statistics_Frag.adoc), as the variable pointing to the document root is not expanded there, as included fragments are not rendered as part of the files that includes them but just linked. So when viewing the fragment, used vars from files that include the fragment cannot be expanded. And we cannot define the document root var in the fragments, as they can be included by files in different locations.

So all in all no additional value to just directly link to the html files for the cases (1) viewing the AsciiDoc source on GitHub and (2) serving HTML from GitHub.

I haven't yet examined how this will behave for PDF output but will keep things as is for now for inter-document links until we find a better solution.


@matyasselmeci @ellert @msalle
Any comments on the "Version numbers" part of #3 (comment)? I don't want to make these changes, commit them and only then get them rejected. :-)

@matyasselmeci
Copy link
Collaborator

Hi Frank,
I support the version number changes except we need to make sure we don't change them in places like release notes and migration guides where the version number should be fixed.

@fscheiner
Copy link
Member Author

[...] we need to make sure we don't change them in places like release notes and migration guides where the version number should be fixed.

Yeah, that's also my concern. I'll come up with a patch and promise to be careful :-D.

@fscheiner
Copy link
Member Author

@matyasselmeci @msalle
This PR is now complete with the changes related to the toolkit version number. HTML files are up to date with all prior changes. Please include it if you're satisfied with the changes.

To see how it could look in the future, please also check my mockup at https://fscheiner.github.io/gct-docs/. The mockup assumes version 7.0 is the latest available version of the GCT and is served from the master branch of my fork. This as you can only serve pages from (1) master, (2) gh-pages or (3) a /docs folder in the master branch and I didn't want to modify the gh-pages branch in my fork directly and consider the third option n/a. Please don't mind, the mockup is missing some last minutes changes I squashed into f36ec3e and 00585bd.

Interesting BTW that GitHub links those two commit hashes above in the context of gct-docs upstream, although they aren't yet included. ?-/

@matyasselmeci
Copy link
Collaborator

Could you update your mockup to add 00585bd? It looks like the literal string {shortversion} is being put into the HTMLs instead of the actual version.

@fscheiner
Copy link
Member Author

Oh, didn't notice this.

I just now checked /admin/install/appendix.html and to me it looks like AsciiDoctor cannot or does not expand variables, if a string with a variable is used in another variable. I.e. in the document mentioned above this comes from /admin/breadcrumb_frag.adoc:

ifdef::backend-html5[link:../../index.html[GCT] -> link:../index.html[Admin Manuals] -> {doctitle}]

...where {doctitle} is expanded to the title of the document that includes this fragment. But the title of /admin/install/appendix.adoc is defined as:

= GCT {shortversion} Installation Appendix =

and AsciiDoctor seems to use this unexpanded. To be sure I have to check other files which include breadcrumb fragments. That was highly unexpected.

UPDATE: Looking at a few other files in the mockup this doesn't seem to happen everywhere, so maybe I am wrong with my assumption above. I'll have to double-check things. Thanks for the pointer.

@fscheiner
Copy link
Member Author

An update:

After double-checking things, my assumption from #3 (comment) seems to be correct: Variables in document titles are not expanded when using the {doctitle} variable. Should we consider that a bug in AsciiDoctor?

The reason why it doesn't seem to happen everywhere in the mockup is that some documents use the version number verbatim, e.g. like in https://raw.githubusercontent.com/fscheiner/gct-docs/master/7.0/appendices/commands/index.adoc

Sadly this means, that we could not use variables with version numbers in document titles. Quickly scanning through f36ec3e, the majority of related changes unfortunately seems to happen in document titles. But using verbatim version numbers in the document titles and version number variables everywhere else seems to be inappropriate.

@fscheiner
Copy link
Member Author

@matyasselmeci
I dropped the version number changes for now to get the corrections into the documentation. HTML files were also updated to only include the corrections from 2018-08-30. So if there are no objections this PR can be merged in its current state.


I'll later come up with a PR for the GCT 6.2 documentation. There I'll follow the original plan to have:

  1. Versioned sub directories (like in https://github.com/fscheiner/gct-docs/tree/master)
  2. Versioned documentation - though done manually for now

Copy link
Collaborator

@matyasselmeci matyasselmeci left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for all the work!

@matyasselmeci matyasselmeci merged commit 4afb016 into gridcf:gh-pages Dec 12, 2018
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 this pull request may close these issues.

3 participants