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

[1.6] [Tooling] Add --exclude flag to Generator to support field removal testing (#1411) #1430

Merged
merged 1 commit into from
May 26, 2021
Merged

[1.6] [Tooling] Add --exclude flag to Generator to support field removal testing (#1411) #1430

merged 1 commit into from
May 26, 2021

Conversation

djptek
Copy link
Contributor

@djptek djptek commented May 26, 2021

Backports the following commits to 1.6:

@djptek djptek requested a review from a team May 26, 2021 10:04
@djptek
Copy link
Contributor Author

djptek commented May 26, 2021

I'm backporting this to 1.6 since the goal is to use --exclude flag for testing with e.g. Beats and that's the current version

@ebeahan @kgeller make sense if I backport to the intermediate versions too?

@kgeller
Copy link
Contributor

kgeller commented May 26, 2021

@djptek if we are going back to 1.6, then I think it makes sense to include in everything since

@djptek
Copy link
Contributor Author

djptek commented May 26, 2021

OK thx @kgeller 👍

@djptek djptek merged commit cedfcb8 into elastic:1.6 May 26, 2021
@ebeahan
Copy link
Member

ebeahan commented May 26, 2021

Where in Beats is using 1.6.0? More recent releases of Beats are using a more recent version of ECS.

For example, Beats 7.13.0 uses ECS 1.9.0: https://github.com/elastic/beats/blob/v7.13.0/libbeat/_meta/fields.ecs.yml#L2

It's possible certain modules may still be using an older version of ECS.

Regardless, if you're trying to target an older version of ECS when running the generator script, you should use the --ref flag:

python scripts/generator.py --ref v1.6.0 --exclude ./custom/fields/exclude.yml

The --ref flag will generate artifacts for the targeted version.

@ebeahan
Copy link
Member

ebeahan commented May 26, 2021

@djptek I was a bit late with my thoughts before you merged 😅

I don't think this change is necessary, and I propose we revert. Let me know if I'm misunderstanding what you're trying to accomplish.

@djptek
Copy link
Contributor Author

djptek commented May 26, 2021

Hi @ebeahan - the goal of the --exclude flag was to generate e.g. fields.ecs.yml for testing with e.g. Beats with 1 or more fields removed. Running this within the context of the current ECS version creates a file that is in sync with the latest version of ECS which is several versions ahead of Beats (currently ECS 1.6), so there are far more differences than just the one removed field and it is no longer possible to assess the impact of building beats with the one field removed, because everything else has changed. By Backporting, we can checkout e.g. ECS 1.6, exclude the field(s) and use the generated files in the context of Beats.

If you're not happy with this approach let me know and I'll revert the backports on elastic main while leaving them on my fork so I can still do the impact evaluation from there

Sorry, hadn't seen the --ref flag. That make more sense, I'll revert these and go with that.

djptek pushed a commit that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants