Npm: Forward warnings from npm just as Hint, not as Warning

[fviernau]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f471b7b) 67.16% compared to head (810bb27) 67.16%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8182   +/-   ##
=========================================
  Coverage     67.16%   67.16%           
  Complexity     2049     2049           
=========================================
  Files           357      357           
  Lines         17158    17158           
  Branches       2461     2461           
=========================================
  Hits          11525    11525           
  Misses         4611     4611           
  Partials       1022     1022           

Flag	Coverage Δ
funTest-docker	66.25% <ø> (ø)
funTest-non-docker	33.96% <ø> (ø)
test	36.98% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sschuberth]

I'm not sure whether I agree with the general rationale of this PR, which seems to boil down to "if the correct dependencies result can be constructed, there should be no analyzer issues at all".

Personally, I like the feature to get some additional info about the general code quality of the project when running the analyzer, even if strictly speaking that info is not required in the context of license compliance checks. (Also see the somewhat related #3317.)

But the info about deprecated dependencies or failed integrity checks could be useful in the context of security compliance checks. Users could create policy rules to forbid any deprecated NPM dependencies, or to not use any NPM dependencies from Git repos (but only those published to the NPM registry; though that could be implemented by looking at the provenance as well).

Maybe a compromise is to lower the severity of these issues to HINT?

[fviernau]

I'm interested in further opinions on this, maybe @tsteenbe @mnonnenmacher @MarcelBochtler?

Further on, maybe we can separately discuss deprecation warnings and failed integrity checks.

1. Deprecations warnings: I'm not agaist having deprecation warnings at all. However, I believe it's not the
   the analyzer's concern to warn about deprecated dependencies, outdated dependencies. I believe this would
   fit much better in an advisor component which checks dependency up-to-date-ness irrespective for any kind of
   package manager. This would also allow for writing policy rules on top of structured more strongly typed data as
   opposed to writing a rule based on parsing free text form issue message directly coming from a particular NPM version.
2. Integrity checks: to me I still feel that there is not really an underlying need, and we do this just because we can. The
   reason is that in large amounts of other places we do not care about integrity checks. For example, each resolved
   provenance which has been resolved via tag matching does not guarantee integrity. But there is no attempt of us to
   warn about.

A downgrade to hint would be an improvement in my view, while I personally prefer to not report these issues in the first place.

[sschuberth]

and we do this just because we can.

I freely admit that's the case. As we get this information as a by-product, we have kept it until now.

To me, the discussion also relates a bit to the semantics of the "Issues" tab in the web-app report, which is what most people seem to use. "Issues" is an awfully generic term, also see #4395. So far, I didn't bother to also see "random" issues reported by NPM there, as it seemed to fit the description. If we would rename that to something that makes more clear that the tab only lists the minimum amount of issues to solve prior to relying on the analyzer results, that would convince me more of the proposed change.

[fviernau]

I'd also be fine to drop this PR entirely. Depends on the consensus here..

[sschuberth]

I'd also be fine to drop this PR entirely. Depends on the consensus here..

And I'd also be fine with a merge if others approve... it's just that I currently do not wholeheartedly agree with the rationale.

[mnonnenmacher]

In my opinion we should at least lower the severity of those issues to hint as they are not related to ORT but we are just forwarding warnings from NPM. The deprecation warning I find useful in the context of the analyzer as its related to the dependency resolution. For the warning about the integrity checks I have no opinion as I don't know what causes it and what it really means.

[sschuberth]

For the warning about the integrity checks I have no opinion as I don't know what causes it and what it really means.

IIUC, it means that for packages coming from a GitHub repo instead of the NPM registry the integrity of the source archive (i.e. the expected hash) cannot be verified, as there is no source archive in that case.

[mnonnenmacher]

For the warning about the integrity checks I have no opinion as I don't know what causes it and what it really means.

IIUC, it means that for packages coming from a GitHub repo instead of the NPM registry the integrity of the source archive (i.e. the expected hash) cannot be verified, as there is no source archive in that case.

Ok, I'm not sure if this information is relevant as I'm never developing with NPM, but I think I would keep the warnings and just lower the severity to hint.

[fviernau]

I've changed this according to the majority decision.

[sschuberth]

I've changed this according to the majority decision.

Another idea just came to my mind: One thing that we could additionally / alternatively do is to change the name of the issue's source for issues that are just forwarded from the NPM CLI to something else than managerName, e.g. "NPM CLI". That could help to filter out those issues by source if a user is not interested in them.

[fviernau]

@sschuberth @mnonnenmacher can we merge this?

[sschuberth]

@sschuberth @mnonnenmacher can we merge this?

@fviernau I was waiting for you to comment on my additional proposal, but I guess we can merge this as a step into the right direction. I'll probably propose some small follow-ups, like adding a comment why we lower the severity here, and doing the same for errors maybe, too.