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 nested type support to go code generator #1254

Merged
merged 6 commits into from
Apr 8, 2021

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented Feb 4, 2021

Introducing type: nested will require handling of nested fields be defined, else the go code generator will fail.

@ebeahan ebeahan self-assigned this Feb 4, 2021
@ebeahan ebeahan mentioned this pull request Feb 4, 2021
3 tasks
@ebeahan ebeahan marked this pull request as ready for review February 5, 2021 14:59
@ebeahan ebeahan changed the title Add nested type to go generator Add additional object types to go generator Feb 8, 2021
@ebeahan ebeahan requested a review from a team February 8, 2021 21:14
@ebeahan
Copy link
Member Author

ebeahan commented Feb 8, 2021

Adjusted this PR to add both nested as well as flattened types to the generator's mapping logic.

@marc-gr
Copy link
Contributor

marc-gr commented Feb 9, 2021

Is it possible to add some test cases un the tests folder for this addition?

@andrewstucki
Copy link
Contributor

@ebeahan @marc-gr I know that there is a plan to eventually strip out the go generators from the ECS repo, does it make sense to do so now? I believe that the only elastic project using the go code is packetbeat, and I think it's pinned to an older version. Otherwise, if we're planning on keeping these around for awhile, maybe we could add the binary type, making it []byte would allow the default json marshaler to automatically do base64 encoding.

@ebeahan
Copy link
Member Author

ebeahan commented Feb 9, 2021

@andrewstucki @marc-gr Thanks for taking a look. These are some good discussion points, but I don't want to expand the original scope here too much. Any objections to the proposed mapping for nested and flattened?

Is it possible to add some test cases un the tests folder for this addition?

Ideally, we'd have something in place for testing the go generator portions of the repo, but we don't today. I don't think we should add that testing framework into the ECS repo right now since...

I know that there is a plan to eventually strip out the go generators from the ECS repo, does it make sense to do so now?

...we definitely want to remove the go generator and eventually the other golang dependencies from this repo.

I have no issue with pulling it out, but I'm also not familiar with where it's a dependency. A team or individual would need to take ownership of setting up the new repo; I don't see this as something the ECS team would be best to maintain (right now at least).

@marc-gr
Copy link
Contributor

marc-gr commented Feb 10, 2021

Ideally, we'd have something in place for testing the go generator portions of the repo, but we don't today. I don't think we should add that testing framework into the ECS repo right now

👍

@ebeahan ebeahan removed the 1.9.0 label Feb 18, 2021
@ebeahan ebeahan changed the title Add additional object types to go generator Add nested type support to go code generator Mar 16, 2021
@ebeahan
Copy link
Member Author

ebeahan commented Mar 16, 2021

Will focus on solely on adding nested type support here. Separate PR opened to add support for flattened: #1302.

@ebeahan ebeahan force-pushed the add-nesting-type-support branch 2 times, most recently from 20803bf to 66eb494 Compare March 19, 2021 23:15
@ebeahan
Copy link
Member Author

ebeahan commented Mar 19, 2021

Took a shot at the improved implementation suggested in #1254 (comment).

@andrewkroh @andrewstucki would appreciate a look.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I think this is good, but I would find it very helpful if you could temporarily add some nested fields to demonstrate what the generated code looks like and then paste that into comment.

@ebeahan
Copy link
Member Author

ebeahan commented Apr 1, 2021

I generated some examples using some nested fields from the experimental schema: https://gist.github.com/ebeahan/2c3038b0accd1786f78d7d7f0b4353da

@ebeahan ebeahan force-pushed the add-nesting-type-support branch from 7645c6b to 9da4f06 Compare April 1, 2021 19:59
@andrewkroh
Copy link
Member

Thanks! It looks like the tag is missing the key name. It has

	// ELF object segment list.
	Segments []Segments `ecs:""`

where I expected

Segments []Segments `ecs:"segments"`

@ebeahan
Copy link
Member Author

ebeahan commented Apr 7, 2021

Thanks, @andrewkroh. I believe it's now addressed.

Updated the sample gist with the latest versions.

@ebeahan ebeahan added the 1.10.0 label Apr 7, 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.

4 participants