Avoid hardcoded links in READMEs by mrodm · Pull Request #938 · elastic/elastic-package

mrodm · 2022-08-23T09:51:27Z

Closes #948

This PR adds a new function ("url") available into Readme templates to render links based on the event and fields functions. All links will be defined in a common file that will be the source of truth.

This intended to be used in integrations repository where a specific ~~csv file links_table.csv~~ YAML file links_table.yml containing all the available links is defined.
This file should be placed at the root of the integrations repository and it should follow this pattern:

links:
  link_key: http://url1.test
  foo_key: http://url2.test

Being there at the root of the repository, this file (all the links) are available and shared by all the packages defined in the repository in building time (elastic-package build).

Example of usages in the README using the above file as basis:

README template:

Interesting links:
- {{ url "link_key" }}
- {{ url "foo_key" "howto guide" }}

README generated:

Interesting links:
- http://url1.test
- [howto guide](http://url2.test)

elasticmachine · 2022-08-23T09:55:59Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-09-07T14:21:26.310+0000
Duration: 32 min 4 sec

Test stats 🧪

Test	Results
Failed	0
Passed	800
Skipped	0
Total	800

🤖 GitHub comments

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

/test : Re-trigger the build.

elasticmachine · 2022-08-23T11:28:44Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`33/33`)	💚
Files	67.213% (`82/122`)	👍 0.264
Classes	62.941% (`107/170`)	👍 0.517
Methods	49.351% (`342/693`)	👎 -0.275
Lines	32.949% (`3084/9360`)	👎 -0.34
Conditionals	100.0% (`0/0`)	💚

mrodm · 2022-08-25T10:43:12Z

/test

mtojek

I'm not sure if I fully understand what's the purposes of this PR. Do you want to add a new file to a package? Is it necessary to be a .csv file?

If so, I would start with a package-spec issue to decide about the form of the file first.

mrodm · 2022-08-26T09:43:29Z

I'm not sure if I fully understand what's the purposes of this PR. Do you want to add a new file to a package? Is it necessary to be a .csv file?

If so, I would start with a package-spec issue to decide about the form of the file first.

I see that the description of the PR lacked some information regarding this proposal as a first step to solve elastic/integrations#2652. I'll give some more information here (and I'll update the description accordingly afterwards).

With this PR, my proposal is adding a file at the root of "integrations" repository that contains all the links to elastic documentation pages. Being there, all these links will be available and shared by all the packages in building time (elastic-package build). Similarly, as License file.

As that file would be out of the package scope, at least with this design, it should not be added to the package-spec.

mtojek · 2022-08-26T10:05:26Z

Yeah, but keep in mind that Elastic packages shouldn't have a strong coupling between with elastic/integrations repository. They can be stored in any place.

EDIT:

I'm wondering if we can decouple links_table.csv from elastic-package and integrations, and make it more generic. I'm thinking about a dedicated ENV which contains a path to the links_table.csv (assuming that we want that format instead of YAML). Whenever elastic-package finds the {{ url ... }}, it will look up the ENV to read the value from there.

mrodm · 2022-08-26T10:26:11Z

I'm wondering if we can decouple links_table.csv from elastic-package and integrations, and make it more generic. I'm thinking about a dedicated ENV which contains a path to the links_table.csv (assuming that we want that format instead of YAML). Whenever elastic-package finds the {{ url ... }}, it will look up the ENV to read the value from there.

That way is way more flexible. Avoiding hard-coding the filename and the path where it should be located will help to avoid issues. Maybe later it needs to be changed to some other folder that file or just being renamed, and that should be independent of elastic-package.

In case ENV is not defined, it could be kept looking in the root of the repository.

I will look into it to update this PR accordingly. Thanks for the suggestion!

About csv vs YAML, as first step I just wanted to follow the same structure it was shown as example in the main issue (example). But definetly, this can be changed later to YAML format. Probably easier to read.

Just some other possible format using YAML:

links:
  link_key: http://url1.test
  foo_key: http://url2.test

mrodm · 2022-09-05T16:51:17Z

Could you please add another test package which uses the links feature?

I've added new tests for generateReadme function instead of adding a new package under test/packages to try to test all the usages of the functions available in readme (fields, event and now url). These tests along with the ones to test linksDefinitionsFilePath would probably cover this.

~~Would it be better to add a new package? Would that test something else ?~~

EDIT:
Added a new packages under test/packages (readme_links) with just a manifest and the readme docs to be able to test this rendering. test-check-packages.sh script has been updated to include the ELASTIC_PACKAGE_LINKS_FILE_PATH definition.

mtojek

Would it be better to add a new package? Would that test something else?

If you ask me, more tests are always better. The benefit of adding a package is that we can catch if the README.md changed. With unit tests, you won't catch it if the calling function suddenly disappears.

If you don't want to create a test package, you can modify an existing one.

mtojek · 2022-09-06T07:49:40Z

docs/howto/add_package_readme.md

+      foo,http://url.com.test
+      help,http://other.url.com/help


It should YAML right?

mtojek · 2022-09-06T07:49:48Z

docs/howto/add_package_readme.md

+      foo,http://url.com.test
+      help,http://other.url.com/help


Same question

mtojek · 2022-09-06T07:53:57Z

internal/docs/readme.go

 }

-func renderReadme(fileName, packageRoot, templatePath string) ([]byte, error) {
+func renderReadme(fileName, packageRoot, templatePath string, linksMap linkMap) ([]byte, error) {


Yeah, I wouldn't preoptimize it. If it becomes a bottleneck in the future, we can refactor it.

mtojek

LGTM! Well done.

mrodm · 2022-09-07T11:42:15Z

/test

jsoriano

👍

mrodm · 2022-09-07T14:12:20Z

/test

mrodm added the Team:Ecosystem Label for the Packages Ecosystem team label Aug 23, 2022

mrodm self-assigned this Aug 23, 2022

mrodm added 3 commits August 23, 2022 11:58

Read optional csv file with key and urls

51755a4

new add function type

c31b019

extract to its own file

4ca14dd

mrodm force-pushed the avoid_hardcoded_links branch from 03d8a78 to 4ca14dd Compare August 23, 2022 09:58

mrodm added 2 commits August 23, 2022 12:04

updated howto doc

dfa63fd

Add license headers

58950a5

mrodm mentioned this pull request Aug 24, 2022

Avoiding hard-coded documentation links in the integration readmes elastic/integrations#2652

Closed

mrodm added 2 commits August 24, 2022 17:50

Add tests for render functions

e6a4454

Add license header

271052f

error messages updated

42c0155

mrodm mentioned this pull request Aug 25, 2022

Update README files using the url placeholder for elastic documentation links elastic/integrations#4068

Merged

2 tasks

mrodm marked this pull request as ready for review August 25, 2022 13:55

mrodm requested a review from a team August 25, 2022 13:55

mtojek reviewed Aug 26, 2022

View reviewed changes

mrodm added 8 commits August 26, 2022 15:34

use filepath

d7ab7dd

Use env var to define path for links file

cec60e3

replace const env prefix

530fb4b

replace other occurrences of elastic prefix env var

754374b

rename var

a17b8ad

add debug message

86d3c11

update readme

7c79855

set optional links definitions file

4bc4876

mrodm added 6 commits September 5, 2022 12:21

review comments

6aac009

just one function to render urls/links adding options

c933862

update tests

e38f8d5

Added tests for readme generation

91c64d0

Add test for linksDefinitionsFilePath

56c341b

add license header

54f4ef2

mrodm requested a review from mtojek September 5, 2022 16:51

mrodm added 3 commits September 5, 2022 18:55

check err before defer function

c00a549

fix debug/error messages

5e062f5

Add package to test readme links render

bb9e87c

mtojek reviewed Sep 6, 2022

View reviewed changes

mrodm added 7 commits September 6, 2022 10:20

Update missing references to csv

cb5614e

update test package

4143171

update error messages

ae8c37b

move links map read to deepest function

32fe8aa

Added links file env var to test-build-zip

24038de

set links map as parameter of renderReadme to help tests

c189301

remove debug commands script

ea100ca

mrodm requested a review from mtojek September 6, 2022 15:53

mtojek approved these changes Sep 7, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into avoid_hardcoded_links

96f29a4

jsoriano approved these changes Sep 7, 2022

View reviewed changes

Reviewed tests

7ee6a2f

mrodm mentioned this pull request Sep 7, 2022

Added new file for links definitions elastic/integrations#4156

Merged

4 tasks

mrodm merged commit 20e8351 into elastic:main Sep 7, 2022

mrodm deleted the avoid_hardcoded_links branch September 7, 2022 15:00

mrodm mentioned this pull request Sep 7, 2022

Add docs template language to elastic-package create package #954

Merged

4 tasks

Conversation

mrodm commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

mrodm commented Aug 25, 2022

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mrodm commented Aug 26, 2022

Uh oh!

mtojek commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrodm commented Aug 26, 2022

Uh oh!

mrodm commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mtojek Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mtojek Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mtojek Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mrodm commented Sep 7, 2022

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

mrodm commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mrodm commented Aug 23, 2022 •

edited

Loading

elasticmachine commented Aug 23, 2022 •

edited

Loading

elasticmachine commented Aug 23, 2022 •

edited

Loading

mtojek commented Aug 26, 2022 •

edited

Loading

mrodm commented Sep 5, 2022 •

edited

Loading