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

refactor: use sophisticated logger #1129

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

refactor: use sophisticated logger #1129

wants to merge 12 commits into from

Conversation

jase88
Copy link

@jase88 jase88 commented Dec 4, 2023

fixes: #158

  • add node logger with npm package pino. See GitHub or Docs
  • Why pino?
    • one of the newer logging packages with very active community
    • node core member is maintainer
  • add --verbosity level with options
    • debug
    • error
    • fatal
    • warn (default)
    • info
    • trace
    • silent
  • all log streams are going through STDERR

Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
@jase88 jase88 requested a review from a team as a code owner December 4, 2023 19:39
Copy link
Member

@jkowalleck jkowalleck left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, @jase88 .
I had a quick review and found some remarks.

src/builders.ts Outdated Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
src/cli.ts Show resolved Hide resolved
src/cli.ts Outdated Show resolved Hide resolved
@jkowalleck jkowalleck added the enhancement New feature or request label Dec 4, 2023
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
@jase88
Copy link
Author

jase88 commented Dec 6, 2023

Thank you for the contribution, @jase88 . I had a quick review and found some remarks.

Thank you @jkowalleck. I have made some changes and am happy to receive further feedback

@@ -17,12 +17,14 @@ SPDX-License-Identifier: Apache-2.0
Copyright (c) OWASP Foundation. All Rights Reserved.
*/

import { existsSync } from 'node:fs'
import * as path from 'node:path'
Copy link
Member

Choose a reason for hiding this comment

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

the node: notation, this might not work on node14.0 -- which is a supported branch.
will add additional CI tests and have it tested

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@jkowalleck jkowalleck Dec 6, 2023

Choose a reason for hiding this comment

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

tests with node14.0.0 do not complete due to a transitive dependency that is not installable.

@jkowalleck
Copy link
Member

jkowalleck commented Dec 6, 2023

first of all, thanks you for the contribution.

Some (transitive) dependencies are not available for node14.0.0. see https://github.com/CycloneDX/cyclonedx-node-npm/actions/runs/7114640707/job/19369116439?pr=1129#step:4:2384
So the min-version of node would have to be adjusted. Meaning, this would be a breaking change. No worries, this is not a problem.
Anyway, I'd rather drop the support for color and such, than raising the deps.

I tried out your implementation, and here are the things i don't like:

  • the output is sent to STDOUT, not STDERROR (even though the code suggests otherwise)
  • the output is sent on program exit as a bulk, not in serial as soon as they are issued
  • I do not like the output of a timestamp

Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
@jase88
Copy link
Author

jase88 commented Dec 6, 2023

Some (transitive) dependencies are not available for node14.0.0. see https://github.com/CycloneDX/cyclonedx-node-npm/actions/runs/7114640707/job/19369116439?pr=1129#step:4:2384 So the min-version of node would have to be adjusted. Meaning, this would be a breaking change. No worries, this is not a problem. Anyway, I'd rather drop the support for color and such, than raising the deps.

so raising to node >=14.17.0 would be no problem? or even raise it to 16 (node 14 security support ended 7 months ago).

  • the output is sent to STDOUT, not STDERROR (even though the code suggests otherwise)

should be fixed by now.

  • the output is sent on program exit as a bulk, not in serial as soon as they are issued

couldn't figure out why, will have a closer look at the docs next days.

  • I do not like the output of a timestamp

you don't want to see any timestamp or you don't like the format of the timestamp?

@jase88
Copy link
Author

jase88 commented Dec 7, 2023

  • the output is sent on program exit as a bulk, not in serial as soon as they are issued

It is no bulk output at the end.

You can test it with some deferral like await new Promise(resolve => setTimeout(resolve, 20_000))

@jkowalleck
Copy link
Member

  • I do not like the output of a timestamp

you don't want to see any timestamp or you don't like the format of the timestamp?

Do not want to see any timestamps at all.

  • the output is sent on program exit as a bulk, not in serial as soon as they are issued

It is no bulk output at the end.

for my tests, it was. for a second of delay no output was sent at all, then all was sent as a bulk. sometimes right BEFORE the SBOM, sometimes right after the SBOM.
try it:

$ ./bin/cyclonedx-npm-cli.js --verbosity debug 
{
  "$schema": "http://cyclonedx.org/schema/bom-1.4.schema.json",
  "bomFormat": "CycloneDX",
  "specVersion": "1.4",
  "version": 1,
  ....
      "ref": "@oozcitak/url@1.0.4",
      "dependsOn": [
        "@oozcitak/infra@1.0.8",
        "@oozcitak/util@8.3.8"
      ]
    },
    {
      "ref": "@oozcitak/util@8.3.8"
    }
  ]
}[2023-12-07 12:02:20.950] DEBUG: options: {"verbosity":"debug","ignoreNpmErrors":false,"packageLockOnly":false,"omit":[],"flattenComponents":false,"shortPURLs":false,"specVersion":"1.4","outputFormat":"JSON","outputFile":"-","validate":true,"mcType":"application"}
[2023-12-07 12:02:20.951] DEBUG: packageFile: .../cyclonedx-node-npm/package.json
[2023-12-07 12:02:20.951] INFO: projectDir: .../cyclonedx-node-npm
[2023-12-07 12:02:20.951] DEBUG: detected a node_modules dir
[2023-12-07 12:02:20.953] DEBUG: BomBuilder > makeNpmRunner caused execSync "npm"
[2023-12-07 12:02:20.953] INFO: BomBuilder > detect NPM version ...
[2023-12-07 12:02:21.673] DEBUG: BomBuilder > detected NPM version '9.4.2'
[2023-12-07 12:02:21.673] INFO: BomBuilder > gather dependency tree ...
[2023-12-07 12:02:21.673] DEBUG: BomBuilder > npm-ls: run npm with ["ls","--json","--long","--all"] in '.../cyclonedx-node-npm'
[2023-12-07 12:02:23.267] INFO: BomBuilder > build BOM ...

@jkowalleck jkowalleck marked this pull request as draft December 7, 2023 11:18
Signed-off-by: jase88 <804836+jase88@users.noreply.github.com>
@jkowalleck jkowalleck changed the title feat: add logger refactor: use sophisticated logger Dec 8, 2023
@jkowalleck jkowalleck removed the enhancement New feature or request label Dec 8, 2023
@jkowalleck
Copy link
Member

see #1131

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.

rework logging/output
2 participants