Skip to content
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

[ rush scan] feat: modify rush scan, support executing projects under rush and custom scanning folders. #5046

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sherlockfeng
Copy link
Contributor

Summary

  • Support for using -o to specify the project to be scanned in a repository that has been connected to rush.
  • Support for specifying the folder to be scanned.

Details

When I was migrating a pnpm workspace project to rush.
I first set up rush.json and wrote in all the projects.

Using -o to specify the project to be scanned can avoid me going into each project directory to execute scan.

The folders to be scanned under the project are not uniform,
so I got all the folders under the project, and used ignore to read the .gitignore file to exclude,
and finally got all the folders to be scanned,
so this requires customizing the folders to be scanned.

How it was tested

  • build rush-lib

  • cd libraries/rush-lib/src/cli/actions/test/scanRepo

  • executenode <path to rush>/libraries/rush-lib/lib-commonjs/start.js scan --json -o b --folder test-folder1
    image

  • executenode <path to rush>/libraries/rush-lib/lib-commonjs/start.js scan --json -o a
    image

Why skip ScanAction.test.ts

image

libraries/rush-lib/config/jest.config.json Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
Comment on lines 64 to 77
this._folders = this.defineStringListParameter({
parameterLongName: '--folder',
parameterShortName: '-f',
argumentName: 'FOLDER',
description:
'The folders that need to be scanned, default is src and lib.' +
'Normally we can input all the folders under the project directory, excluding the ignored folders.'
});
this._projects = this.defineStringListParameter({
parameterLongName: '--only',
parameterShortName: '-o',
argumentName: 'PROJECT',
description: 'Projects that need to be checked for phantom dependencies.'
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would support all project selection parameters. There is a way to do that for an arbitrary action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, but i feel that this place is not very suitable for other selection capabilities, such as --to, because some projects may have their own directory structure. If it is --to, it will scan the same folder for these projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In actual use, I will detect which folders under the project will be submitted, and then pass these folders to the scan command one by one for scanning, so I think using the --project flag is sufficient.

libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/ScanAction.ts Outdated Show resolved Hide resolved
description:
'The folders that need to be scanned, default is src and lib.' +
'Normally we can input all the folders under the project directory, excluding the ignored folders.'
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick the default value here.

Copy link
Contributor Author

@sherlockfeng sherlockfeng Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't quite understand what this [stick] is for.

@sherlockfeng sherlockfeng force-pushed the feat/rush-scan-support-folders branch from 6ce11b3 to f2fe6b2 Compare December 16, 2024 03:47
@sherlockfeng sherlockfeng force-pushed the feat/rush-scan-support-folders branch from f2fe6b2 to 5a2532a Compare December 16, 2024 03:49
sherlockfeng and others added 6 commits December 16, 2024 11:49
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
@D4N14L
Copy link
Member

D4N14L commented Dec 17, 2024

I think it may be useful to follow the rush approach and allow special filter types, ex. -o path:<path-to-folder> would include only the projects under a specific folder, instead of using a dedicated parameter. This would mean that you would be selecting all projects under that folder though, but I think that's pretty much what you're trying to accomplish here.

@sherlockfeng
Copy link
Contributor Author

sherlockfeng commented Dec 18, 2024

oh, rush already has this -o path:<path-to-folder> filter ? i can change to use SelectionParameterSet to allow all rush filter. So in future, we can add other special filter in SelectionParameterSet @D4N14L what do you think ?

@sherlockfeng sherlockfeng deleted the feat/rush-scan-support-folders branch December 18, 2024 03:44
@sherlockfeng sherlockfeng restored the feat/rush-scan-support-folders branch December 18, 2024 03:44
@sherlockfeng sherlockfeng reopened this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants