-
Notifications
You must be signed in to change notification settings - Fork 554
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
make link usage consistent across the specification #687
Conversation
@opencontainers/runtime-spec-maintainers PTAL. It's kind of loud, but I don't think (and definitely should not) change any intent. Just a clean-up run. Thanks! |
The current Pandoc generation does not like repeated anchors (see
#626), and you have a few:
$ git grep -h '^\[.*\]:' origin/pr/687 | sed 's/\].*//' | sort | uniq -c | sort -n
2 [namespaces.7
2 [oci
2 [runtime-wiki
2 [zonecfg.1m
3 [charter
The three [charter] anchors are in master, and they're ok because they
aren't part of the spec (README, GOVERNANCE, and RELEASES). You're
[runtime-wiki] dup is new, but also for files that aren't in the spec.
Your [oci] dup is new, but only one is in the spec. However, you'll
need to dedup the new [namespaces.7] and [zonecfg.1m] references to
avoid confusing Pandoc (or we'll have to fix the current Pandoc
approach).
On the change itself, I also prefer reference links for internal links
with large fragments (e.g. config.md#platform-specific-configuration),
especially when they occur multiple times in the same file. So my
rule is something like “if I can save five or more characters by using
a reference link with a clear reference name, use a reference link”).
But why you have sounds workable and gives us an incentive to shorten
headers where that does not impact descriptiveness ;).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes needed to address @wking 's comment.
config-linux.md
Outdated
|
||
## Namespaces | ||
|
||
A namespace wraps a global system resource in an abstraction that makes it appear to the processes within the namespace that they have their own isolated instance of the global resource. | ||
Changes to the global resource are visible to other processes that are members of the namespace, but are invisible to other processes. | ||
For more information, see [the man page](http://man7.org/linux/man-pages/man7/namespaces.7.html). | ||
For more information, see the [NAMESPACES.7][namespaces.7] man page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/namespaces.7/ref-namespaces.7/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the ref-
prefix? I'd rather keep appending integers (e.g. namespaces.7.2
, namespaces.7.3
, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I was thinking along the lines of ref1; ref2.. prefixes given suffixes were related to the context of the subject. But really it doesn't matter to me. I'd rather go fix the tool. Just trying to help push these PRs out so we can get to 1.0 faster.
config-linux.md
Outdated
[console.4]: http://man7.org/linux/man-pages/man4/console.4.html | ||
[full.4]: http://man7.org/linux/man-pages/man4/full.4.html | ||
[mknod.1]: http://man7.org/linux/man-pages/man1/mknod.1.html | ||
[mknod.2]: http://man7.org/linux/man-pages/man2/mknod.2.html | ||
[namespaces.7]: http://man7.org/linux/man-pages/man7/namespaces.7.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/namespaces.7/ref-namespaces.7/
config-solaris.md
Outdated
@@ -1,6 +1,6 @@ | |||
# <a name="solarisApplicationContainerConfiguration" />Solaris Application Container Configuration | |||
|
|||
Solaris application containers can be configured using the following properties, all of the below properties have mappings to properties specified under zonecfg(8) man page, except milestone. | |||
Solaris application containers can be configured using the following properties, all of the below properties have mappings to properties specified under [zonecfg(1M)][zonecfg.1m] man page, except milestone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/zonecfg.1m/ref-zonecfg.1m/
config-solaris.md
Outdated
|
||
[priv-str-to-set.3c]: http://docs.oracle.com/cd/E53394_01/html/E54766/priv-str-to-set-3c.html | ||
[zoneadmd.1m]: http://docs.oracle.com/cd/E53394_01/html/E54764/zoneadmd-1m.html | ||
[zonecfg.1m]: http://docs.oracle.com/cd/E53394_01/html/E54764/zonecfg-1m.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/zonecfg.1m/ref-zonecfg.1m/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good to me.
@jlbutler can you rebase, change LGTM |
Sorry about delay, I was away for a bit and still catching up. @crosbymichael rebased and ready to go I hope, thanks! @mikebrow and @wking thanks for feedback and suggestions. I've appended '_2' to duplicate link tags in the config files, and also added a note to the style guide. |
Looks good to me.. |
GOVERNANCE.md
Outdated
@@ -67,4 +67,5 @@ For example: | |||
|
|||
> [runtime-spec adopted]: Tag 0647920 as 1.0.0-rc (+6 -0 #3) | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not churn this file unless we need to. Can we drop this insertion? Or if you feel particularly strongly about it, insert it upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me.
RELEASES.md
Outdated
@@ -48,4 +48,5 @@ Specifications have a variety of different timelines in their lifecycle. | |||
For example if a breaking change is introduced in v1.0.0-rc2 then the series would end with v1.0.0-rc4 and v1.0.0. | |||
- Minor and patch releases SHOULD be made on an as-needed basis. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fine by me.
bundle.md
Outdated
@@ -20,3 +20,6 @@ This directory MUST be referenced from within the `config.json` file. | |||
|
|||
While these artifacts MUST all be present in a single directory on the local filesystem, that directory itself is not part of the bundle. | |||
In other words, a tar archive of a *bundle* will have these artifacts at the root of the archive, not nested within a top-level directory. | |||
|
|||
|
|||
[macos_bundle]: http://en.wikipedia.org/wiki/Bundle_%28OS_X%29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This redirects to https://en.wikipedia.org/wiki/Bundle_%28macOS%29. Since you've updated the link text above, I suggest updating the URL here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow your suggestion, but I think I fixed it in next refresh. Maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow your suggestion…
I'm suggesting this line be:
[macos_bundle]: https://en.wikipedia.org/wiki/Bundle_%28macOS%29
config-linux.md
Outdated
|
||
## Namespaces | ||
|
||
A namespace wraps a global system resource in an abstraction that makes it appear to the processes within the namespace that they have their own isolated instance of the global resource. | ||
Changes to the global resource are visible to other processes that are members of the namespace, but are invisible to other processes. | ||
For more information, see [the man page](http://man7.org/linux/man-pages/man7/namespaces.7.html). | ||
For more information, see the [NAMESPACES.7][namespaces.7_2] man page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man pages seem to be mostly referenced in lowercase. So despite the NAMESPACES(7)
at the top of namespaces(7)
, I'd rather call it namespaces(7)
to match the “SEE ALSO” references at the end of user_namespaces(7)
, etc. No need to shout here ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were more than a few of these, I dropped all to lower case. Fine by me.
config-linux.md
Outdated
@@ -494,8 +494,8 @@ For more information, see [the man page](http://man7.org/linux/man-pages/man8/sy | |||
|
|||
Seccomp provides application sandboxing mechanism in the Linux kernel. | |||
Seccomp configuration allows one to configure actions to take for matched syscalls and furthermore also allows matching on values passed as arguments to syscalls. | |||
For more information about Seccomp, see [Seccomp kernel documentation](https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt) | |||
The actions, architectures, and operators are strings that match the definitions in seccomp.h from [libseccomp](https://github.com/seccomp/libseccomp) and are translated to corresponding values. | |||
For more information about seccomp, see [seccomp][seccomp] kernel documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the link text matches the reference, you can skip the target (e.g. [seccomp][]
). See the docs for the implicit link name shortcut. Taking advantage of that makes the line here a bit easier to read, since it removes the distraction of an explicit reference name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks!
config-solaris.md
Outdated
|
||
* **`linkname`** *(string, OPTIONAL)* Specify a name for the automatically created VNIC datalink. | ||
* **`lowerLink`** *(string, OPTIONAL)* Specify the link over which the VNIC will be created. | ||
Mapped to lower-link in the zonecfg(8) man page. | ||
Mapped to 'lower-link' in the [zonecfg(1M)][zonecfg.1m] man page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of marking these up, but I'd rather use backticks (lower-link
) like we do in most other parts of the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fixed all of these, thanks.
style.md
Outdated
This also facilitates updates of external link targets on a per-file basis. | ||
|
||
Referenced links should be kept in two alphabetically sorted sets, a general reference section followed by a man page section. | ||
To keep Pandoc happy, duplicate naming of links within the scope of the repository should be avoided by appending an '_N' to the link tagname, where 'N' is some number not currently in use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“within the scope of the repository” → “within pages compiled into the spec”. Although keeping that link up to date might be hard, and we may want to say “within pages listed in the Makefile's DOC_FILES
”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call.
style.md
Outdated
|
||
[github]: https://github.com | ||
[open-containers]: https://github.com/opencontainers | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can drop this trailing blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, thanks.
Thanks @wking for the additional feedback. Those changes have been committed. @opencontainers/runtime-spec-maintainers PTAL and thank you. |
Signed-off-by: Jesse Butler <jesse.butler@oracle.com>
Re-rebased, oh yes. |
1 similar comment
Unwind an overly-aggressive backtick replacement from f9dc90b (make link usage consistent across the specification, 2017-02-09, opencontainers#687). Signed-off-by: W. Trevor King <wking@tremily.us>
Through d87ec69 (Merge pull request opencontainers#687 from jlbutler/link-cleanup-676, 2017-03-03). Signed-off-by: W. Trevor King <wking@tremily.us>
This was broken by f9dc90b (make link usage consistent across the specification, 2017-02-09, opencontainers#687), which updated the link label, but not this link. Now that the link label matches the link text, we can use the implicit link name shortcut [1]. [1]: https://daringfireball.net/projects/markdown/syntax#link Signed-off-by: W. Trevor King <wking@tremily.us>
This was broken by f9dc90b (make link usage consistent across the specification, 2017-02-09, opencontainers#687), which updated the link label, but not this link. Now that the link label matches the link text, we can use the implicit link name shortcut [1]. [1]: https://daringfireball.net/projects/markdown/syntax#link Signed-off-by: W. Trevor King <wking@tremily.us>
These were added in f9dc90b (make link usage consistent across the specification, 2017-08-09, opencontainers#687) to follow the new _N name-dedup policy discussed in style.md. They were removed in ea65eb3 (config-solaris.md: fix info, 2017-04-28, opencontainers#786), overlooking that policy. This commit brings them back. Signed-off-by: W. Trevor King <wking@tremily.us>
Since f9dc90b (make link usage consistent across the specification, 2017-02-09, opencontainers#687), the official style is to only use reference-style links for external links. I expect the remaining three entries just slipped through. This commit adjusts everything found with: $ git grep ']: [a-z]' | grep -v http It also fixes the underscore -> hyphen in the glossary.md#container-namespace target and updates the capabilities location to catch up with 5a8a779 (Move process specific settings to process, 2016-03-02, opencontainers#329). Signed-off-by: W. Trevor King <wking@tremily.us>
Some hopefully useful link cleanup throughout the specification. Closes #676
Signed-off-by: Jesse Butler jesse.butler@oracle.com