-
Notifications
You must be signed in to change notification settings - Fork 19
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
Update subsets to conform with new format #18
Conversation
@@ -162,7 +162,7 @@ build-package: | |||
.PHONY: run-registry | |||
run-registry: check-go $(ROOT_DIR)/out $(MAGE_BIN) $(REG_DIR) build-package | |||
cd $(REG_DIR) && git pull | |||
cd $(REG_DIR) && PACKAGE_PATHS="$(PACKAGES_DIR),$(DEF_PACKAGES_DIR)" mage build && go run . | |||
cd $(REG_DIR) && PACKAGE_PATHS="$(PACKAGES_DIR),$(DEF_PACKAGES_DIR)" $(MAGE_BIN) build && go run . |
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.
oops thanks
@@ -78,6 +78,7 @@ define gen_mapping_files | |||
# remove unused files | |||
rm -r $(ROOT_DIR)/generated/$(1)/elasticsearch/6 | |||
rm $(ROOT_DIR)/generated/$(1)/ecs/ecs_nested.yml | |||
rm $(ROOT_DIR)/generated/$(1)/ecs/subset/*/ecs_nested.yml |
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.
Eventually we might want to check these in because we could leverage them to mark which fields in the mapping are optional/required for the endpoint to populate right? We could just read in the generated subset file and look for some special fields?
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.
Yeah, it still generates the ecs_flat.yml file for each subset, I just haven't checked them in yet since it adds ~10k lines. The subsets would be an ideal place to put the optional/required option.
I'll go ahead and check in the subset generated files so we can start tracking them too.
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.
Sounds good thanks!
$(REAL_ECS_DIR): | ||
git clone --branch add-enabled-support https://github.com/jonathan-buttner/ecs.git $(REAL_ECS_DIR) | ||
git clone --branch subset-format-update https://github.com/marshallmain/ecs.git $(REAL_ECS_DIR) |
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.
👍
@@ -6,6 +6,7 @@ | |||
description: > | |||
Fields describing a stack frame. call_stack is expected to be an array where each array element represents a stack frame. | |||
reusable: | |||
order: 1 |
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.
These are related to the ecs refactoring that Mat did?
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.
Yeah, we should be able to remove these in the future when the ECS tooling automatically determines the order but for now they're required.
The changes in elastic/ecs#873 will introduce some breaking changes to the subset format in preparation for custom options which will be used to add annotations to individual fields. This PR updates our subsets to use this format, and updates the makefile to temporarily use a fork of the tooling that has the changes already. When the tooling changes get merged to ECS master we can simply replace the fork repo with the ECS repo in our makefile.
The changes in this PR can be verified by checking that the generated files have not changed, with the exception of the intermediate object field
process.parent
being added in a few places due to tooling changes.