-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: add support for targetting specific workspaces #1212
base: main
Are you sure you want to change the base?
Conversation
4ac8c52
to
a26d8da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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 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). |
@MalickBurger, for adding additional integration tests,
since you are planning on adding conditional parameters in the |
caused by #1212 --------- Signed-off-by: Jan Kowalleck <[email protected]>
Looking at this, could you provide some testing support for an existing argument that is passed to |
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. |
I've overhauled the integration tests. 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 |
d95dee9
to
052b2a6
Compare
052b2a6
to
e7c1f1d
Compare
@MalickBurger , i see you've rebased this feature branch recently. |
Hi @jkowalleck, yes I have been busy recently but am getting back to this now. Will be posting updates soon. |
e7c1f1d
to
a5ff5bc
Compare
merged in the latest master, fixed some merge conflicts. |
Will take a look and update over the next few days when I have a chance |
ae6e4d3
to
a300d13
Compare
seams like some tests are failing. |
4b610a3
to
99114f6
Compare
Should be resolved now. |
i am considering this feature to be integrated and marked as "experimental". |
99114f6
to
8e86e56
Compare
Hi @jkowalleck just want to find out where we stand regarding this PR. From my side I believe it is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something seams not right
src/builders.ts
Outdated
} | ||
} | ||
|
||
// No need to set explicitly if true as this is default behaviour |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// No need to set explicitly if true as this is default behaviour | |
// No need to set explicitly if true as this seams to be default behaviour for `npm-ls` |
['workspaces disabled npm 8', `8.${rMinor}.${rPatch}`, ['--no-workspaces'], [...npm8ArgsGeneral, '--workspaces=false']], | ||
['workspaces disabled npm 9', `9.${rMinor}.${rPatch}`, ['--no-workspaces'], [...npm9ArgsGeneral, '--workspaces=false']], | ||
['workspaces disabled npm 10', `10.${rMinor}.${rPatch}`, ['--no-workspaces'], [...npm10ArgsGeneral, '--workspaces=false']], | ||
['workspaces disabled npm 11', `11.${rMinor}.${rPatch}`, ['--no-workspaces'], [...npm11ArgsGeneral, '--workspaces=false']] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this flag seams unknown to npm11
$ npm11 ls --help
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> ...]]
[--include <prod|dev|optional|peer> [--include <prod|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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though it is not documented, it still has effect.
it is the same as npm ls --workspaces=false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default behaviour by npm ls
is to include all workspaces if present, this requires no explicit argument (no need for --workspaces=true
, which actually errors if there are no workspaces). If you want to disable this and exclude workspaces you can do this using --workspaces=false
. Now this means there is actually three modes for this, the default (undefined
) which includes workspaces if they are present, explicitly true
which errors if there are no workspaces, and explicitly false
which ignores workspaces completely.
Now in terms of how we pass this logic through the cdx plugin we have two options, either we name the argument --workspaces
to match npm ls
but now we need to tell users that the default for this boolean is undefined
which actually acts the exact same as true
except it won't cause an error if there are no workspaces (quite confusing and unclear for users in my opinion).
Or the second option is to add a argument which explicitly allows you to disable resolving workspaces using --no-workspaces
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason I did it this way is because of how poor the documentation for npm ls
is for this specific argument, this is what the official docs say:
[workspaces](https://docs.npmjs.com/cli/v11/commands/npm-ls#workspaces)
Default: null
Type: null or Boolean
Set to true to run the command in the context of all configured workspaces.
Explicitly setting this to false will cause commands like install to ignore workspaces altogether. When not set explicitly:
Commands that operate on the node_modules tree (install, update, etc.) will link workspaces into the node_modules folder. - Commands that do other things (test, exec, publish, etc.) will operate on the root project, unless one or more workspaces are specified in the workspace config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason I did it this way is because of how poor the documentation for
npm ls
is for this specific argument, this is what the official docs say:[workspaces](https://docs.npmjs.com/cli/v11/commands/npm-ls#workspaces) Default: null Type: null or Boolean Set to true to run the command in the context of all configured workspaces. Explicitly setting this to false will cause commands like install to ignore workspaces altogether. When not set explicitly: Commands that operate on the node_modules tree (install, update, etc.) will link workspaces into the node_modules folder. - Commands that do other things (test, exec, publish, etc.) will operate on the root project, unless one or more workspaces are specified in the workspace config.
How does a user determine what the default behaviour is from this documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
My suggestion: We use --workspaces
and have it a boolean with default value of null
. (like npm docs say)
if the value of options.workspaces
is not null
, then the value is passed to npm-ls
.
And of course, our own help text should be meaningful. Unlike the one from npm-ls
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, I think its a good compromise, I will make the relevant updates
).default([], 'empty') | ||
).addOption( | ||
new Option( | ||
'--no-workspaces', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i still do not see how this comes together.
where comes this --no-workspaces
from? I mean, npm knows -ws|--workspaces
-and it defaults to null
for this. and you can set tot to true/false ...
why does this option not mimic npm-ls options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment about Commander. Happy to change this to --workspaces
it just makes it less intuitive for users in my opinion but if you want to always mimic npm ls
then I will make the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this option.
we tend to mimic npm-ls
- and they dont call this out explicitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i still do not see how this comes together. where comes this
--no-workspaces
from? I mean, npm knows-ws|--workspaces
-and it defaults tonull
for this. and you can set tot to true/false ... why does this option not mimic npm-ls options?
Npm ls does support it, test it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i still do not see how this comes together. where comes this
--no-workspaces
from? I mean, npm knows-ws|--workspaces
-and it defaults tonull
for this. and you can set tot to true/false ... why does this option not mimic npm-ls options?
I don't follow you, this is best commander.js supports booleans unless we parse the value as a string and convert to a boolean tj/commander.js#344.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this option. we tend to mimic
npm-ls
- and they dont call this out explicitely.
They also don't explicitly mention how workspace logic works by default but we are now telling users how that works through or params and README (which we have to do because it is not intuitive at all so they need some guidance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this option. we tend to mimic
npm-ls
- and they dont call this out explicitely.They also don't explicitly mention how workspace logic works by default but we are now telling users how that works through or params and README (which we have to do because it is not intuitive at all so they need some guidance)
Additionally, without it, how will users disable the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think some parts need discussion
@@ -64,6 +67,9 @@ export class BomBuilder { | |||
flattenComponents: boolean | |||
shortPURLs: boolean | |||
gatherLicenseTexts: boolean | |||
workspace: string[] | |||
includeWorkspaceRoot: boolean | |||
workspaces?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workspaces?: boolean | |
workspaces: boolean | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the builder perspective we can treat this as undefined as well and thus don't pass to npm ls (default behaviour) but will change it if you prefer.
@@ -86,6 +92,9 @@ export class BomBuilder { | |||
this.flattenComponents = options.flattenComponents ?? false | |||
this.shortPURLs = options.shortPURLs ?? false | |||
this.gatherLicenseTexts = options.gatherLicenseTexts ?? false | |||
this.workspace = options.workspace ?? [] | |||
this.includeWorkspaceRoot = options.includeWorkspaceRoot ?? false | |||
this.workspaces = options.workspaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.workspaces = options.workspaces | |
this.workspaces = options.workspaces ?? null |
src/builders.ts
Outdated
// No need to set explicitly if false as this is default behaviour | ||
if (this.includeWorkspaceRoot) { | ||
if (npmVersionT[0] >= 8) { | ||
args.push('--include-workspace-root=true') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args.push('--include-workspace-root=true') | |
args.push('--include-workspace-root=${this.includeWorkspaceRoot}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary I believe, as we are in the if statement and know it is true.
src/cli.ts
Outdated
@@ -238,6 +270,20 @@ export async function run (process: NodeJS.Process): Promise<number> { | |||
throw new Error('missing evidence') | |||
} | |||
|
|||
if (options.workspaces === false) { | |||
if (options.workspace !== undefined && options.workspace.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.workspace
is never undefined
, right?
if (options.workspace !== undefined && options.workspace.length > 0) { | |
if (options.workspace.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is remnant of previous logic, will remove the check
src/cli.ts
Outdated
@@ -238,6 +270,20 @@ export async function run (process: NodeJS.Process): Promise<number> { | |||
throw new Error('missing evidence') | |||
} | |||
|
|||
if (options.workspaces === false) { | |||
if (options.workspace !== undefined && options.workspace.length > 0) { | |||
myConsole.error('ERROR | Bad config: `--workspace` option cannot be used when `--no-workspaces` is also configured') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this custom output, throwing an error is enough
myConsole.error('ERROR | Bad config: `--workspace` option cannot be used when `--no-workspaces` is also configured') |
src/cli.ts
Outdated
if (options.workspaces === false) { | ||
if (options.workspace !== undefined && options.workspace.length > 0) { | ||
myConsole.error('ERROR | Bad config: `--workspace` option cannot be used when `--no-workspaces` is also configured') | ||
throw new Error('bad config') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets copy the error message npm-ls
uses.
throw new Error('bad config') | |
throw new Error('Can not use --no-workspaces and --workspace at the same time') |
src/cli.ts
Outdated
if (options.includeWorkspaceRoot) { | ||
if (options.workspace.length === 0 && options.workspaces !== true) { | ||
myConsole.error('ERROR | Bad config: `--include-workspace-root` can only be used when `--workspace` or `--workspaces` is also configured') | ||
throw new Error('bad config') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not true. please get rid of this error constraint.
reason:
if i want my workspace root only, i could set all other workspace options to empty/false ...
npm ls
honors this behaviour.
am i wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it depends on how you look at it, the "--include-workspace-root" param only has an impact or relevance if you are specifying "--workspace=x" or "--workspaces" because all other scenarios we are not dealing with workspaces explicitly so why use the param?
In this case the logic is slightly wrong because I am not handling the default behaviour of "workspaces" which is effectively true (but is actually null.........).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I included this was to help guide users a bit on how it all works together, so they don't do things like always pass in "--include-workspace-root" thinking they need to even though root dependencies are always included unless under certain circumstances (which I document in the error and README/cli).
I think we are conflicting slightly on how this should be implemented because I am trying to design this in a way that is the most intuitive and yet still flexible experience for users. The aim is for it to be clear what the default behaviour is and how they can modify this behaviour if they wish. The way you would like this done is to mimic npm ls exactly which I still don't fully understand since most users of the plugin wouldn't really care how the internals work they just want a tool that can create sboms for npm projects as easily as possible. In an ideal world we could both have our way but I don't think it is the case here because the behaviour of npm ls with regards to workspaces is non intuitive and a bit of a black box (it also changes between version 7 and newer versions). Before I go back and rewrite the logic again let's please fully decide how this is going to look, how we are going to explain it to users, and what default logic will be for all combination of workspace related parameters. |
This PR and a reading I had the idea to mimic My idea was to go the "easy" path - from a product owner's perspective - as all responsibility would be shifted to the users. Especially with workspaces, I would expect to have experienced users who knew what they do. I think I understand your point, and you convinced me. After the implementation is done, we might see which kind of test data is required and how it can be provided. |
@MalickBurger and @jkowalleck what is the status here? I would like to use the cyclonedx-node-npm with some projects that uses npm workspaces. Let me know if I can help here in any way. I will try, the branch build on my projects for now. |
Hi @mapau, I will be getting back to this later in the week, we are close (hopefully) to having this PR finalized. Happy if you would consider doing some testing once complete? |
Signed-off-by: MalickBurger <[email protected]>
11817e0
to
02824c2
Compare
Hi @jkowalleck, I have refactored my changes based on our last discussion. I have simplified the logic and have tried to make it as simple and intuitive as possible to users while still mimicking npm ls where possible. I have designed it in such a way that users do not need to have an indepth understanding of npm ls or knowledge on which version they are running. I have updated the tests but I need to do a few manual sanity tests as well however I will wait for now incase I need to make any additional changes. |
fixes #1126