Skip to content

Honor data stream dataset if provided#283

Merged
ycombinator merged 9 commits intoelastic:masterfrom
ycombinator:bugfix-asset-test-runner-es-tmpl-name
Mar 9, 2021
Merged

Honor data stream dataset if provided#283
ycombinator merged 9 commits intoelastic:masterfrom
ycombinator:bugfix-asset-test-runner-es-tmpl-name

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

This PR fixes a bug in the asset test runner, related to Elasticsearch index template assets.

Prior to this PR, the asset test runner was assuming that Elasticsearch index templates installed by a package were always named <package type>-<package name>.<data stream name>.

However, a data stream may optionally define a dataset in its manifest.yml. If this field is defined, the installed index template name becomes <package type>-<dataset>. This PR checks for the optional dataset field and, if it exists, uses it to create the index template name.

@ycombinator ycombinator requested a review from mtojek March 9, 2021 17:11
Copy link
Copy Markdown
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Nothing blocking due the emergency nature of the PR.

Name string `config:"name" json:"name" yaml:"name"`
Title string `config:"title" json:"title" yaml:"title"`
Type string `config:"type" json:"type" yaml:"type"`
Dataset string `config:"dataset" yaml:"dataset"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: json?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LOL yes, moving too fast 🤦

indexTemplateName := fmt.Sprintf("%s-%s.%s", dsManifest.Type, pkgManifest.Name, dsManifest.Name)
var indexTemplateName string
if dsManifest.Dataset == "" {
indexTemplateName = fmt.Sprintf("%s-%s.%s", dsManifest.Type, pkgManifest.Name, dsManifest.Name)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I wonder if it shouldn't be data stream manifest's method: dataStreamManifest.IndexTemplateName()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, good idea. I will make this change.

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 9, 2021

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #283 updated

  • Start Time: 2021-03-09T18:15:53.640+0000

  • Duration: 20 min 47 sec

  • Commit: 0ea19e1

Test stats 🧪

Test Results
Failed 0
Passed 311
Skipped 1
Total 312

Trends 🧪

Image of Build Times

Image of Tests

@ycombinator ycombinator requested a review from mtojek March 9, 2021 17:20
Copy link
Copy Markdown
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

If linter doesn't complain, feel free to merge this PR.

@ycombinator ycombinator merged commit 3685022 into elastic:master Mar 9, 2021
@ycombinator ycombinator deleted the bugfix-asset-test-runner-es-tmpl-name branch March 9, 2021 18:39
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.

3 participants