chore: migrate google-storage-control#8650
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables generation for the Google Storage Control v2 library and updates the npm installation command in librarian.js to use a specific cache directory and suppress audit/funding messages. The reviewer suggests avoiding the hardcoded /tmp/npm-cache path to prevent permission conflicts on shared build agents and ensure cross-platform compatibility, recommending a cache directory within the package root instead.
|
|
||
| try { | ||
| execSync('npm install', {cwd: packageRoot, stdio: 'inherit'}); | ||
| execSync('npm install --cache=/tmp/npm-cache --no-audit --no-fund', {cwd: packageRoot, stdio: 'inherit'}); |
There was a problem hiding this comment.
Hardcoding /tmp/npm-cache can cause permission conflicts on shared build agents (e.g., if different builds run under different users) and is not cross-platform. Consider using a directory within the package root to ensure a safe, isolated, and cross-platform cache location.
| execSync('npm install --cache=/tmp/npm-cache --no-audit --no-fund', {cwd: packageRoot, stdio: 'inherit'}); | |
| execSync('npm install --cache=' + path.join(packageRoot, '.npm-cache') + ' --no-audit --no-fund', {cwd: packageRoot, stdio: 'inherit'}); |
c2c2a06 to
3c27196
Compare
danieljbruce
left a comment
There was a problem hiding this comment.
Just a few nits, but nothing seems particularly risky about this PR to me so approving to unblock librarian migration.
| private _universeDomain: string; | ||
| private _servicePath: string; | ||
| private _log = logging.log('storage'); | ||
| private _log = logging.log('storage-control'); |
There was a problem hiding this comment.
nit: Do we need to change this log? What improvement does it offer?
There was a problem hiding this comment.
Looks like the previous name is not correct since this is storage control client.
Migration google-storage-control to librarian generate.
Fixes googleapis/librarian#6359
Fixes googleapis/librarian#5955