Skip to content
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

Add validation for dataset type #501

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Jun 10, 2020

The valid dataset types at the moment are logs, metrics and events. events will be removed in the future but at the moment it is still used in the endpoint package. As soon as this is removed, the code here will be cleaned up.

With recent changes to the code, the datasets were not validated anymore during build or running the registry. This is now changed by adding validate for datasets every time a package is created.

Part of #478

@ruflin ruflin self-assigned this Jun 10, 2020
@elasticmachine
Copy link

elasticmachine commented Jun 10, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #501 updated]

  • Start Time: 2020-06-10T07:53:38.528+0000

  • Duration: 10 min 45 sec

The valid dataset types at the moment are logs, metrics and events. events will be removed in the future but at the moment it is still used in the endpoint package. As soon as this is removed, the code here will be cleaned up.

With recent changes to the code, the datasets were not validated anymore during build or running the registry. This is now changed by adding validate for datasets every time a package is created.
@ruflin ruflin force-pushed the validate-dataset-types branch from dc2d3bd to 59b81eb Compare June 10, 2020 06:56
@ruflin ruflin requested a review from mtojek June 10, 2020 06:56
@ruflin
Copy link
Contributor Author

ruflin commented Jun 10, 2020

@jonathan-buttner Would be great to get your feedback on this one as the endpoint package will need an update.

util/dataset.go Outdated Show resolved Hide resolved
util/dataset.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
util/dataset.go Outdated
@@ -24,6 +24,13 @@ const (
DirIngestPipeline = "ingest-pipeline"
)

var ValidTypes = map[string]interface{}{
"logs": nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause problems in the future if someone would like to check if the value logs exists. They would get a nil value :) Maybe bool instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went an other approach and made the second part a String which is something we could also show in a UI if needed. Thinking of the case that someday we might have a /types endpoint if someone wants to know all the available dataset types.

@ruflin ruflin requested a review from mtojek June 10, 2020 07:53
@@ -24,6 +24,13 @@ const (
DirIngestPipeline = "ingest-pipeline"
)

var validTypes = map[string]string{
"logs": "Logs",
Copy link
Contributor

Choose a reason for hiding this comment

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

For string-to-string enums there is a different approach recommended: stringer (see: https://godoc.org/golang.org/x/tools/cmd/stringer).

We can leave it as, it's not a full blown enum

@ruflin ruflin merged commit 314263e into elastic:master Jun 10, 2020
@ruflin ruflin deleted the validate-dataset-types branch June 10, 2020 08:55
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