-
Notifications
You must be signed in to change notification settings - Fork 209
Dev v3 amanabiy pages #4136
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
base: main
Are you sure you want to change the base?
Dev v3 amanabiy pages #4136
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4136 +/- ##
=======================================
Coverage 97.15% 97.15%
=======================================
Files 878 878
Lines 25716 25716
Branches 9297 9297
=======================================
Hits 24985 24985
Misses 725 725
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cb18531 to
8b1eafe
Compare
8b1eafe to
112b41b
Compare
|
|
||
| function execCommand(command, options = {}) { | ||
| try { | ||
| execSync(command, { stdio: 'inherit', ...options }); |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
absolute path
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
Generally, the problem is fixed by avoiding concatenating environment-derived values into a shell command string executed via a shell. Instead, call the program directly and pass arguments as an array using child_process.execFileSync or spawnSync, so the shell doesn’t reinterpret special characters. If dynamic construction is unavoidable, inputs must be safely escaped—but that is error-prone and unnecessary here.
In this specific file, we should:
- Replace
execSync(command, ...)withexecFileSyncusing a command binary and an arguments array. - Stop passing full shell command strings like
mkdir -p ${copyBuildToolsPath}intoexecCommand. Instead, callexecCommandwith a command name and a list of arguments, e.g.execCommand('mkdir', ['-p', copyBuildToolsPath]). - Update
execCommandto accept(cmd, args = [], options = {}), log a human-readable representation, and pass those directly toexecFileSync. - Keep functionality identical: still run
mkdir -p <path>,rm -rf <path>, andgit clone --branch ...with the same options and error logging.
Concretely in scripts/setup-build-tools.js:
- Add
execFileSyncto the import fromchild_process. - Change the three top-level
execCommandcalls on lines 17–19 to use(command, args)instead of a single string. - Rewrite
execCommandon lines 23–33 accordingly, and update error messages to print the reconstructed command string for readability.
-
Copy modified line R9 -
Copy modified lines R17-R19 -
Copy modified line R23 -
Copy modified line R25 -
Copy modified lines R27-R28
| @@ -6,7 +6,7 @@ | ||
| // "postinstall": "node ./scripts/install-peer-dependency.js collection-hooks:property-filter-token-groups" | ||
| // where "collection-hooks" is the package to fetch and "property-filter-token-groups" is the branch name in GitHub. | ||
|
|
||
| import { execSync } from 'child_process'; | ||
| import { execFileSync } from 'child_process'; | ||
| import process from 'node:process'; | ||
| import path from 'path'; | ||
|
|
||
| @@ -14,17 +14,18 @@ | ||
| const packageName = 'build-tools'; | ||
| const targetRepository = `https://github.com/cloudscape-design/${packageName}.git`; | ||
| const copyBuildToolsPath = path.join(process.cwd(), 'shared', 'build-tools'); | ||
| execCommand(`mkdir -p ${copyBuildToolsPath}`); | ||
| execCommand(`rm -rf ${copyBuildToolsPath}`); | ||
| execCommand(`git clone --branch ${branch} --single-branch ${targetRepository} ${copyBuildToolsPath}`); | ||
| execCommand('mkdir', ['-p', copyBuildToolsPath]); | ||
| execCommand('rm', ['-rf', copyBuildToolsPath]); | ||
| execCommand('git', ['clone', '--branch', branch, '--single-branch', targetRepository, copyBuildToolsPath]); | ||
|
|
||
| console.log(`build-tools has been successfully installed!`); | ||
|
|
||
| function execCommand(command, options = {}) { | ||
| function execCommand(command, args = [], options = {}) { | ||
| try { | ||
| execSync(command, { stdio: 'inherit', ...options }); | ||
| execFileSync(command, args, { stdio: 'inherit', ...options }); | ||
| } catch (error) { | ||
| console.error(`Error executing command: ${command}`); | ||
| const commandString = [command, ...args].join(' '); | ||
| console.error(`Error executing command: ${commandString}`); | ||
| console.error(`Error message: ${error.message}`); | ||
| console.error(`Stdout: ${error.stdout && error.stdout.toString()}`); | ||
| console.error(`Stderr: ${error.stderr && error.stderr.toString()}`); |
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Can be used in postinstall script like so: | ||
| // "postinstall": "node ./scripts/install-peer-dependency.js collection-hooks:property-filter-token-groups" |
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 description is a leftover, I believe we don't need any for this script as all
| const copyBuildToolsPath = path.join(process.cwd(), 'shared', 'build-tools'); | ||
| execCommand(`mkdir -p ${copyBuildToolsPath}`); | ||
| execCommand(`rm -rf ${copyBuildToolsPath}`); | ||
| execCommand(`git clone --branch ${branch} --single-branch ${targetRepository} ${copyBuildToolsPath}`); |
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.
For such a simple command - can we inline that in the package.json?
E.g. "rimraf shared/build-tools && mkdir -p shared/build-tools && git clone --branch add-test-pages-util-permutation-view --single-branch https://github.com/cloudscape-design/build-tools.git shared/build-tools"
| <PermutationsView permutations={permutations} render={permutation => <Alert {...permutation} />} /> | ||
| <PermutationsView | ||
| permutations={permutations} | ||
| render={(permutation: AlertProps) => <Alert {...permutation} />} |
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.
why the param is explicitly annotated here - it is necessary?
| "@babel/plugin-syntax-typescript": "^7.23.3", | ||
| "@cloudscape-design/browser-test-tools": "^3.0.0", | ||
| "@cloudscape-design/build-tools": "github:cloudscape-design/build-tools#main", | ||
| "@cloudscape-design/build-tools-repo": "github:cloudscape-design/build-tools#add-test-pages-util-permutation-view", |
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.
what is @cloudscape-design/build-tools-repo for?
Description
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.