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

Remove the Go library and the code generating it. #628

Closed
wants to merge 3 commits into from

Conversation

webmat
Copy link
Contributor

@webmat webmat commented Nov 22, 2019

Elastic is in the process of releasing ECS logging formatters and libraries for
multiple languages. Each of them will consume ECS artifacts as needed, to generate
the code & so on. None of them will be hosted directly in the ECS repo itself,
however.

This PR removes the Golang library, as recently discussed, @andrewkroh. Happy
to hold off a bit, until a new repo for it has been created.

Issues to carry over to that new repo: #603, #343

@webmat webmat self-assigned this Nov 22, 2019
@webmat webmat added the cleanup label Nov 22, 2019
@webmat webmat mentioned this pull request Nov 22, 2019
4 tasks
@ruflin
Copy link
Contributor

ruflin commented Nov 25, 2019

@webmat I think the logging library and this go code are two different things. It might be used for a go library but can be used for other things too. Be aware, this is a breaking change in case someone depends on it.

Could you share some background on why these don't belong into the ECS repo and where they should land in the future? I would prefer to keep ECS "things" together in one repo and only scale out when we really have to.

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

Yes, it's true that this isn't one of the upcoming logging formatters per se. Part of the logging formatter initiative is also focusing on creating actual libraries like this one. Case in point: the new ecs-dotnet.

Having ECS libraries specific to the languages people use is a thing that makes sense for generating and consuming ECS events. However for obvious reasons, these language libraries can't all be built in this repo. I can't deal with all the languages at the same time here, especially if I don't maintain the libraries themselves.

Since none of these libraries have anything to do with publishing the schema per se, I think none of them actually belong here. It was fine to start with the Golang one in this repo, because it was the simplest way to accomplish that at the time, and we had other Golang things in here. But now it's time to make the separation IMO.

I'm happy to wait a bit on this one if there's a need, but I don't think moving this library is going to be a big deal. The two issues linked in the body seem to indicate that this library isn't actually ready for prime time: it looks like it's still at the experimental stage, and issues aren't being addressed.

Note that for CI, I've started talking to the infra team about setting up multiple related projects, so one build here could trigger builds of other dependent projects.

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

@ruflin
Copy link
Contributor

ruflin commented Nov 25, 2019

I think it wouldn't be a technical problem to have all the libs also in the spec folder. For me the strongest argument to have it separate is the release cycles. As far as I understand, the logging library have their own release versions?

For the Golang files they are tied to the schema as far as I can tell. Updating it in a single PR instead of having to go to a second repo sounds beneficial to me.

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

But if we push that to the extreme, are you saying the .Net library should move here as well? If not, why would we have the Golang library here and not the .Net one?

There's no material difference between the two, IMO.

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

To be clear, I'm conscious of the fact that this will require a few more manual steps to build the code libraries by being outside this repo. But we can automate this as needed.

@ruflin
Copy link
Contributor

ruflin commented Nov 25, 2019

I think so, yes. It would for example solve this "version" directory: https://github.com/elastic/ecs-dotnet/tree/master/src/Specification

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

And what about when there's a Java library? Should this repo require Python, Golang, .Net and Java just to run make? 😬

I would much rather have this repo's CI reuse a Docker artifact from these other repositories, to ensure things still work on every PR. But there's no need to have up to date artifacts for every commit to master for all of these libraries. These can be triggered once, when an ECS release is out.

@webmat
Copy link
Contributor Author

webmat commented Nov 25, 2019

On the "version" directory in ecs-dotnet: actually having all versions at the same time in this repo is a benefit that's enabled by this separation. I don't see it as a problem.

@ruflin
Copy link
Contributor

ruflin commented Nov 27, 2019

We consolidated repos in the past, for example getting all Beats into one repo to move faster which I think was very beneficial. But one or many repos, both is going to work. Not going to further push back on this.

I don't think this PR should be merged before the new code exists somewhere in a go repo.

@gen0cide
Copy link

gen0cide commented Apr 8, 2020

Not asking for an endorsement, but just so you can have peace of mind, I published a Go code generator for ECS:

https://github.com/gen0cide/ecsgen

@webmat
Copy link
Contributor Author

webmat commented Apr 8, 2020

@gen0cide Does it use the Go code from the ECS repo?

@gen0cide
Copy link

Nope! Its goal is to follow convention and generate code based on the generated ECS YML. (In my case, Flat YML).

It parses the YML into an AST, then generators walk/enumerate the AST to generate code. Right now it supports Go, but working on an Avro schema generator as well.

@ebeahan
Copy link
Member

ebeahan commented Jun 22, 2021

Separating this code from elastic/ecs is still something in the works (#630), but I'm closing this PR as it's been inactive for some time.

@ebeahan ebeahan closed this Jun 22, 2021
@ebeahan ebeahan removed 8.0.0 cleanup ready Issues we'd like to address in the future. labels Jun 22, 2021
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.

4 participants