Skip to content

Conversation

@colleenmcginnis
Copy link
Contributor

@colleenmcginnis colleenmcginnis commented Aug 30, 2022

Closes #880

In elastic/integrations#3433, we added more detailed documentation guidelines for integrations (including template language). In that PR @jsoriano suggested:

elastic-package create package can be used to create an initial scaffolding for new packages, as a follow up we could apply these recommendations in the templates there.

@jsoriano pointed me to this directory, but I'm not sure if I put the template language in the correct place. 🙃 Let me know if it should go somewhere else!

To do

  • Confirm template language is in the correct place
  • Approval from @jsoriano
  • [outside this repo] Update the script for building integration docs to reformat code comments to prevent build failures cc @bmorelli25
  • After ☝️ is complete, merge this PR

@colleenmcginnis colleenmcginnis self-assigned this Aug 30, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 30, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-12T11:08:42.955+0000

  • Duration: 30 min 3 sec

Test stats 🧪

Test Results
Failed 0
Passed 829
Skipped 0
Total 829

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 30, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (34/34) 💚
Files 66.929% (85/127) 👍
Classes 61.667% (111/180) 👍
Methods 48.834% (356/729) 👍
Lines 32.231% (3225/10006) 👍
Conditionals 100.0% (0/0) 💚

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Nice, this is going to help a lot to have consistent readmes. Thanks!

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Added a couple of suggestion I have just thought about.

<!-- ### {Data stream name}
The ` + "`{data stream name}`" + ` data stream provides events from {source} of the following types: {list types}. -->
Copy link
Member

Choose a reason for hiding this comment

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

Btw, there are helpers to generate documentation of fields, see for example in the template for the README of the Apache integration: https://github.com/elastic/integrations/blob/8d2ecab10823e2b54ea7ffa3e4252b9eaba597a8/packages/apache/_dev/build/docs/README.md?plain=1#L17

{{ fields "access" }}

It may be good to include them also here.

<!-- Optional -->
<!-- #### Example
An example event for ` + "`{data stream name}`" + ` looks as following:
Copy link
Member

Choose a reason for hiding this comment

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

@colleenmcginnis colleenmcginnis marked this pull request as ready for review September 6, 2022 19:02
@colleenmcginnis
Copy link
Contributor Author

@jsoriano can you help with the failing checks or tag in someone else to help? Thanks!

@jsoriano
Copy link
Member

jsoriano commented Sep 7, 2022

@jsoriano can you help with the failing checks or tag in someone else to help? Thanks!

Oh, I missed this is already a template.

Try to "print" the function call:

{{"{{event \"status\"}}"}}

<!-- Any prerequisite instructions -->
For step-by-step instructions on how to set up an integration, see the
[Getting started](https://www.elastic.co/guide/en/welcome-to-elastic/current/getting-started-observability.html) guide.
Copy link
Contributor

Choose a reason for hiding this comment

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

A branch has been just merged that introduces a new helper in elastic-package to render this kind of links to Elastic documentation guides (links like https://www.elastic.co/guide/*).

With that PR, this link could be replaced by the "url" helper:

{{ url "getting-started-observability" "Getting started" }} guide.

To be used in integrations repository, currently there are still some requirements pending (on-going):

Copy link
Member

Choose a reason for hiding this comment

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

Good point, but if we use url here, we should also probably add support for the creation of the links map file when it doesn't exist, as this command can be used on any place. Let's leave this for a follow up.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, let's leave it as it is 👍.
As you mentioned at this point it is not mandatory that the links definitions file exist.

@colleenmcginnis
Copy link
Contributor Author

@jsoriano I'm confused about your last comment. Can you clarify what needs to be updated?

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

@colleenmcginnis sorry for the confusion, find here as suggestions what I meant.

An example event for ` + "`{data stream name}`" + ` looks as following:
{{event "{data stream}"}} -->
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
{{event "{data stream}"}} -->
{{"{{event \"{data stream}\"}}"}} -->

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems complicated unless I'm missing something about how templates work. Would it work to just use this?

Suggested change
{{event "{data stream}"}} -->
\{\{event "{data stream}"\}\} -->

Or are you using the syntax above because the data stream name can be somehow automatically injected into the template?

An example event for ` + "`{data stream name}`" + ` looks as following:
{{event "{data stream}"}} -->
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
{{event "{data stream}"}} -->
{{"{{event \"{data stream}\"}}"}} -->

<!-- #### Exported fields
{{fields "{data stream}"}} -->`
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
{{fields "{data stream}"}} -->`
{{"{{fields \"{data stream}\"}}"}} -->`

@jsoriano
Copy link
Member

jsoriano commented Sep 8, 2022

Hey @colleenmcginnis, sorry for the back and forth, but after trying some things I think it'd be better to go back to the original version you sent before my suggestions in #954 (review).

The thing is that we have two ways of defining the readme in a package, one is using a static file, the other one is using a template that generates on build time the static file. The event and fields helpers only work when using the template, but elastic-package create package creates the static file, not the template, so my suggestion cannot work.

I think we need to refactor this code to be more coherent with our current recommendations. On most packages we are using the template and not the static file. In the meantime I think that we could merge your original changes.

Sorry again for the confusion!

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks!

@colleenmcginnis colleenmcginnis merged commit 58db47f into elastic:main Sep 12, 2022
@colleenmcginnis colleenmcginnis deleted the issue-880 branch September 12, 2022 14:34
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.

[docs] Add new docs template language to elastic-package create package

4 participants