Skip to content

Conversation

@mtojek
Copy link
Contributor

@mtojek mtojek commented Jun 3, 2022

Meta-issue: elastic/package-spec#319

Changes:

  • render the table with fields in docs
  • import fields from ECS

Related:
elastic/package-spec#350

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 3, 2022

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-06-07T14:53:18.648+0000

  • Duration: 31 min 13 sec

Test stats 🧪

Test Results
Failed 0
Passed 720
Skipped 0
Total 720

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 3, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (30/30) 💚
Files 64.22% (70/109) 👍
Classes 58.278% (88/151) 👍
Methods 46.256% (278/601) 👎 -1.165
Lines 31.279% (2532/8095) 👎 -1.07
Conditionals 100.0% (0/0) 💚

@mtojek mtojek requested a review from a team June 3, 2022 15:00
@mtojek mtojek marked this pull request as ready for review June 3, 2022 15:00
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 some comments about possible future refactors that may be done to support multiple package types.

My only concern is about cases where we may be accepting that we cannot find the package root.

v.Schema, err = loadFieldsFromDir(fieldsDir)
if err != nil {
return nil, errors.Wrapf(err, "can't load fields for data stream (path: %s)", dataStreamRootPath)
return nil, errors.Wrapf(err, "can't load fields for data stream (path: %s)", fieldsParentDir)
Copy link
Member

Choose a reason for hiding this comment

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

Nit. This message is also now about input packages, right? We may need to change this error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if err != nil {
return nil, errors.Wrap(err, "can't find package root")
}
if !found {
Copy link
Member

Choose a reason for hiding this comment

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

Could this hide problems with malformed packages?

On what cases is it ok to not find the package root? Could you add a comment here about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment, let me know what do you think.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we can revisit this if found to be problematic at some point. Would it make sense to add a debug message here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

if err != nil {
return nil, errors.Wrap(err, "can't find package root")
}
if !found {
Copy link
Member

Choose a reason for hiding this comment

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

Ok, we can revisit this if found to be problematic at some point. Would it make sense to add a debug message here?

@mtojek mtojek merged commit dd908a8 into elastic:main Jun 7, 2022
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.

3 participants