-
Notifications
You must be signed in to change notification settings - Fork 88
Validate kibana tags duplicates #1042
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
Validate kibana tags duplicates #1042
Conversation
code/go/internal/validator/spec.go
Outdated
| {fn: semantic.ValidateIntegrationPolicyTemplates, types: []string{"integration"}}, | ||
| {fn: semantic.ValidatePipelineTags, types: []string{"integration"}, since: semver.MustParse("3.6.0")}, | ||
| {fn: semantic.ValidateStaticHandlebarsFiles, types: []string{"integration", "input"}}, | ||
| {fn: semantic.ValidateKibanaTagDuplicates, types: []string{"integration"}}, |
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.
@jsoriano
content packages also have a tags.yml file, so perhaps we should check if inside the file there are duplicates? this is done inside this function now..
regarding the version, i guess this will "break" more than one integration, as for example crowdstrike is having the duplicate tags. should we address this fix at the integration repository and add this validation for all, or add it from 3.6.0 as a breaking change?
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.
contentpackages also have atags.ymlfile, so perhaps we should check if inside the file there are duplicates? this is done inside this function now..
Yes please, let's check this in all cases. We can probably enable this check for all types, if the files don't exist it doesn't do anything, right?
| {fn: semantic.ValidateKibanaTagDuplicates, types: []string{"integration"}}, | |
| {fn: semantic.ValidateKibanaTagDuplicates}, |
regarding the version, i guess this will "break" more than one integration, as for example crowdstrike is having the duplicate tags. should we address this fix at the integration repository and add this validation for all, or add it from 3.6.0 as a breaking change?
I think this should to be reviewed in existing integrations, so I would check this in all packages, or at least in all versions where tags.yml is supported (it is supported since 2.10.0), what would be equivalent.
As escape hatch, I would assign it an error code, so it can be skipped if needed.
We would need to check in any case how many packages in the integrations repository are affected.
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.
looking into the integrations test, as it was expected, mostly all are failing because of the Security Solution exception. Inline with my comment on elastic/elastic-package#3163 (comment)
i am guessing at least all the json corresponding to the security solution tag can be removed (safely)
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.
i am guessing at least all the json corresponding to the security solution tag can be removed (safely)
Yes, but we would also need to remove the references to these ids from the dashboards, right?
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.
yes, the json and the reference to that id in the dasboards or any other object where they are referenced
jsoriano
left a comment
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.
Overall approach LGTM 👍
code/go/internal/validator/semantic/validate_kibana_tag_duplicates.go
Outdated
Show resolved
Hide resolved
code/go/internal/validator/semantic/validate_kibana_tag_duplicates.go
Outdated
Show resolved
Hide resolved
code/go/internal/validator/semantic/validate_kibana_tag_duplicates.go
Outdated
Show resolved
Hide resolved
code/go/internal/validator/spec.go
Outdated
| {fn: semantic.ValidateIntegrationPolicyTemplates, types: []string{"integration"}}, | ||
| {fn: semantic.ValidatePipelineTags, types: []string{"integration"}, since: semver.MustParse("3.6.0")}, | ||
| {fn: semantic.ValidateStaticHandlebarsFiles, types: []string{"integration", "input"}}, | ||
| {fn: semantic.ValidateKibanaTagDuplicates, types: []string{"integration"}}, |
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.
contentpackages also have atags.ymlfile, so perhaps we should check if inside the file there are duplicates? this is done inside this function now..
Yes please, let's check this in all cases. We can probably enable this check for all types, if the files don't exist it doesn't do anything, right?
| {fn: semantic.ValidateKibanaTagDuplicates, types: []string{"integration"}}, | |
| {fn: semantic.ValidateKibanaTagDuplicates}, |
regarding the version, i guess this will "break" more than one integration, as for example crowdstrike is having the duplicate tags. should we address this fix at the integration repository and add this validation for all, or add it from 3.6.0 as a breaking change?
I think this should to be reviewed in existing integrations, so I would check this in all packages, or at least in all versions where tags.yml is supported (it is supported since 2.10.0), what would be equivalent.
As escape hatch, I would assign it an error code, so it can be skipped if needed.
We would need to check in any case how many packages in the integrations repository are affected.
code/go/internal/validator/semantic/validate_kibana_tag_duplicates.go
Outdated
Show resolved
Hide resolved
| require.Len(t, errs, 1) | ||
| require.Contains(t, errs[0].Error(), "duplicate tag name 'tag1' found in kibana/tags.yml") | ||
| require.Len(t, tagMap, 2) | ||
| require.Contains(t, tagMap, "tag1") | ||
| require.Contains(t, tagMap, "tag2") |
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.
Nit. Even on failure it is nice to return as much information as possible, use assert and conditions when the test can continue if the assertion fails.
| require.Len(t, errs, 1) | |
| require.Contains(t, errs[0].Error(), "duplicate tag name 'tag1' found in kibana/tags.yml") | |
| require.Len(t, tagMap, 2) | |
| require.Contains(t, tagMap, "tag1") | |
| require.Contains(t, tagMap, "tag2") | |
| if assert.Len(t, errs, 1) { | |
| assert.Contains(t, errs[0].Error(), "duplicate tag name 'tag1' found in kibana/tags.yml") | |
| } | |
| assert.Len(t, tagMap, 2) | |
| assert.Contains(t, tagMap, "tag1") | |
| assert.Contains(t, tagMap, "tag2") |
…alidate_kibana_tag_duplicates_test.go for consistency
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#16597 |
code/go/internal/validator/semantic/validate_kibana_tag_duplicates.go
Outdated
Show resolved
Hide resolved
code/go/internal/validator/semantic/validate_kibana_tag_duplicates_test.go
Outdated
Show resolved
Hide resolved
| | [SVR00004] | Visualization by value | | ||
| | [SVR00005] | Minimum Kibana version | | ||
| | [SVR00006] | Processor tag is required | | ||
| | [SVR00007] | Kibana tag is duplicate | |
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 could be some conflicts with this other PR that is also adding the same validation code errors: #1038
💚 Build Succeeded
History
|
What does this PR do?
This PR implements validation into kibana tags assets. When a tag is defined inside the
tags.ymlis duplicated and an asset is created when exporting the dashboard withelastic-package.Why is it important?
Avoid duplicate tags at kibana UI, as .json assets when is already declared at
tags.ymlChecklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues