feat: add support for targetting specific workspaces #1212

MalickBurger · 2024-08-23T11:58:48Z

jkowalleck

some things require chen. see my remarks.

also, please add proper unit tests

CONTRIBUTING.md

jkowalleck · 2024-08-25T09:46:54Z

src/cli.ts

@@ -166,6 +167,12 @@ function makeCommand (process: NodeJS.Process): Command {
    ).default(
      Enums.ComponentType.Application
    )
+  ).addOption(
+    new Option(


for the things we pass truth to npm, it is intended to mimic the CLI of npm - so that the CLI args are familiar to the users.

the proposed option workspaces here does not do so.

compare with npm ls:

List installed packages Usage: npm ls <package-spec> Options: [-a|--all] [--json] [-l|--long] [-p|--parseable] [-g|--global] [--depth <depth>] [--omit <dev|optional|peer> [--omit <dev|optional|peer> ...]] [--link] [--package-lock-only] [--no-unicode] [-w|--workspace <workspace-name> [-w|--workspace <workspace-name> ...]] [-ws|--workspaces] [--include-workspace-root] [--install-links] alias: list Run "npm help ls" for more info

src/builders.ts

MalickBurger · 2024-08-26T05:50:07Z

some things require chen. see my remarks.

also, please add proper unit tests

Happy to do the changes. I would think some integration tests would be required for this change as it is not really feasible to test it in isolation due to the logic effectively sitting on the npm ls side.

I agree with the requirement of testing. I spent some time initially before making the PR going through the existing unit and integration tests and it is not very clear to me how we should go about adding tests for new functionality. For example, I do not see tests for several of the CLI options (such as --short-PURLs).

Could you provide a bit of a guide regarding how we should lay out tests for new functionality going forward as well as a brief of how the existing integration tests work and how they are structured. I intend on making additional PRs in the future for other issues and features so this would be very beneficial for me (and I'm sure other community members).

jkowalleck · 2024-08-26T07:43:55Z

@MalickBurger, for adding additional integration tests,

you would add new setups here, if needed: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/demo
Then, add the appropriate npm ls ... run here: https://github.com/CycloneDX/cyclonedx-node-npm/blob/main/.github/workflows/npm-ls_demo-results.yml
have the npm ls ... result data gathered via a CI (Github action/workflow).
you can dispatch the action in your repo here: https://github.com/MalickBurger/cyclonedx-node-npm/actions by dispatching the workflow (you might need to enable workflows for your repo)
download the action artifacts and add them here: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results
remove unnecessary data via https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results/_duplicates-cleaner (might require a back-merge or rebase to pull the scripts in)

since you are planning on adding conditional parameters in the npm ls call, you need to incorporate this in the existing testing processes somehow.

caused by #1212 --------- Signed-off-by: Jan Kowalleck <[email protected]>

MalickBurger · 2024-09-16T14:30:28Z

@MalickBurger, for adding additional integration tests,

you would add new setups here, if needed: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/demo

Then, add the appropriate npm ls ... run here: https://github.com/CycloneDX/cyclonedx-node-npm/blob/main/.github/workflows/npm-ls_demo-results.yml

have the npm ls ... result data gathered via a CI (Github action/workflow).
you can dispatch the action in your repo here: https://github.com/MalickBurger/cyclonedx-node-npm/actions by dispatching the workflow (you might need to enable workflows for your repo)

download the action artifacts and add them here: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results

remove unnecessary data via https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results/_duplicates-cleaner (might require a back-merge or rebase to pull the scripts in)

since you are planning on adding conditional parameters in the npm ls call, you need to incorporate this in the existing testing processes somehow.

Looking at this, could you provide some testing support for an existing argument that is passed to npm ls by the plugin which I could then base my additional tests on? It would be easier and cleaner for a code maintainer to set these up so there is a standard to follow going forward for community members.

jkowalleck · 2024-09-29T18:41:29Z

@MalickBurger, for adding additional integration tests,

you would add new setups here, if needed: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/demo

Then, add the appropriate npm ls ... run here: https://github.com/CycloneDX/cyclonedx-node-npm/blob/main/.github/workflows/npm-ls_demo-results.yml

have the npm ls ... result data gathered via a CI (Github action/workflow).
you can dispatch the action in your repo here: https://github.com/MalickBurger/cyclonedx-node-npm/actions by dispatching the workflow (you might need to enable workflows for your repo)

download the action artifacts and add them here: https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results

remove unnecessary data via https://github.com/CycloneDX/cyclonedx-node-npm/tree/main/tests/_data/npm-ls_demo-results/_duplicates-cleaner (might require a back-merge or rebase to pull the scripts in)

since you are planning on adding conditional parameters in the npm ls call, you need to incorporate this in the existing testing processes somehow.

Looking at this, could you provide some testing support for an existing argument that is passed to npm ls by the plugin which I could then base my additional tests on? It would be easier and cleaner for a code maintainer to set these up so there is a standard to follow going forward for community members.

will do. just bare with me, it may take a while

jkowalleck · 2024-10-29T15:31:21Z

will do. just bare with me, it may take a while

I am currently working on a solution. Will ping as soon as I have something for you.

src/cli.ts

jkowalleck · 2024-10-31T09:53:58Z

@MalickBurger ,

I've overhauled the integration tests.
It would currently be enough to add your tests, showcasing that the intended parameters are sent to npm-ls. This could be done here: https://github.com/CycloneDX/cyclonedx-node-npm/blob/main/tests/integration/cli.args-pass-through.test.js

you might add something like

// region workspace
['workspace not supported npm 6', `6.${rMinor}.${rPatch}`, ['--workspace my-wsA', '--workspace my-wsB'], [...npm6ArgsGeneral]],
['workspace npm 7', `7.${rMinor}.${rPatch}`, ['--workspace my-wsA', '--workspace my-wsB'], [...npm7ArgsGeneral, '--workspace my-wsA', '--workspace my-wsB']],
['workspace npm 8', `8.${rMinor}.${rPatch}`, ['--workspace my-wsA', '--workspace my-wsB'], [...npm8ArgsGeneral, '--workspace my-wsA', '--workspace my-wsB']],
['workspace npm 9', `9.${rMinor}.${rPatch}`, ['--workspace my-wsA', '--workspace my-wsB'], [...npm9ArgsGeneral, '--workspace my-wsA', '--workspace my-wsB']],
['workspace npm 10', `10.${rMinor}.${rPatch}`, ['--workspace my-wsA', '--workspace my-wsB'], [...npm10ArgsGeneral, '--workspace my-wsA', '--workspace my-wsB']]
// endregion workspace

PS: i've also prepared demo data for additional integration tests. I might add them as soon as the basic pass-through tests are done

jkowalleck · 2024-12-11T14:35:52Z

@MalickBurger , i see you've rebased this feature branch recently.
Are planning on integrating the needed tests as described here ?

MalickBurger · 2024-12-11T14:47:52Z

@MalickBurger , i see you've rebased this feature branch recently. Are planning on integrating the needed tests as described here ?

Hi @jkowalleck, yes I have been busy recently but am getting back to this now. Will be posting updates soon.

Signed-off-by: MalickBurger <[email protected]>

Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck · 2024-12-24T14:22:00Z

src/cli.ts

+    ).default([], 'empty')
+  ).addOption(
+    new Option(
+      '-no-ws, --no-workspaces',


why is this needed?
which version of npm ls knowns this?

jkowalleck · 2024-12-24T14:24:25Z

src/cli.ts

+    )
+  ).addOption(
+    new Option(
+      '-wr, --include-workspace-root',


where does the -wr come from?

it is intended to use similar args like nom ls does, so adoption is easy.
which version of npm knows -wr here?

jkowalleck · 2024-12-24T14:25:49Z

tests/integration/cli.args-pass-through.test.js

+      // endregion workspace
+      // region include-workspace-root
+      ['workspace root not supported npm 6', `6.${rMinor}.${rPatch}`, ['-w', 'my-wsA', '--include-workspace-root'], [...npm6ArgsGeneral]],
+      ['workspace root npm 7', `7.${rMinor}.${rPatch}`, ['--workspace', 'my-wsA', '-w', 'my-wsB', '--include-workspace-root'], [...npm7ArgsGeneral, '--workspace=my-wsA', '--workspace=my-wsB', '--include-workspace-root=true']],


workspace root not supported npm 7

jkowalleck · 2024-12-24T14:32:57Z

src/cli.ts

@@ -52,6 +52,9 @@ interface CommandOptions {
  outputFile: string
  validate: boolean | undefined
  mcType: Enums.ComponentType
+  workspace: string[]


please move the worksapce-related options up so they appear right below the omit

jkowalleck · 2024-12-24T14:33:05Z

src/cli.ts

@@ -173,6 +176,22 @@ function makeCommand (process: NodeJS.Process): Command {
    ).default(
      Enums.ComponentType.Application
    )
+  ).addOption(


please move the worksapce-related options up so they appear right below the omit

jkowalleck · 2024-12-24T14:40:59Z

merged in the latest master, fixed some merge conflicts.
did another round of review. result: there are open questions and issues

MalickBurger requested a review from a team as a code owner August 23, 2024 11:58

MalickBurger force-pushed the feat/workspace-support branch from 4ac8c52 to a26d8da Compare August 23, 2024 12:16

jkowalleck requested changes Aug 25, 2024

View reviewed changes

jkowalleck mentioned this pull request Sep 4, 2024

tests: WS and project extra unused #1215

Merged

jkowalleck added a commit that referenced this pull request Sep 4, 2024

tests: WS and project extra unused (#1215)

d4490c8

caused by #1212 --------- Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck requested changes Oct 30, 2024

View reviewed changes

src/cli.ts Outdated Show resolved Hide resolved

MalickBurger force-pushed the feat/workspace-support branch from d95dee9 to 052b2a6 Compare November 4, 2024 10:45

MalickBurger force-pushed the feat/workspace-support branch from 052b2a6 to e7c1f1d Compare December 11, 2024 14:16

jkowalleck self-requested a review December 11, 2024 14:55

jkowalleck added the enhancement New feature or request label Dec 11, 2024

jkowalleck mentioned this pull request Dec 11, 2024

[FEAT] support workspaces #1126

Open

feat: add support for targetting specific workspaces

a5ff5bc

Signed-off-by: MalickBurger <[email protected]>

MalickBurger force-pushed the feat/workspace-support branch from e7c1f1d to a5ff5bc Compare December 12, 2024 13:00

Merge branch 'main' into feat/workspace-support

ae6e4d3

Signed-off-by: Jan Kowalleck <[email protected]>

jkowalleck requested changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for targetting specific workspaces #1212

feat: add support for targetting specific workspaces #1212

MalickBurger commented Aug 23, 2024 •

edited by jkowalleck

Loading

jkowalleck left a comment

jkowalleck Aug 25, 2024 •

edited

Loading

MalickBurger commented Aug 26, 2024

jkowalleck commented Aug 26, 2024 •

edited

Loading

MalickBurger commented Sep 16, 2024

jkowalleck commented Sep 29, 2024

jkowalleck commented Oct 29, 2024

jkowalleck commented Oct 31, 2024 •

edited

Loading

jkowalleck commented Dec 11, 2024

MalickBurger commented Dec 11, 2024

jkowalleck Dec 24, 2024

jkowalleck Dec 24, 2024

jkowalleck Dec 24, 2024

jkowalleck Dec 24, 2024

jkowalleck Dec 24, 2024

jkowalleck commented Dec 24, 2024

feat: add support for targetting specific workspaces #1212

Are you sure you want to change the base?

feat: add support for targetting specific workspaces #1212

Conversation

MalickBurger commented Aug 23, 2024 • edited by jkowalleck Loading

jkowalleck left a comment

Choose a reason for hiding this comment

jkowalleck Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

MalickBurger commented Aug 26, 2024

jkowalleck commented Aug 26, 2024 • edited Loading

MalickBurger commented Sep 16, 2024

jkowalleck commented Sep 29, 2024

jkowalleck commented Oct 29, 2024

jkowalleck commented Oct 31, 2024 • edited Loading

jkowalleck commented Dec 11, 2024

MalickBurger commented Dec 11, 2024

jkowalleck Dec 24, 2024

Choose a reason for hiding this comment

jkowalleck Dec 24, 2024

Choose a reason for hiding this comment

jkowalleck Dec 24, 2024

Choose a reason for hiding this comment

jkowalleck Dec 24, 2024

Choose a reason for hiding this comment

jkowalleck Dec 24, 2024

Choose a reason for hiding this comment

jkowalleck commented Dec 24, 2024

MalickBurger commented Aug 23, 2024 •

edited by jkowalleck

Loading

jkowalleck Aug 25, 2024 •

edited

Loading

jkowalleck commented Aug 26, 2024 •

edited

Loading

jkowalleck commented Oct 31, 2024 •

edited

Loading