Validate that dashboard IDs start with package names #66

mtojek · 2020-10-21T10:44:40Z

Issue: #65

elasticmachine · 2020-10-21T10:47:24Z

💚 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

Build Cause: [Pull request #66 updated]
Start Time: 2020-10-22T06:55:59.392+0000
Duration: 2 min 17 sec

ycombinator · 2020-10-21T14:05:30Z

versions/1/kibana/spec.yml

@@ -9,7 +9,7 @@
      - description: A dashboard asset file
        type: file
        contentMediaType: "application/json"
-        pattern: '.+\.json'
+        pattern: '{PACKAGE_NAME}+-.+\.json'


This is an interesting idea - to allow certain keywords in the spec that can act as variables/placeholders whose value depends on the context. TBH, this is not how I had imagined implementing semantic validations but I like it - its simple! Let's go with this idea and see how far we can take it.

We should probably document these keywords somewhere (outside the spec, of course) for spec developers so they know which keywords are available to them and what they mean.

Do you think about README or CONTRIBUTING file in package-spec or somewhere else?

I think CONTRIBUTING is the right place for this sort of thing. Let's try to keep the README more for spec consumers and CONTRIBUTING for spec contributors, although there might be some overlap/fuzziness, which is okay IMHO.

I added a short one.

ycombinator · 2020-10-21T14:11:50Z

code/go/internal/validator/folder_spec.go

 	for _, itemSpec := range s.Contents {
 		if itemSpec.Name != "" && itemSpec.Name == folderItemName {
 			return &itemSpec, nil
 		}
 		if itemSpec.Pattern != "" {
-			isMatch, err := regexp.MatchString(itemSpec.Pattern, folderItemName)
+			isMatch, err := regexp.MatchString(strings.ReplaceAll(itemSpec.Pattern, "{PACKAGE_NAME}", packageName), folderItemName)


Validating that the dashboard file name starts with the package is a good start. But we also need to implement two more related validations:

validating that the ID of the dashboard, as defined in the dashboard asset JSON file, starts with the package name or, better yet, matches the file name (sans extension) of the dashboard asset JSON file, and

validating that there aren't any references to the dashboard ID with the package name prefix removed.

For both these additional validations, I think you'll need to define the spec files for Kibana assets. I was hoping to avoid this but I think we have to bite that bullet now. Perhaps we could define them with only the fields we need and leave the rest open-ended (that is, allow additional fields in the asset file that are not explicitly defined in the spec)?

If you want to tackle these additional validations in a separate PR that's fine by me. In that case, just update the issue corresponding to this PR with a checklist of the 3 total validations so we can keep track of the work done vs. remaining.

I agree with the first point (validating the ID of the dashboard inside the file). I can introduce the schema for dashboard file. We might need to introduce the binding between ID and packageName also there.

Regarding the second point, it's hard to implement it as it's mainly "search and replace" operation and we may fail the validation if there are references between dashboards in different packages (e.g. jump to the system dashboard).

Let's break these out into follow up PRs. Implementing the first point will introduce more files so let's not bloat this PR with those. Implementing the second point might be tricky as you pointed out so let's keep that out of scope for this PR. What you have in this PR here is already useful so let's move that forward first.

Regarding the second point, it's hard to implement it as it's mainly "search and replace" operation and we may fail the validation if there are references between dashboards in different packages (e.g. jump to the system dashboard).

Visualization used by the dashboard should all be contained within the same integration (we cannot include a visualization from a different integration, as it may not be installed). Are you thinking about links here? Most of what we have is just visualizations, and their IDs should match this pattern.

++ on following up with these cases

Perhaps let's not close #65 until the other PRs get in?

Good point, Carlos. I updated the description.

code/go/internal/validator/folder_spec.go

mtojek · 2020-10-22T06:51:25Z

I'll merge this PR as it's already an improvement. We can work on follow up PRs then.

EDIT:

Unfortunately I don't have permissions to overwrite required approvals in this repository.

ruflin · 2020-10-22T09:27:56Z

Is this only about dashboards or also visualizations, searchs etc?

mtojek · 2020-10-22T09:46:59Z

It's only for dashboards (according to the email thread).

ruflin · 2020-10-22T09:49:25Z

I would assume we also have conflicts on visualizations, search etc?

exekias

Thank you for jumping into this so quick!

mtojek · 2020-10-22T15:35:37Z

I would assume we also have conflicts on visualizations, search etc?

I tried to reproduce it and we might need to update ID references one more time, @ycombinator . How to reproduce it:

$ elastic-package stack up -d
$ metricbeat setup --dashboards

Then navigate to the system dashboard which may be scrambled.

I'm not quite sure about the previous fix elastic/integrations#171 , which only covered dashboards. In the worst case we need to wrap up a similar fix to elastic/integrations#319 for visualization and searches, and the same way iterate on other PRs related to it.

ycombinator · 2020-10-26T00:52:55Z

Thanks @ruflin for pointing out that this validation applies to other Kibana assets as well.

@mtojek I have restructured #65 to be more of a meta issue, covering the various types of Kibana assets that need this type of ID validation + making room for multiple, follow-up PRs.

ycombinator · 2020-10-26T00:53:38Z

I'm not quite sure about the previous fix elastic/integrations#171 , which only covered dashboards. In the worst case we need to wrap up a similar fix to elastic/integrations#319 for visualization and searches

Yes, I will work on a PR today to the @elastic/integrations repo for fixing any visualization and saved search IDs.

[EDIT] @mtojek can you work on any corresponding changes to the migration script?

ycombinator · 2020-10-26T00:57:08Z

CONTRIBUTING.md

+
+Currently, the following placeholders are available:
+
+* `{PACKAGE_NAME}` - name of the package


ycombinator

LGTM.

ycombinator · 2020-10-26T02:48:21Z

I'm not quite sure about the previous fix elastic/integrations#171 , which only covered dashboards. In the worst case we need to wrap up a similar fix to elastic/integrations#319 for visualization and searches

Yes, I will work on a PR today to the @elastic/integrations repo for fixing any visualization and saved search IDs.

I started to create this PR but ran into an issue that needs some discussion. Please take a look: elastic/integrations#334 (comment) and continue the discussion on that issue (not here). Thanks!

mtojek · 2020-10-26T07:33:44Z

@mtojek I have restructured #65 to be more of a meta issue, covering the various types of Kibana assets that need this type of ID validation + making room for multiple, follow-up PRs.

Thank you for adjusting the issue. I wonder if we shouldn't include all these changes in a single PR. Otherwise we'll end up in few iterations on adjusting integrations and spec. Or I will wait with merging this one, until all packages are in a good shape in terms of dashboard names (to merge elastic-package dependency update).

EDIT:

I will try now, I suppose they should be correct now.

@mtojek can you work on any corresponding changes to the migration script?

Yes, that's on my TODO list.

Validate that dashboard IDs start with package names

6cc2335

mtojek requested a review from ycombinator October 21, 2020 10:44

mtojek self-assigned this Oct 21, 2020

mtojek added 3 commits October 21, 2020 12:51

Fix

f399a40

Fix: rename test function

6aac7ba

Fix

c307520

ycombinator reviewed Oct 21, 2020

View reviewed changes

code/go/internal/validator/folder_spec.go Show resolved Hide resolved

Add CONTRIBUTING

7e4e3ca

mtojek requested review from ruflin and exekias October 22, 2020 06:54

Unnecessary +

36817b6

exekias approved these changes Oct 22, 2020

View reviewed changes

mtojek requested a review from ycombinator October 22, 2020 14:37

ycombinator reviewed Oct 26, 2020

View reviewed changes

CONTRIBUTING.md

Currently, the following placeholders are available:

* `{PACKAGE_NAME}` - name of the package

Copy link

Contributor

ycombinator Oct 26, 2020

Choose a reason for hiding this comment

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

❤️

ycombinator approved these changes Oct 26, 2020

View reviewed changes

mtojek merged commit cdb8fbe into elastic:master Oct 26, 2020

mtojek mentioned this pull request Oct 28, 2020

Validate that Kibana asset IDs start with lowercase package names #65

Closed

3 tasks

ycombinator mentioned this pull request Oct 29, 2020

Validation: Kibana asset IDs should not contain -ecs suffix #71

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate that dashboard IDs start with package names #66

Validate that dashboard IDs start with package names #66

mtojek commented Oct 21, 2020 •

edited

Loading

elasticmachine commented Oct 21, 2020 •

edited

Loading

Build stats

ycombinator Oct 21, 2020

mtojek Oct 21, 2020

ycombinator Oct 21, 2020

mtojek Oct 21, 2020

ycombinator Oct 21, 2020

mtojek Oct 21, 2020 •

edited

Loading

ycombinator Oct 21, 2020

exekias Oct 22, 2020

exekias Oct 22, 2020

mtojek Oct 22, 2020

mtojek commented Oct 22, 2020 •

edited

Loading

ruflin commented Oct 22, 2020

mtojek commented Oct 22, 2020

ruflin commented Oct 22, 2020

exekias left a comment

mtojek commented Oct 22, 2020

ycombinator commented Oct 26, 2020

ycombinator commented Oct 26, 2020 •

edited

Loading

ycombinator Oct 26, 2020

ycombinator left a comment

ycombinator commented Oct 26, 2020

mtojek commented Oct 26, 2020 •

edited

Loading


		Currently, the following placeholders are available:

		* `{PACKAGE_NAME}` - name of the package

Validate that dashboard IDs start with package names #66

Validate that dashboard IDs start with package names #66

Conversation

mtojek commented Oct 21, 2020 • edited Loading

elasticmachine commented Oct 21, 2020 • edited Loading

💚 Build Succeeded

Build stats

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek commented Oct 22, 2020 • edited Loading

ruflin commented Oct 22, 2020

mtojek commented Oct 22, 2020

ruflin commented Oct 22, 2020

exekias left a comment

Choose a reason for hiding this comment

mtojek commented Oct 22, 2020

ycombinator commented Oct 26, 2020

ycombinator commented Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

ycombinator left a comment

Choose a reason for hiding this comment

ycombinator commented Oct 26, 2020

mtojek commented Oct 26, 2020 • edited Loading

mtojek commented Oct 21, 2020 •

edited

Loading

elasticmachine commented Oct 21, 2020 •

edited

Loading

mtojek Oct 21, 2020 •

edited

Loading

mtojek commented Oct 22, 2020 •

edited

Loading

ycombinator commented Oct 26, 2020 •

edited

Loading

mtojek commented Oct 26, 2020 •

edited

Loading