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 ability to include external directory of schemas when running generator #494

Merged

Conversation

mcpate
Copy link
Contributor

@mcpate mcpate commented Jul 2, 2019

This adds the ability to pass a path to make (which is used by the generator target) to a directory where users might have their own schemas defined.

The use case this is solving for is when a user has custom schemas that are required due to being unable to map fields directly to ECS Core/Extended fields. This change allows them to run the following to have their schemas included in the output of make or make generate:

$> make INCLUDE=/path/to/dir/of/yml/files
or 
$> make generator INCLUDE=/path/to/dir/of/yml/files

If INCLUDE does not exist, everything works as per usual. If INCLUDE does not contain any yml files everything works as per usual. If INCLUDE contains yml files the keys/values are merged with those found in the schemas directory and the output is for both sets of values. If the user tries to include a yml file that will overwrite a key from the schemas directory, an exception is raised.

As mentioned above, the generator target was the focus here. Should this PR add this functionality to any other targets/scripts?

@mcpate mcpate self-assigned this Jul 2, 2019
@mcpate mcpate requested a review from webmat July 2, 2019 18:08
@webmat
Copy link
Contributor

webmat commented Jul 2, 2019

Thanks a lot for submitting this, @mcpate! This will be very useful. I can't review in depth at this time, but I can still answer your question:

The make generator target is for the "new" generator, the other targets generating files are to be considered legacy, mostly. So you don't need to worry about adding support elsewhere for now. If we determine we need to add support for this elsewhere, it'll be addressed as separate PRs.

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

One small tweak required to fix the tests and we can merge

scripts/tests/test_ecs_spec.py Outdated Show resolved Hide resolved
@mcpate mcpate requested a review from webmat July 8, 2019 19:03
@mcpate mcpate requested a review from webmat July 8, 2019 19:46
Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

All good!

Thanks a lot for submitting this :-)

@nick-george
Copy link
Contributor

nick-george commented Aug 1, 2019

Hi @mcpate,

Thanks very much for writing this and apologies for commenting on an already merged PR. Happy to take this discussion somewhere else if you like.

In my opinion, this feature would be more useful if it allowed for adding fields to schemas that already exist in the ECS. For example, add a new field to the 'source' object.

I have taken code from https://stackoverflow.com/questions/20656135/python-deep-merge-dictionary-data to make your feature do what I need. See below. Obviously this could be shortened somewhat, but I wanted to leave the stackoverflow answer unmodified.

I believe merging two ECS dictionaries is safe if they contain the same key, and I also believe that the user should be able to override any field within the ECS too.

Thanks for your time!
Nick

def deep_merge(source, destination):
    for key, value in source.items():
        if isinstance(value, dict):
            # get node or create one
            node = destination.setdefault(key, {})
            deep_merge(value, node)
        else:
            destination[key] = value

    return destination

def safe_merge_dicts(a, b):
    """Merges two dictionaries into one."""
    return deep_merge(a,b)

@webmat
Copy link
Contributor

webmat commented Oct 30, 2019

@nick-george Sorry, forgot to respond. A PR that would make it possible to do that is always welcome.

If you look at #497 you'll see it's actually something we'd like to flesh out, as time permits :-)

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