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

Govendor ECS 1.0.1 #12317

Merged
merged 4 commits into from
Jun 5, 2019
Merged

Govendor ECS 1.0.1 #12317

merged 4 commits into from
Jun 5, 2019

Conversation

cwurm
Copy link
Contributor

@cwurm cwurm commented May 28, 2019

Updates vendor dependency to ECS 1.0.1. Field definitions were already imported in #12284.

@andrewkroh
Copy link
Member

It looks like this is what controls the ecs.version value and now many of the golden files in Filebeat need to be updated. Perhaps a more sustainable fix would be to ignore the value of the ecs.version field in the Filebeat module tests.

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.

LGTM

@webmat
Copy link
Contributor

webmat commented May 29, 2019

Oh wait. You'll need to re-generate the golden files

@webmat
Copy link
Contributor

webmat commented May 29, 2019

Brain waking up. +1 to what @andrewkroh suggests.

☕️🏃

@ruflin
Copy link
Member

ruflin commented May 29, 2019

I'm on the fence about this one as we also use this to generate the examples. I kind of like that we have it in all examples but we could also remove it.

For now I would generate it and follow up if we remove it or not. You will also have to update the generated files for metricbeat.

@cwurm
Copy link
Contributor Author

cwurm commented May 30, 2019

I think I'd want to ignore ecs.version. If we keep checking it, we have countless (hundreds? thousands?) places to update every time we want to import even a bugfix release of ECS.

@cwurm cwurm requested a review from a team as a code owner May 30, 2019 20:18
@cwurm
Copy link
Contributor Author

cwurm commented May 30, 2019

You will also have to update the generated files for metricbeat.

I don't see any of the golden files for Metricbeat containing ecs.version? The build for Metricbeat did not fail either, just Filebeat OSS and Filebeat X-Pack.

I've pushed a commit ignoring ecs.version to see if that fixes the build. Let me know if that's the way we want to go, or if you prefer another way.

@ruflin
Copy link
Member

ruflin commented Jun 3, 2019

Ok, SGTM. You are correct we don't have it in the metricbeat golden files.

One problem with the above change is that it goes green on CI but the next time someone runs generate it will remove the ECS fields and create a diff. Suggestion: Merge this PR as is but have a follow up PR which updates all the generated files?

@webmat
Copy link
Contributor

webmat commented Jun 4, 2019

Or run with INTEGRATION_TESTS=1 GENERATE=1 and update this PR again?

@cwurm
Copy link
Contributor Author

cwurm commented Jun 4, 2019

Or run with INTEGRATION_TESTS=1 GENERATE=1 and update this PR again?

What's the command to run with that?

@webmat
Copy link
Contributor

webmat commented Jun 4, 2019

Ah sorry. From one non-Filebeat developer to another 😂

INTEGRATION_TESTS=1 nosetests tests/system/test_modules.py

@webmat webmat closed this Jun 4, 2019
@webmat webmat reopened this Jun 4, 2019
@webmat
Copy link
Contributor

webmat commented Jun 4, 2019

Oh no, accidentally closed the PR, sorry.

I think I had Cmd or shift stuck, when hitting enter (I'm turning my laptop in for a keyboard repair this Friday, true story)

@webmat
Copy link
Contributor

webmat commented Jun 4, 2019

Make sure your Python venv is set & all that. If you want the full slew of commands to build Filebeat & get the venv right, hit me up when you see me online

@cwurm
Copy link
Contributor Author

cwurm commented Jun 4, 2019

Or run with INTEGRATION_TESTS=1 GENERATE=1 and update this PR again?

Yeah, let's do it all in one so the repo is not inconsistent. I'll push a commit with the changes to the generated files in a minute.

@cwurm cwurm requested review from a team as code owners June 4, 2019 21:53
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.

LGTM, thanks so much for seeing this through :-)

@webmat
Copy link
Contributor

webmat commented Jun 5, 2019

Metricbeat in Travis seems completely unrelated. Some dep install problem. Re-started them.

Also, Jenkins, test this

@cwurm cwurm merged commit 6708772 into elastic:master Jun 5, 2019
@cwurm cwurm deleted the ecs_101_govendor branch June 5, 2019 16:11
andrewvc pushed a commit to andrewvc/beats that referenced this pull request Jun 12, 2019
Updates vendor dependency to ECS 1.0.1. Field definitions were already imported in elastic#12284. Removes `ecs.version` from Filebeat golden files.
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.

4 participants