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.

jkowalleck · 2024-10-30T11:39:33Z

src/cli.ts

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


Suggested change

'--workspaces <workspace...>',

'-w, --workspace <workspace-name...>',

An option '-ws, --workspaces' is to be added, too. it should be piped to npm-ls, just like '-w, --workspace ...'

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

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

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

feat: add support for targetting specific workspaces

052b2a6

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

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

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 Oct 30, 2024

jkowalleck Oct 30, 2024

jkowalleck commented Oct 31, 2024 •

edited

Loading

	'--workspaces <workspace...>',
	'-w, --workspace <workspace-name...>',

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 Oct 30, 2024

Choose a reason for hiding this comment

jkowalleck Oct 30, 2024

Choose a reason for hiding this comment

jkowalleck commented Oct 31, 2024 • edited Loading

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