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

build(lint): disallow node process api, favor Toolkit ChildProcess api #6113

Merged
merged 28 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
86405be
fix problem
Hweinstock Nov 22, 2024
9a96353
add await to ensure promise resolves before test ends
Hweinstock Nov 22, 2024
1fb2337
remove sleep
Hweinstock Nov 22, 2024
1547c7f
move await before telemetry assertion to avoid race cond
Hweinstock Nov 22, 2024
f026fab
implement lint rule
Hweinstock Nov 26, 2024
c06ba48
refine imports of child process
Hweinstock Nov 26, 2024
b789af9
continue migration
Hweinstock Nov 27, 2024
279a6be
finish migration
Hweinstock Nov 27, 2024
a952c30
remove dead import
Hweinstock Nov 27, 2024
018d719
run branch check on PRs only
Hweinstock Dec 9, 2024
d03a2da
use event branch name
Hweinstock Dec 9, 2024
4fb8e66
change wording
Hweinstock Dec 9, 2024
cdecab6
add extra check
Hweinstock Dec 9, 2024
f2900e5
switch to single quotes
Hweinstock Dec 9, 2024
902de77
Merge branch 'aws:master' into master
Hweinstock Dec 9, 2024
bf03fd3
Merge branch 'aws:master' into master
Hweinstock Dec 10, 2024
4cd5400
Merge branch 'aws:master' into master
Hweinstock Dec 10, 2024
0234303
Merge branch 'aws:master' into master
Hweinstock Dec 11, 2024
4fa0118
merge in master
Hweinstock Dec 11, 2024
998e11b
add exception for new case
Hweinstock Dec 11, 2024
f988458
Merge branch 'aws:master' into master
Hweinstock Dec 12, 2024
54be0b3
Merge branch 'aws:master' into master
Hweinstock Dec 12, 2024
d4f7dda
Merge branch 'aws:master' into master
Hweinstock Dec 12, 2024
33f900a
Merge branch 'aws:master' into master
Hweinstock Dec 12, 2024
bed199a
Merge branch 'master' into lint/noCP
Hweinstock Dec 12, 2024
6911026
Update .eslintrc.js
Hweinstock Dec 12, 2024
ae380bc
Merge branch 'lint/noCP' of github.com:Hweinstock/aws-toolkit-vscode …
Hweinstock Dec 12, 2024
b13d754
use local var
Hweinstock Dec 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ module.exports = {
name: 'fs',
message: 'Avoid node:fs and use shared/fs/fs.ts when possible.',
},
{
name: 'child_process',
message:
'Avoid child_process, use ChildProcess from `shared/utilities/processUtils.ts` instead.',
},
],
},
],
Expand Down
2 changes: 1 addition & 1 deletion packages/core/scripts/build/generateServiceClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as proc from 'child_process'
import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
import * as nodefs from 'fs' // eslint-disable-line no-restricted-imports
import * as path from 'path'

Expand Down
2 changes: 1 addition & 1 deletion packages/core/scripts/test/launchTestUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as proc from 'child_process'
import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
Hweinstock marked this conversation as resolved.
Show resolved Hide resolved
import packageJson from '../../package.json'
import { downloadAndUnzipVSCode, resolveCliArgsFromVSCodeExecutablePath } from '@vscode/test-electron'
import { join, resolve } from 'path'
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/amazonq/lsp/lspClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import * as vscode from 'vscode'
import * as path from 'path'
import * as nls from 'vscode-nls'
import * as cp from 'child_process'
import { spawn } from 'child_process' // eslint-disable-line no-restricted-imports
import * as crypto from 'crypto'
import * as jose from 'jose'

Expand Down Expand Up @@ -199,7 +199,7 @@ export async function activate(extensionContext: ExtensionContext) {

const nodename = process.platform === 'win32' ? 'node.exe' : 'node'

const child = cp.spawn(extensionContext.asAbsolutePath(path.join('resources', nodename)), [
const child = spawn(extensionContext.asAbsolutePath(path.join('resources', nodename)), [
serverModule,
...debugOptions.execArgv,
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import { VueWebview, VueWebviewPanel } from '../../../webviews/main'
import { ExtContext } from '../../../shared/extensions'
import { telemetry } from '../../../shared/telemetry/telemetry'
import { AccessAnalyzer, SharedIniFileCredentials } from 'aws-sdk'
import { execFileSync } from 'child_process'
import { ToolkitError } from '../../../shared/errors'
import { makeTemporaryToolkitFolder, tryRemoveFolder } from '../../../shared/filesystemUtilities'
import { globals } from '../../../shared'
Expand All @@ -28,6 +27,7 @@ import {
} from './constants'
import { DefaultS3Client, parseS3Uri } from '../../../shared/clients/s3Client'
import { ExpiredTokenException } from '@aws-sdk/client-sso-oidc'
import { ChildProcess } from '../../../shared/utilities/processUtils'

const defaultTerraformConfigPath = 'resources/policychecks-tf-default.yaml'
// Diagnostics for Custom checks are shared
Expand Down Expand Up @@ -277,7 +277,7 @@ export class IamPolicyChecksWebview extends VueWebview {
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
]
this.executeValidatePolicyCommand({
await this.executeValidatePolicyCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand All @@ -300,7 +300,7 @@ export class IamPolicyChecksWebview extends VueWebview {
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
}
this.executeValidatePolicyCommand({
await this.executeValidatePolicyCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -357,7 +357,7 @@ export class IamPolicyChecksWebview extends VueWebview {
'--reference-policy-type',
`${policyType}`,
]
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -391,7 +391,7 @@ export class IamPolicyChecksWebview extends VueWebview {
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
}
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -454,7 +454,7 @@ export class IamPolicyChecksWebview extends VueWebview {
if (resources !== '') {
args.push('--resources', `${resources}`)
}
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -489,7 +489,7 @@ export class IamPolicyChecksWebview extends VueWebview {
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
}
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -525,7 +525,7 @@ export class IamPolicyChecksWebview extends VueWebview {
'--config',
`${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
]
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand Down Expand Up @@ -554,7 +554,7 @@ export class IamPolicyChecksWebview extends VueWebview {
if (cfnParameterPath !== '') {
args.push('--template-configuration-file', `${cfnParameterPath}`)
}
this.executeCustomPolicyChecksCommand({
await this.executeCustomPolicyChecksCommand({
command,
args,
cfnParameterPathExists: !!cfnParameterPath,
Expand All @@ -573,16 +573,16 @@ export class IamPolicyChecksWebview extends VueWebview {
}
}

public executeValidatePolicyCommand(opts: PolicyCommandOpts & { policyType?: PolicyChecksPolicyType }) {
telemetry.accessanalyzer_iamPolicyChecksValidatePolicy.run((span) => {
public async executeValidatePolicyCommand(opts: PolicyCommandOpts & { policyType?: PolicyChecksPolicyType }) {
await telemetry.accessanalyzer_iamPolicyChecksValidatePolicy.run(async (span) => {
try {
span.record({
cfnParameterFileUsed: opts.cfnParameterPathExists,
documentType: opts.documentType,
inputPolicyType: opts.policyType ?? 'None',
})
const resp = execFileSync(opts.command, opts.args)
const findingsCount = this.handleValidatePolicyCliResponse(resp.toString())
const result = await ChildProcess.run(opts.command, opts.args, { collect: true })
const findingsCount = this.handleValidatePolicyCliResponse(result.stdout)
span.record({
findingsCount: findingsCount,
})
Expand Down Expand Up @@ -633,10 +633,10 @@ export class IamPolicyChecksWebview extends VueWebview {
return findingsCount
}

public executeCustomPolicyChecksCommand(
public async executeCustomPolicyChecksCommand(
opts: PolicyCommandOpts & { checkType: PolicyChecksCheckType; referencePolicyType?: PolicyChecksPolicyType }
) {
telemetry.accessanalyzer_iamPolicyChecksCustomChecks.run((span) => {
await telemetry.accessanalyzer_iamPolicyChecksCustomChecks.run(async (span) => {
try {
span.record({
cfnParameterFileUsed: opts.cfnParameterPathExists,
Expand All @@ -645,8 +645,8 @@ export class IamPolicyChecksWebview extends VueWebview {
inputPolicyType: 'None', // Note: This will change once JSON policy language is enabled for Custom policy checks
referencePolicyType: opts.referencePolicyType ?? 'None',
})
const resp = execFileSync(opts.command, opts.args)
const findingsCount = this.handleCustomPolicyChecksCliResponse(resp.toString())
const resp = await ChildProcess.run(opts.command, opts.args)
const findingsCount = this.handleCustomPolicyChecksCliResponse(resp.stdout)
span.record({
findingsCount: findingsCount,
})
Expand Down Expand Up @@ -790,7 +790,7 @@ export async function renderIamPolicyChecks(context: ExtContext): Promise<VueWeb
checkAccessNotGrantedResourcesTextArea,
customChecksFileErrorMessage,
cfnParameterPath: cfnParameterPath ? cfnParameterPath : '',
pythonToolsInstalled: arePythonToolsInstalled(),
pythonToolsInstalled: await arePythonToolsInstalled(),
},
client,
context.regionProvider.defaultRegionId
Expand Down Expand Up @@ -828,20 +828,20 @@ export async function _readCustomChecksFile(input: string): Promise<string> {
}

// Check if Cfn and Tf tools are installed
export function arePythonToolsInstalled(): boolean {
export async function arePythonToolsInstalled(): Promise<boolean> {
const logger: Logger = getLogger()
let cfnToolInstalled = true
let tfToolInstalled = true
try {
execFileSync('tf-policy-validator')
await ChildProcess.run('tf-policy-validator')
} catch (err: any) {
if (isProcessNotFoundErr(err.message)) {
tfToolInstalled = false
logger.error('Terraform Policy Validator is not found')
}
}
try {
execFileSync('cfn-policy-validator')
await ChildProcess.run('cfn-policy-validator')
} catch (err: any) {
if (isProcessNotFoundErr(err.message)) {
cfnToolInstalled = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
import path from 'path'
import { testGenState } from '..'
import { ChatSessionManager } from '../../amazonqTest/chat/storages/chatSession'
import { ChildProcess, spawn } from 'child_process'
import { ChildProcess, spawn } from 'child_process' // eslint-disable-line no-restricted-imports
import { BuildStatus } from '../../amazonqTest/chat/session/session'
import { fs } from '../../shared/fs/fs'
import { TestGenerationJobStatus } from '../models/constants'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import * as vscode from 'vscode'
import { FolderInfo, transformByQState } from '../../models/model'
import { getLogger } from '../../../shared/logger'
import * as CodeWhispererConstants from '../../models/constants'
import { spawnSync } from 'child_process' // Consider using ChildProcess once we finalize all spawnSync calls
// Consider using ChildProcess once we finalize all spawnSync calls
import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
import { CodeTransformBuildCommand, telemetry } from '../../../shared/telemetry/telemetry'
import { CodeTransformTelemetryState } from '../../../amazonqGumby/telemetry/codeTransformTelemetryState'
import { ToolkitError } from '../../../shared/errors'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import { BuildSystem, JDKVersion, TransformationCandidateProject } from '../../m
import { getLogger } from '../../../shared/logger'
import * as CodeWhispererConstants from '../../models/constants'
import * as vscode from 'vscode'
import { spawnSync } from 'child_process' // Consider using ChildProcess once we finalize all spawnSync calls
// Consider using ChildProcess once we finalize all spawnSync calls
import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
import {
NoJavaProjectsFoundError,
NoMavenJavaProjectsFoundError,
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/lambda/commands/createNewSamApp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ import {
isCloud9,
getLaunchConfigDocUrl,
} from '../../shared/extensionUtilities'
import { execFileSync } from 'child_process'
import { checklogs } from '../../shared/localizedText'
import globals from '../../shared/extensionGlobals'
import { telemetry } from '../../shared/telemetry/telemetry'
import { LambdaArchitecture, Result, Runtime } from '../../shared/telemetry/telemetry'
import { getTelemetryReason, getTelemetryResult } from '../../shared/errors'
import { openUrl, replaceVscodeVars } from '../../shared/utilities/vsCodeUtils'
import { fs } from '../../shared'
import { ChildProcess } from '../../shared/utilities/processUtils'

export const samInitTemplateFiles: string[] = ['template.yaml', 'template.yml']
export const samInitReadmeFile: string = 'README.TOOLKIT.md'
Expand Down Expand Up @@ -218,7 +218,9 @@ export async function createNewSamApplication(
// Needs to be done or else gopls won't start
if (goRuntimes.includes(createRuntime)) {
try {
execFileSync('go', ['mod', 'tidy'], { cwd: path.join(path.dirname(templateUri.fsPath), 'hello-world') })
await ChildProcess.run('go', ['mod', 'tidy'], {
spawnOptions: { cwd: path.join(path.dirname(templateUri.fsPath), 'hello-world') },
})
} catch (err) {
getLogger().warn(
localize(
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/shared/extensions/git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import * as vscode from 'vscode'
import * as GitTypes from '../../../types/git.d'
import { SemVer, parse as semverParse } from 'semver'
import { execFile } from 'child_process'
import { execFile } from 'child_process' // eslint-disable-line no-restricted-imports
import { promisify } from 'util'
import { VSCODE_EXTENSION_ID } from '../extensions'
import { makeTemporaryToolkitFolder, tryRemoveFolder } from '../filesystemUtilities'
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/shared/sam/cli/samCliInvokerUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { SpawnOptions } from 'child_process'
import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
import { getLogger } from '../../logger'
import { getUserAgent } from '../../telemetry/util'
import { ChildProcessResult, ChildProcessOptions } from '../../utilities/processUtils'
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/shared/sam/cli/samCliLocalInvoke.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as proc from 'child_process'
import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
import { pushIf } from '../../utilities/collectionUtils'
import * as nls from 'vscode-nls'
import { getLogger, getDebugConsoleLogger, Logger } from '../../logger'
Expand All @@ -30,7 +30,7 @@ export const waitForDebuggerMessages = {
export interface SamLocalInvokeCommandArgs {
command: string
args: string[]
options?: proc.SpawnOptions
options?: SpawnOptions
/** Wait until strings specified in `debuggerAttachCues` appear in the process output. */
waitForCues: boolean
timeout?: Timeout
Expand Down
15 changes: 9 additions & 6 deletions packages/core/src/shared/sam/debugger/goSamDebug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { getLogger } from '../../logger'
import fs from '../../fs/fs'
import { ChildProcess } from '../../utilities/processUtils'
import { Timeout } from '../../utilities/timeoutUtils'
import { execFileSync, SpawnOptions } from 'child_process'
import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
import * as nls from 'vscode-nls'
import { sleep } from '../../utilities/timeoutUtils'
import globals from '../../extensionGlobals'
Expand Down Expand Up @@ -174,9 +174,11 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom
// Go from trying to find the manifest file and uses GOPATH provided below.
installOptions.env!['GO111MODULE'] = 'off'

function getDelveVersion(repo: string, silent: boolean): string {
async function getDelveVersion(repo: string, silent: boolean): Promise<string> {
try {
return execFileSync('git', ['-C', repo, 'describe', '--tags', '--abbrev=0']).toString().trim()
return (
await ChildProcess.run('git', ['-C', repo, 'describe', '--tags', '--abbrev=0'], { collect: true })
).stdout.trim()
} catch (e) {
if (!silent) {
throw e
Expand All @@ -187,7 +189,8 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom

// It's fine if we can't get the latest Delve version, the Toolkit will use the last built one instead
try {
const goPath: string = JSON.parse(execFileSync('go', ['env', '-json']).toString()).GOPATH
const result = await ChildProcess.run('go', ['env', '-json'], { collect: true })
const goPath: string = JSON.parse(result.stdout).GOPATH
let repoPath: string = path.join(goPath, 'src', delveRepo)

if (!getDelveVersion(repoPath, true)) {
Expand All @@ -200,11 +203,11 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom
installOptions.env!['GOPATH'] = debuggerPath
repoPath = path.join(debuggerPath, 'src', delveRepo)
const args = ['get', '-d', `${delveRepo}/cmd/dlv`]
const out = execFileSync('go', args, installOptions as any)
const out = await ChildProcess.run('go', args, { ...(installOptions as any), collect: true })
getLogger().debug('"go %O": %s', args, out)
}

delveVersion = getDelveVersion(repoPath, false)
delveVersion = await getDelveVersion(repoPath, false)
} catch (e) {
getLogger().debug('Failed to get latest Delve version: %O', e as Error)
}
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/shared/utilities/processUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as proc from 'child_process'
import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
import * as crossSpawn from 'cross-spawn'
import * as logger from '../logger'
import { Timeout, CancellationError, waitUntil } from './timeoutUtils'
Expand Down Expand Up @@ -100,6 +100,13 @@ export class ChildProcess {
// TODO: allow caller to use the various loggers instead of just the single one
this.#log = baseOptions.logging !== 'no' ? logger.getLogger() : logger.getNullLogger()
}
public static async run(
command: string,
args: string[] = [],
options?: ChildProcessOptions
): Promise<ChildProcessResult> {
return await new ChildProcess(command, args, options).run()
}

// Inspired by 'got'
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import assert from 'assert'
import { SpawnOptions } from 'child_process'
import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
import * as path from 'path'
import { makeTemporaryToolkitFolder } from '../../../../shared/filesystemUtilities'
import { makeUnexpectedExitCodeError } from '../../../../shared/sam/cli/samCliInvokerUtils'
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/test/shared/sam/cli/samCliInit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import assert from 'assert'
import { SpawnOptions } from 'child_process'
import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
import {
eventBridgeStarterAppTemplate,
getSamCliTemplateParameter,
Expand Down
Loading
Loading