-
Notifications
You must be signed in to change notification settings - Fork 85
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
[DOC] Fix bad link to resolve docs-ci failure #166
Conversation
docs/index.asciidoc
Outdated
@@ -143,7 +143,7 @@ This plugin supports the following configuration options plus the <<plugins-{typ | |||
| <<plugins-{type}s-{plugin}-query_template>> |<<string,string>>|No | |||
| <<plugins-{type}s-{plugin}-result_size>> |<<number,number>>|No | |||
| <<plugins-{type}s-{plugin}-retry_on_failure>> |<<number,number>>|No | |||
| <<plugins-{type}s-{plugin}-result_on_status_>> |<<number,number list>>|No |
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.
@yaauie, please validate that this fix aligns with your original intent before I merge it.
docs/index.asciidoc
Outdated
@@ -143,7 +143,7 @@ This plugin supports the following configuration options plus the <<plugins-{typ | |||
| <<plugins-{type}s-{plugin}-query_template>> |<<string,string>>|No | |||
| <<plugins-{type}s-{plugin}-result_size>> |<<number,number>>|No | |||
| <<plugins-{type}s-{plugin}-retry_on_failure>> |<<number,number>>|No | |||
| <<plugins-{type}s-{plugin}-retry_on_status>> |<<number,number list>>|No | |||
| <<plugins-{type}s-{plugin}-retry_on_status>> |<<number,number>>|No |
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.
The intent here was to have the text of the link say "number list" because it takes a list of numbers.
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.
What about result_on_status_>>
--> retry_on_status>>
? This change aligns with labeling in descriptions section, but I don't want to guess incorrectly.
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.
<<number,number list>>
will work for plugins generated for the Logstash Plugin Reference, but will fail in the Versioned Plugin Reference (VPR). Why? Because the abbreviated anchor format (<<internal-anchor>>
) works only within the same book. Outside of the context of a particular book, that format has no meaning.
-
LSR. This format works in the LSR because the referenced topic is present in the Logstash Reference.
-
VPR. The reason the format for our typical Value types works in the VPR at all is because of gsubs in the VPR docgen script:
So when you modify the expected format to be <<number,number list>>
, it doesn't match the gsub <<number,number>>
. It renders literally and fails docs CI in the VPR docgen PR.
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.
Related:
If a number list
is something we expect to use more frequently in the future, we should update docs for to add number list
as a value type. OR expand the current entry for number to show an example/description of a number list. The current description and example is for a single entry.
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.
Not sure we want to make it so easy to deviate from the standard. Instead we should vet and update links and gsubs to be more appropriate to our input types.
If a number list is something we expect to use more frequently in the future, we should update docs for to add number list as a value type. OR expand the current entry for number to show an example/description of a number list. The current description and example is for a single entry.
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.
DO NOT MERGE until same issue on line 353 is resolved!
@yaauie I'd like to get this fix merged to avoid breaking future Versioned Plugin Reference builds. Please consider comment and open an issue in
|
Replaced by #172 |
No description provided.