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 initial support for structured settings references (yaml) #105

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented Dec 16, 2024

Prioritized this work in the context of #94 where @lcawl and I are debating if we need the inline product applicability annotation.

Since most of these are in settings it'd be good to start supporting these earlier so writers can validate that this feature set is rich enough.

@bmorelli25 @KOTungseth do we need to be able to link to each individual setting directly? I assume so in which case I'll need to add these to the linkmap docs builder produces.

@kilfoyle I made some changes to your format that is part of:

elastic/kibana#191787

Primarily each description field now uses a multiline string. Your format used a string array.

A multiline string has two benefits.

  • I don't need to create a markdown parser for each line
  • You can annotate these strings as markdown in IDE's (IntelliJ and I think VsCode).
image

Compare the first un-annotated description with the 2nd.

Note I did not go over all the settings to change from asciidoc to markdown in the example.

Secondly this changes the product applicability format to match #95 and #103

- setting: x
  applies:
    stack: ga 8.1
    serverless: tech-preview
    hosted: beta 8.1.1
    eck: beta 3.0.2
    ece: unavailable

Instead of

- setting: x
  platforms:
    - cloud

@lcawl what would be the equivalent of the latter in the new format? Do we need a shorthand notation for cloud vs self managed?

Thirdly, I do not think we should expose id: let docs builder generate the slug for each of these settings.

WDYT: @kilfoyle @bmorelli25 @KOTungseth ?

Example output

Not all properties of a setting are rendered out yet, will tackle that in a follow up PR.

image

@Mpdreamz Mpdreamz added the authoring Relates to our markdown parser label Dec 16, 2024
@kilfoyle
Copy link

@Mpdreamz Awesome! I like the changes a lot.

  • As soon as I can, I'll update the descriptions in my PR to use multi-line strings (specifically, I'd need to update the settings descriptions that are part of that PR, and the generation script).
  • As discussed in Slack, it may not be worth updating the platform -> applies property in the old system, and not having to do so would speed things up a bit. @bmorelli25 Let us know if you have thoughts on that, please.

@KOTungseth
Copy link

@bmorelli25 @KOTungseth do we need to be able to link to each individual setting directly? I assume so in which case I'll need to add these to the linkmap docs builder produces.
Yes, there is a Kibana use case for direct linking to each setting. I'm unsure about areas, but I would assume the same. @kilfoyle keep me honest here.

Thirdly, I do not think we should expose id: let docs builder generate the slug for each of these settings.
I agree. The more we can autogenerate with docs builder, the better.

@kilfoyle
Copy link

kilfoyle commented Dec 16, 2024

Yes, there is a Kibana use case for direct linking to each setting. I'm unsure about areas, but I would assume the same.

For sure. A good example is that when one setting is deprecated our guidance typically links to the alternative, newer setting to use. I think this is the case in all of the settings docs.

Also, I agree that generating the IDs would be so much nicer than explicitly defining them in the source.


Also, some additional thoughts on:

- setting: x
  applies:
    stack: ga 8.1
    serverless: tech-preview
    hosted: beta 8.1.1
    eck: beta 3.0.2
    ece: unavailable

For the vast majority of existing settings I don't know when they GAed, on which version they're available in a hosted environment, if they're supported in ECE / ECK, etc. I'm also not sure how we'd gather this info. For serverless, I don't think config settings can be adjusted at all. I think we may need to simplify this a bit, at least for now.

@lcawl
Copy link

lcawl commented Dec 17, 2024

@lcawl what would be the equivalent of the latter in the new format? Do we need a shorthand notation for cloud vs self managed?

I think that for references pages (same as narrative pages), we'd want to have the metadata at the page level and ideally only override it at the section / paragraph / line level if a particular setting diverges from that. If we don't know how/if a setting works in ECE or ECK IMO we ought to omit those items from that page's metadata (i.e. only assert and display the things that we know and can verify to be true). IMO most of our pages won't have ECE or ECK assertions with the exception of the installation and configuration pages specific to those contexts.

For the vast majority of existing settings I don't know when they GAed, on which version they're available in a hosted environment, if they're supported in ECE / ECK, etc. I'm also not sure how we'd gather this info. For serverless, I don't think config settings can be adjusted at all. I think we may need to simplify this a bit, at least for now.

In the original proposal for the meta-data and admonitions most of those properties are optional. That is to say, you could assert that the content in the page was applicable to serverless/cloud/stack without being required to specify the exact version/date since it's true that we don't have that GA date for a lot of existing features (without a lot of investigation through our old docs). I think for our contributor guidelines, however, we need to be clear about what we want the new behaviour to be for future features.

@Mpdreamz
Copy link
Member Author

Mpdreamz commented Dec 17, 2024

In the original proposal for the meta-data and admonitions most of those properties are optional. That is to say, you could assert that the content in the page was applicable to serverless/cloud/stack without being required to specify the exact version/date since it's true that we don't have that GA date for a lot of existing features (without a lot of investigation through our old docs). I think for our contributor guidelines, however, we need to be clear about what we want the new behaviour to be for future features.

Just to be explicit in docs-builder the version component is also optional.

I think that for references pages (same as narrative pages), we'd want to have the metadata at the page level and ideally only override it at the section / paragraph / line level if a particular setting diverges from that.

This makes a a lot of sense to me.

I did open #113 to allow folks to set all self managed or cloud products in one go.

e.g:

  applies:
    self: ga 8.1
    cloud: unavailable

With that you could possibly update the PR to use

applies:
   self:
   cloud:

@Mpdreamz
Copy link
Member Author

@kilfoyle I noticed we don't document a settings data type e.g

- setting: x
  type: bool

Do you think this is worthwhile? We often specify it in the description, i think it'd good to explicitly state it.

I think we cover a lot of area by limiting it to

  • string
  • bool
  • int
  • float
  • enum (e.g we expect the setting block to have an options block listing available options).

If unspecified we'll assume string but docs-builder will emit a warning.

@Mpdreamz
Copy link
Member Author

Merging this in so I can focus on a follow up PR that focusses more on the HTML output.

Please feel free to continue to comment on this PR!

@Mpdreamz Mpdreamz merged commit 880b7b1 into main Dec 17, 2024
4 checks passed
@Mpdreamz Mpdreamz deleted the feature/settings-directive branch December 17, 2024 09:02
@kilfoyle
Copy link

@Mpdreamz Thanks, you're suggestions were really great. Both the yaml file and the script are a lot cleaner now with the multiline string descriptions, and I agree that having a required datatype setting is a very good thing.

Specifically, I've made these changes in my settings PR:

  • Each setting description is now a whitespace-preserving multiline string, rather than an array of strings.
  • Each setting now has a required "datatype" field that can be one of string/bool/int/float/enum.

The generation script doesn't actually enforce that "datatype" is present in the yaml nor that it's set to one of the available types. I can add that in or we can just punt it to the V3 setup. It'll probably be me creating these yaml files anyway, so I'll do my best to play by the rules. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authoring Relates to our markdown parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants