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

Support field customization #179

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jpage-godaddy
Copy link

Support customization of fields on a per-request basis

Support customization of fields on a per-request basis
Copy link

cla-checker-service bot commented Mar 14, 2024

💚 CLA has been signed

@jpage-godaddy
Copy link
Author

We've reportedly signed a contributor agreement; I'm not sure how to get it to be reflected here.

@jpage-godaddy
Copy link
Author

Going to try closing and reopening

@trentm
Copy link
Member

trentm commented Mar 26, 2024

cla/check

@trentm
Copy link
Member

trentm commented Mar 26, 2024

@jpage-godaddy Regarding the CLA, my understanding is that you or someone at GoDaddy will need to send an email to cla-questions@elastic.co asking to add you to your corporate CLA. Per this on https://www.elastic.co/contributor-agreement

If you are a company with an existing contributor license agreement and would like to add or change the list of employees that are authorized to contribute, please email us at cla-questions@elastic.co.

I don't know how to verify on my end whether GoDaddy has a corporate CLA. Let me know if you hit a wall here and I can start asking around.

@trentm
Copy link
Member

trentm commented Mar 26, 2024

Running npm run lint:fix will fix the "lint" check failure.

You'll have to avoid the ??= syntax in the test to maintain the support back to node v10 that we currently support:

/home/runner/work/ecs-logging-nodejs/ecs-logging-nodejs/packages/ecs-morgan-format/test/basic.test.js:249
        fields.labels ??= {};
                      ^^^

SyntaxError: Unexpected token '??='
    at wrapSafe (internal/modules/cjs/loader.js:1029:16)
    ...

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The https://github.com/elastic/ecs-logging-nodejs/blob/main/docs/morgan.asciidoc#reference docs file should get an update for this. I can help with that if you like -- no need for you to learn asciidoc just for this.

Another request/comment below.

packages/ecs-morgan-format/index.js Outdated Show resolved Hide resolved
- Add docs
- Change signature of customization function
- Change test to use old JavaScript syntax
@jpage-godaddy
Copy link
Author

@trentm thanks for the review. I've made some updates and am ready for another pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants