Skip to content

Replace --production by --omit=dev - #1120

Open
swaldmann wants to merge 1 commit into
SAP:masterfrom
swaldmann:production-omit-dev
Open

swaldmann wants to merge 1 commit into
SAP:masterfrom
swaldmann:production-omit-dev

Conversation

@swaldmann

@swaldmann swaldmann commented Jun 25, 2024 •

Copy link
Copy Markdown

Description

This PR replaces all occurrences of --production in an npm context with --omit=dev.

Currently you get these warnings when deploying MTA projects with the standard npm builder:
"npm warn config production Use --omit=dev instead"

The omit option was introduced with npm 8, so it's available in all supported versions.

Checklist

  • Code compiles correctly
  • Relevant tests were added (unit / contract / integration)
  • Relevant logs were added
  • Formatting and linting run locally successfully
  • All tests pass
  • UA review
  • Design is documented
  • Extended the README / documentation, if necessary
  • Open source is approved

@cla-assistant

cla-assistant Bot commented Jun 25, 2024 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@swaldmann
swaldmann marked this pull request as ready for review June 25, 2024 10:54
@swaldmann
swaldmann requested a review from goodboyws as a code owner June 25, 2024 10:54
@yutaoj

yutaoj commented Aug 8, 2024

Copy link
Copy Markdown
Contributor

this parameter "--production" is still used by Node v14 .

@swaldmann

Copy link
Copy Markdown
Author

Node 14 (and 16) are already end-of-life though. They shouldn't be used any more, as they won't even get patched. IMO you should drop support for them, as in the worst case this enables stakeholders using those outdated versions.

Even if Node 14 support has to be kept for some reason there should be a conditional to use the --omit=dev version for later Node versions. We really shouldn't show warnings for standard projects using a current LTS version just to accommodate to some version deprecated for years.

@jerome-benoit

jerome-benoit commented Aug 8, 2024 •

Copy link
Copy Markdown
Contributor

Even if Node 14 support has to be kept for some reason there should be a conditional to use the --omit=dev version for later Node versions. We really shouldn't show warnings for standard projects using a current LTS version just to accommodate to some version deprecated for years.

If you expect that repo to follow the most basic best current security practices or even SAP security policies, you will face disillusionment :) I've tried to push a bunch of security compliance PRs a year ago, most of them have been merged/taken over.

Dunno why such a critical piece in the SAP software supply chain can be left with known critical CVEs such as https://security-tracker.debian.org/tracker/CVE-2024-2961 several months ... or years.

@yutaoj

yutaoj commented Nov 8, 2024

Copy link
Copy Markdown
Contributor

MBT requires support for Node 14, and the Node 14 MBT Docker image is utilized by SAP Piper. Therefore, it cannot be replaced at this time.

@jerome-benoit

Copy link
Copy Markdown
Contributor

So critical components in the SAP software supply chain use unmaintained and cluttered by serious security flaws node.js version?
How can it be considered as a valid justification to continue to support them in another SAP software supply chain critical component?

@agiguere

Copy link
Copy Markdown

still there in 2025/2026, I hope this warning could be fixed so we can have a clean console log

@whydrae

whydrae commented Mar 20, 2026

Copy link
Copy Markdown

@kbarnold, could you please also approve this small one?

@littleamigo

littleamigo commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

@kbarnold, could you please also approve this small one?

I fully support to merge this soon to avoid more severe problems when this option is removed and no longer available in npm after it's been deprecated for a really long time. And, of course, I'd like to get rid of this annoying warning in the console as well. @kbarnold

@kbarnold

Copy link
Copy Markdown
Contributor

This looks good and I will review and approve it, but am currently busy with rebuilding the CI and release setup.

This branch has not been deployed

No deployments
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.

7 participants