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.

Sign up for GitHub

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

Jump to bottom

Introduce "external" property for fields #179

Merged
merged 6 commits into from
Jun 10, 2021
Merged

Introduce "external" property for fields #179

merged 6 commits into from
Jun 10, 2021

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Jun 1, 2021 •
edited
Loading

What does this PR do?

This PR introduces additional "external" property for fields.

Why is it important?

The property will be used by elastic-package to pull the definition from ECS repository.

Checklist

Related issues

@mtojek mtojek self-assigned this Jun 1, 2021
@mtojek mtojek changed the title Introduce source property for fields Introduce "source" property for fields Jun 1, 2021
@mtojek mtojek marked this pull request as ready for review June 1, 2021 08:51
@elasticmachine
Copy link

elasticmachine commented Jun 1, 2021 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #179 updated

  • Start Time: 2021-06-09T13:41:12.806+0000

  • Duration: 7 min 22 sec

  • Commit: 1d7b936

Trends 🧪

Image of Build Times

@mtojek mtojek requested review from ruflin and jsoriano June 1, 2021 08:53
@mtojek mtojek mentioned this pull request Jun 1, 2021
Closed
jsoriano
jsoriano approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Looks good, added only some questions.

code/go/internal/spec/statik.go Outdated Show resolved Hide resolved
versions/1/data_stream/fields/fields.spec.yml Show resolved Hide resolved
versions/1/data_stream/fields/fields.spec.yml Show resolved Hide resolved
@mtojek mtojek mentioned this pull request Jun 2, 2021
Closed
@ruflin
Copy link
Contributor

ruflin commented Jun 2, 2021

Do you have by chance already started on the actual elastic-package PR that will use this source field? Having the second PR already open would help to explain how the source field exactly works and if there is perhaps an alternative name.

If this PR is blocking to get you moving on the elastic-package implementation, please get it in. Over LGTM.

@mtojek
Copy link
Contributor Author

mtojek commented Jun 2, 2021

I can start drafting something aside and then we can decide on naming.

In general I need two spec PRs, this and one which adds "buildDependencies" property to the package manifest (see comment: #63 (comment)).

I will prepare the second spec PR and then switch to implementation. It might be tricky (and time consuming) to develop as I need to checkout ECS registry, so it would be helpful if we agree on the spec changes sooner than later. I'm also happy to review other ideas.

@mtojek mtojek marked this pull request as draft June 2, 2021 11:50
@mtojek mtojek mentioned this pull request Jun 2, 2021
Merged
2 tasks
ruflin
ruflin reviewed Jun 7, 2021
View reviewed changes
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Sorry for the late review comment, seems I didn't hit submit before ...

test/packages/good/data_stream/foo/fields/external-fields.yml Outdated
description: Event family
fields:
- name: category
source: ecs
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of a nitpick but I still want to mention it. The term source comes with a lot of baggage. It has some history in filebeat and is also used in ecs.

Do we expect any other source in the future? Or could we start with ecs: true or something like that for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative name: reference: ecs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we expect any other source in the future?

Yes, as described in #63 (comment) . We'd like to introduce similar fields schema for Agent's fields.

Alternative name: reference: ecs ?

I can adjust to reference. I wasn't aware of the history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed, although I believe this is more like personal preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, not happy with both names :-D Especially as source sounds technically more correct. Maybe it is just me that gets reminded of all the history here.

@mtojek mtojek changed the title Introduce "source" property for fields Introduce "reference" property for fields Jun 7, 2021
ruflin
ruflin reviewed Jun 8, 2021
View reviewed changes
versions/1/data_stream/fields/fields.spec.yml
@@ -78,9 +78,13 @@ spec:
type: string
examples:
- '^[a-zA-Z]$'
reference:
description: External source reference
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, both names are in here 🤦 source and reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option:

external: ecs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I renamed to external.

@andresrc
Copy link

andresrc commented Jun 8, 2021 •
edited
Loading

Sorry for the late comment and if this complicates things a bit more, but don't we need more information for the source?

E.g. if we are talking ECS, the author should select a version. In a more general case (assuming we have some file defining a field "library") we would need the URL to that "library" (and probably a content hash for reproducible builds)

Does this make sense?

@mtojek
Copy link
Contributor Author

mtojek commented Jun 8, 2021

Sorry for the late comment and if this complicates things a bit more, but don't we need more information for the source?

E.g. if we are talking ECS, the author should select a version. In a more general case (assuming we have some file defining a field "library") we would need the URL to that "library" (and probably a content hash for reproducible builds)

Does this make sense?

Yes, that's true. The ecs value is a reference to a dependency defined by package-scope property. Dependencies are part of this PR: #181

@mtojek mtojek marked this pull request as ready for review June 9, 2021 13:41
@mtojek mtojek requested a review from ruflin June 9, 2021 13:41
@mtojek mtojek changed the title Introduce "reference" property for fields Introduce "external" property for fields Jun 9, 2021
@mtojek mtojek merged commit 96d8ae4 into elastic:master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@jsoriano jsoriano jsoriano approved these changes

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mtojek @elasticmachine @ruflin @andresrc @jsoriano