dry-run: validate required fields #507
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
|
util/package.go
Outdated
| } | ||
|
|
||
| // validateRequiredFields method loads fields from all files and checks if required fields are present. | ||
| func validateRequiredFields(datasetPath string) error { |
There was a problem hiding this comment.
Shouldn't this be part of the Dataset validation. Like this also now datasetBasePath has to be passed in.
There was a problem hiding this comment.
Fixed.
BTW I removed the constraint for not having - (dash) in dataset names. We have some in the aws integration.
There was a problem hiding this comment.
Hm, this is not good because it means the - will end up in the index name which breaks the indexing strategy. Or do I miss something here?
There was a problem hiding this comment.
aws integration:
ec2-logs
ec2-metrics
elb-logs
elb-metrics
s3_daily_storage
s3_request
I understand that we need to rename ones with - (dash). Is the underscore safe? Should we switch from dash to underscore in integrations?
Also, I didn't get the point why does - breaks the indexing strategy and _ doesn't. Both looks like same special characters. Is the indexing strategy correctly defined?
There was a problem hiding this comment.
_ are allowed. The indexing strategy uses the following patterns: logs-*-*. Because of this if there is a - in the dataset name, it will assume it is the namespace. All other chars except - are allowed.
There was a problem hiding this comment.
To add a bit more flavour to this: A package could be allowed to have a -. It is only the dataset that is not allowed to have one. But as in most cases (not all) we autogenerate the dataset name (currently id) out of the package name, it is better they don't.
There was a problem hiding this comment.
Thanks for the reference to docs. I will prepare a set of PRs to rename all invalid datasets.
There was a problem hiding this comment.
|
@mtojek You also need to update the test packages :-( |
Changes: