New deploy service#244
Conversation
This reverts commit 27e548c.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
This diff migrates the web deployment infrastructure from AWS S3 (via TVM credentials) to a new deploy-service REST API using bearer token authentication. The code is generally well-structured with good error handling, input validation, and comprehensive test coverage. A few issues remain around hardcoded URLs, leftover commented-out code, package.json inconsistency, and a minor security concern with base64 content encoding.
📝 8 suggestion(s) - Please review inline comments below.
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff represents a significant architectural change from AWS S3/TVM-based storage to a custom deployment service API using bearer token auth. Most previous suggestions have been partially addressed (e.g., AIO_DEPLOYMENT_SERVICE_URL env var is now supported, proxy support is restored via undici). However, several issues remain: commented-out code is still present, the warning for overwriting deployments was removed without re-adding it, and the namespace validation regex still uses the same pattern for both fields despite different valid character sets.
📝 6 suggestion(s) (1 new, 5 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| let relativeDir = path.dirname(path.relative(dir, file)) | ||
| // path.relative returns '.' for files in the root directory, normalize to empty string | ||
| relativeDir = relativeDir === '.' ? '' : relativeDir | ||
|
|
There was a problem hiding this comment.
[Re-raised] Commented-out sleep/rate-limiting code should be removed or replaced with a real implementation if rate limiting is a concern.
| const res = await Promise.all(fileBatch.map(async file => { |
| if (Object.keys(responseHeaders).length > 0) { | ||
| uploadParams.Metadata = responseHeaders | ||
| // server expected body is: { contentType, cacheControl, customHeaders: {}, file: { name, content } } | ||
| // file.name is the path relative to namespace (e.g., 'images/photo.jpg' or 'index.html') |
There was a problem hiding this comment.
[Re-raised] File content is sent as base64-encoded string in a JSON body. For large static assets this doubles memory usage (Buffer + base64 string) and increases payload size by ~33%. Consider using multipart/form-data or a streaming upload approach for files above a certain size threshold.
There was a problem hiding this comment.
🤖 PR Reviewer
The diff represents a significant architectural change from AWS S3/TVM-based storage to a custom CDN deploy-service API using bearer token auth. Most previous suggestions have been addressed (namespace regex now uses /^[a-zA-Z0-9_-]+$/, warning log is re-added, separate hostname/namespace validation). However, several previously flagged issues remain: commented-out code in remote-storage.js, the commented-out sleep/rate-limiting code, the @adobe/aio-lib-core-tvm unused dependency, and the base64 upload concern. The test coverage is thorough and the refactor is generally clean.
📝 5 suggestion(s) (1 new, 4 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const allResults = [] | ||
| if (!this._authToken) { | ||
| throw new Error('cannot upload files, Authorization is required') | ||
| } |
There was a problem hiding this comment.
[Re-raised] Commented-out sleep/rate-limiting code should be removed. If rate limiting is a concern, implement it properly or remove the comment entirely.
| } | |
| const res = await Promise.all(fileBatch.map(async file => { |
| log('warning: an existing deployment will be overwritten') | ||
| } | ||
| await remoteStorage.emptyFolder(config.s3.folder + '/') | ||
| await remoteStorage.emptyFolder('/', config) |
There was a problem hiding this comment.
uploadDir is called with config.s3.folder which may be undefined if s3 config is absent. Add a guard or default value to prevent passing undefined as basePath, which would cause uploadDir to throw a confusing error.
| await remoteStorage.emptyFolder('/', config) | |
| const s3Folder = config.s3?.folder ?? '' | |
| await remoteStorage.uploadDir(dist, s3Folder, config, _log) |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff represents a significant architectural migration from AWS S3/TVM-based file storage to a new deploy-service REST API using bearer token authentication. The code is generally well-structured and the tests are comprehensive. A few issues remain: commented-out code, potential undefined config access, and the joi package remains in dependencies even though it's no longer used in remote-storage.js.
📝 5 suggestion(s) (2 new, 3 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| "joi": "^17.2.1", | ||
| "klaw": "^4", | ||
| "lodash.clonedeep": "^4.5.0", | ||
| "mime-types": "^2.1.24", |
There was a problem hiding this comment.
joi is still listed as a dependency but is no longer used in remote-storage.js (the Joi validation in the constructor was removed). Remove it to keep the dependency list clean.
| "mime-types": "^2.1.24", | |
| "lodash.clonedeep": "^4.5.0", |
| await remoteStorage.emptyFolder('/', config) | ||
| } | ||
| const _log = log ? (f) => log(`deploying ${path.relative(dist, f)}`) : null | ||
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) |
There was a problem hiding this comment.
[Re-raised] config.s3?.folder may be undefined if the s3 config is absent, which would pass undefined as basePath to uploadDir, causing a confusing 'basePath must be a valid string' error. Add a guard or default.
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) | |
| const s3Folder = config.s3?.folder ?? '' | |
| await remoteStorage.uploadDir(dist, s3Folder, config, _log) |
| if (!(await remoteStorage.folderExists('/', config))) { | ||
| throw new Error(`cannot undeploy static files, there is no deployment for ${config.s3.folder}`) | ||
| } | ||
|
|
There was a problem hiding this comment.
The error message still references config.s3.folder which may be undefined under the new API. Update the error message to reference the namespace instead, or guard against undefined.
| throw new Error(`cannot undeploy static files, there is no deployment for namespace ${config.ow.namespace}`) |
|
/review |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured refactor replacing AWS S3 direct SDK usage with a deploy-service HTTP API. The code is generally clean, tests are comprehensive, and the migration is logical. However, several previously raised issues remain unaddressed, and a few new concerns exist in the updated files.
📝 6 suggestion(s) (2 new, 4 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const { NodeHttpHandler } = require('@smithy/node-http-handler') | ||
| const { ProxyAgent } = require('proxy-agent') | ||
| const { EnvHttpProxyAgent } = require('undici') | ||
| const { codes, logAndThrow } = require('./StorageError') |
There was a problem hiding this comment.
[Re-raised] Commented-out code and stale comments ('// dev deploy => ...', '// run local => ...') should be removed before merging.
| const { codes, logAndThrow } = require('./StorageError') | |
| const deploymentServiceUrl = process.env.AIO_DEPLOYMENT_SERVICE_URL || | |
| (getCliEnv() === PROD_ENV | |
| ? 'https://deploy-service.app-builder.adp.adobe.io' | |
| : 'https://deploy-service.stg.app-builder.corp.adp.adobe.io') |
| if (!this._authToken) { | ||
| throw new Error('cannot upload files, Authorization is required') | ||
| } | ||
| while (fileBatch.length > 0) { |
There was a problem hiding this comment.
[Re-raised] Commented-out sleep/rate-limiting code should be removed. If rate limiting is needed, implement it properly or open a tracked issue.
| while (fileBatch.length > 0) { | |
| const res = await Promise.all(fileBatch.map(async file => { |
| "fs-extra": "^11", | ||
| "joi": "^17.2.1", | ||
| "klaw": "^4", | ||
| "lodash.clonedeep": "^4.5.0", |
There was a problem hiding this comment.
joi is still listed as a dependency but is no longer used in remote-storage.js. Remove it to keep the dependency list clean.
| "lodash.clonedeep": "^4.5.0", | |
| "lodash.clonedeep": "^4.5.0", |
| await remoteStorage.emptyFolder('/', config) | ||
| } | ||
| const _log = log ? (f) => log(`deploying ${path.relative(dist, f)}`) : null | ||
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) |
There was a problem hiding this comment.
[Re-raised] config.s3?.folder may be undefined if the s3 config is absent, causing a confusing 'basePath must be a valid string' error downstream in uploadDir. Add a guard or default.
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) | |
| const s3Folder = config.s3?.folder ?? '' | |
| await remoteStorage.uploadDir(dist, s3Folder, config, _log) |
|
|
||
| const remoteStorage = new RemoteStorage(creds) | ||
| const remoteStorage = new RemoteStorage(bearerToken) | ||
|
|
There was a problem hiding this comment.
[Re-raised] Error message still references config.s3.folder which may be undefined under the new API. Update to reference the namespace instead.
| throw new Error(`cannot undeploy static files, there is no deployment for namespace ${config.ow.namespace}`) |
There was a problem hiding this comment.
🤖 PR Reviewer
This PR is a significant refactor that replaces the AWS S3 SDK with a REST-based deploy-service API and bearer token authentication. The code is generally well-structured and tested, but several previously raised issues remain (stale comments in remote-storage.js and missing joi removal from package.json), and a few new issues exist around error messages, the config.s3?.folder usage in deploy-web.js, and the module-level side effect for URL initialization.
📝 8 suggestion(s) (1 new, 7 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
There was a problem hiding this comment.
🤖 PR Reviewer
This is a significant refactoring that replaces AWS S3 direct SDK usage with a new deploy-service API using fetch/undici. The overall approach is clean and well-tested. A few issues need attention: the module-level deploymentServiceUrl computation prevents environment mocking in tests without jest.resetModules(), there's a potential security concern with auth token storage, and some minor issues with error message consistency and the joi dependency remaining in package.json despite no longer being needed for validation.
📝 8 suggestion(s) - Please review inline comments below.
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| throw res.error | ||
| } | ||
| // First check AIO_DEPLOYMENT_SERVICE_URL environment variable | ||
| // If not set, use the default value based on the environment |
There was a problem hiding this comment.
The deploymentServiceUrl and fetchDispatcher are module-level singletons. This is fine for production but makes testing environment changes harder. This is already worked around in tests with jest.resetModules(), but it's worth noting the fetchDispatcher singleton is never reset between module reloads (since it's a closure), which is fine since jest.resetModules() discards the whole module.
| // If not set, use the default value based on the environment | |
| // No code change needed, but consider documenting the singleton behavior |
| this._authToken = authToken | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The JSDoc comment for the constructor parameter should note that the token is stored in memory. For security, consider not storing the token as an instance variable directly if it could be logged or serialized, though this is a minor concern for a server-side Node.js module.
| /** | |
| /** | |
| * Constructor for RemoteStorage | |
| * @param {string} authToken - The authorization token (e.g. 'Bearer <token>') to use for the remote storage | |
| * @private This token is stored in memory only and not serialized | |
| */ |
| headers: { | ||
| Authorization: this._authToken | ||
| } | ||
| }) |
There was a problem hiding this comment.
The emptyFolder method returns response.ok (a boolean) but the caller in undeploy-web.js checks if (!deleted) and throws. This is fine, but the method's JSDoc says it returns Promise<boolean> - however when auth check throws, it doesn't return false, it throws. The behavior is inconsistent: auth failure throws, but HTTP failure returns false. Consider making these consistent by throwing on HTTP failure too, or documenting the distinction.
| }) | |
| if (!response.ok) { | |
| throw new Error(`cannot empty folder, request failed: ${response.status} ${response.statusText}`) | |
| } | |
| return true |
| await remoteStorage.emptyFolder('/', config) | ||
| } | ||
| const _log = log ? (f) => log(`deploying ${path.relative(dist, f)}`) : null | ||
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) |
There was a problem hiding this comment.
The folderExists call passes '/' as prefix but the implementation ignores the prefix parameter entirely (it lists all files for the namespace). The prefix parameter in the new implementation is kept only for API compatibility but is misleading. Consider documenting this or simplifying the call.
| await remoteStorage.uploadDir(dist, config.s3.folder, config, _log) | |
| if (await remoteStorage.folderExists('/' /* prefix unused in new API */, config)) { |
| throw new Error('config.app.hostname and config.ow.namespace are required') | ||
| } | ||
| if (!config.app.hostname.match(/^[a-zA-Z0-9.-]+$/)) { | ||
| throw new Error('config.app.hostname is invalid') |
There was a problem hiding this comment.
The hostname validation regex /^[a-zA-Z0-9.-]+$/ is applied before the auth check and dist directory check. If the hostname or namespace validation is expensive or if order matters for UX, consider consolidating all config validation at the top. Also, this regex allows hostnames starting/ending with dots or hyphens which are technically invalid hostnames.
| throw new Error('config.app.hostname is invalid') | |
| if (!config.app.hostname.match(/^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?$/)) { | |
| throw new Error('config.app.hostname is invalid') | |
| } |
| "proxy-agent": "^6.3.0", | ||
| "regenerator-runtime": "^0.13.7" | ||
| "regenerator-runtime": "^0.13.7", | ||
| "undici": "^7.25.0" |
There was a problem hiding this comment.
The joi package is still listed as a dependency in package.json but validation using joi was removed from remote-storage.js constructor. Check if joi is used elsewhere; if not, it should be removed to reduce bundle size.
| "undici": "^7.25.0" | |
| // Remove 'joi' from dependencies if not used elsewhere |
| // ideally we deprecate cache control config in favor of response headers directly | ||
| if (responseHeaders?.['adp-cache-control']) { | ||
| uploadParams.CacheControl = responseHeaders['adp-cache-control'] | ||
| cacheControlString = responseHeaders['adp-cache-control'] |
There was a problem hiding this comment.
File content is read with fs.readFile which returns a Buffer, then Buffer.from(content).toString('base64') is called. Since content is already a Buffer, Buffer.from(content) creates an unnecessary copy. Use content.toString('base64') directly.
| cacheControlString = responseHeaders['adp-cache-control'] | |
| content: content.toString('base64') |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a substantial refactor replacing AWS S3/TVM-based storage with a new deploy-service HTTP API. The migration is well-structured with good test coverage. Several previously raised issues remain unaddressed, and a few new issues are present including the Buffer.from(content) redundancy and the hostname regex allowing invalid formats.
📝 6 suggestion(s) (1 new, 5 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| httpsAgent: agent | ||
| }) | ||
| } | ||
| const deployServiceFetch = (url, init) => { |
There was a problem hiding this comment.
[Re-raised] The deployServiceFetch function accesses the global fetch. If fetch is not available in the Node.js version being used, this will fail silently at runtime. Consider adding a guard or using undici's fetch directly.
| const deployServiceFetch = (url, init) => { | |
| const deployServiceFetch = (url, init) => { | |
| const fetchFn = typeof fetch !== 'undefined' ? fetch : require('undici').fetch | |
| return fetchFn(url, { ...init, dispatcher: getFetchDispatcher() }) | |
| } |
| const response = await deployServiceFetch(url, { | ||
| method: 'PUT', | ||
| body: JSON.stringify(data), | ||
| headers: { |
There was a problem hiding this comment.
[Re-raised] Buffer.from(content) creates an unnecessary copy since content is already a Buffer returned by fs.readFile. Use content.toString('base64') directly.
| headers: { | |
| content: content.toString('base64') |
| method: 'GET', | ||
| headers: { | ||
| Authorization: this._authToken | ||
| } |
There was a problem hiding this comment.
[Re-raised] emptyFolder returns response.ok (boolean false) on HTTP failure, but throws on auth failure. The behavior is inconsistent. Consider throwing on HTTP failure too for uniform error handling, since the caller in undeploy-web.js explicitly checks the return value and throws anyway.
| } | |
| if (!response.ok) { | |
| throw new Error(`cannot empty folder, request failed: ${response.status} ${response.statusText}`) | |
| } | |
| return true |
|
|
||
| if (exists) { | ||
| const remoteStorage = new RemoteStorage(bearerToken) | ||
| // validate config.app.hostname and config.ow.namespace are not empty and are valid and do not contain invalid characters |
There was a problem hiding this comment.
[Re-raised] The hostname regex /^[a-zA-Z0-9.-]+$/ allows hostnames starting or ending with dots or hyphens (e.g. -bad.host, bad.host.), which are technically invalid. Use a stricter regex.
| // validate config.app.hostname and config.ow.namespace are not empty and are valid and do not contain invalid characters | |
| if (!config.app.hostname.match(/^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?$/)) { | |
| throw new Error('config.app.hostname is invalid') | |
| } |
| await remoteStorage.emptyFolder(config.s3.folder + '/') | ||
| await remoteStorage.emptyFolder('/', config) | ||
| } | ||
| const _log = log ? (f) => log(`deploying ${path.relative(dist, f)}`) : null |
There was a problem hiding this comment.
[Re-raised] folderExists is called with '/' but the implementation ignores the prefix parameter entirely. Consider documenting this explicitly to avoid future confusion.
| const _log = log ? (f) => log(`deploying ${path.relative(dist, f)}`) : null | |
| if (await remoteStorage.folderExists('/' /* prefix unused, kept for API compatibility */, config)) { |
| if (!this._authToken) { | ||
| throw new Error('cannot upload file, Authorization is required') | ||
| } | ||
|
|
There was a problem hiding this comment.
The uploadFile method silently passes false as contentType to the server when the MIME type is unknown (mime-types returns false for unknown extensions). The server may not handle false gracefully. Consider omitting the field or using a fallback like 'application/octet-stream'.
| contentType: mimeType || 'application/octet-stream', |
…rs responsibility
There was a problem hiding this comment.
🤖 PR Reviewer
This is a significant refactor replacing AWS S3/TVM-based deployment with a new deploy-service HTTP API using undici's fetch. The code is generally well-structured with good test coverage. Several previously raised issues remain unaddressed, and a few new concerns are present in the diff.
📝 7 suggestion(s) (2 new, 5 re-raised)
⚠️ Diff exceeded 100 000 chars and was truncated — some files may not have been reviewed.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| Prefix: prefix | ||
| // Call the list files endpoint (GET /files) - there is no GET /files/:key route | ||
| const response = await deployServiceFetch(`${getDeploymentServiceUrl()}/cdn-api/namespaces/${appConfig.ow.namespace}/files`, { | ||
| method: 'GET', |
There was a problem hiding this comment.
[Re-raised] The deployServiceFetch function accesses the global fetch. If fetch is not available in the Node.js version being used, this will fail silently at runtime. Consider adding a guard or using undici's fetch directly.
| method: 'GET', | |
| const deployServiceFetch = (url, init) => { | |
| const fetchFn = typeof fetch !== 'undefined' ? fetch : require('undici').fetch | |
| return fetchFn(url, { ...init, dispatcher: getFetchDispatcher() }) | |
| } |
| }).catch(error => { | ||
| console.error('Error uploading file:', file) | ||
| throw error | ||
| }) |
There was a problem hiding this comment.
[Re-raised] Buffer.from(content) creates an unnecessary copy since content is already a Buffer returned by fs.readFile. Use content.toString('base64') directly.
| }) | |
| content: content.toString('base64') |
| * This becomes file.name in the API request. The server will prepend the namespace. | ||
| * @param {Object} appConfig - application config | ||
| * @param {string} distRoot - Distribution root dir | ||
| * @param {string} distRoot - Distribution root dir (used for header matching) |
There was a problem hiding this comment.
[Re-raised] emptyFolder returns response.ok (boolean false) on HTTP failure, but throws on other errors. The behavior is inconsistent. Consider throwing on HTTP failure too for uniform error handling, since the caller in undeploy-web.js explicitly checks the return value and throws anyway.
| * @param {string} distRoot - Distribution root dir (used for header matching) | |
| if (!response.ok) { | |
| throw new Error(`cannot empty folder, request failed: ${response.status} ${response.statusText}`) | |
| } | |
| return true |
| if (exists) { | ||
| const remoteStorage = new RemoteStorage() | ||
| // validate config.app.hostname and config.ow.namespace are not empty and are valid and do not contain invalid characters | ||
| if (!config.app.hostname || !config.ow.namespace) { |
There was a problem hiding this comment.
[Re-raised] The hostname regex /^[a-zA-Z0-9.-]+$/ allows hostnames starting or ending with dots or hyphens (e.g. -bad.host, bad.host.), which are technically invalid. Use a stricter regex.
| if (!config.app.hostname || !config.ow.namespace) { | |
| if (!config.app.hostname.match(/^[a-zA-Z0-9]([a-zA-Z0-9.-]*[a-zA-Z0-9])?$/)) { | |
| throw new Error('config.app.hostname is invalid') | |
| } |
| }).catch(error => { | ||
| console.error('Error uploading file:', file) | ||
| throw error | ||
| }) |
There was a problem hiding this comment.
[Re-raised] uploadFile silently passes false as contentType to the server when the MIME type is unknown (mime-types returns false for unknown extensions). The server may not handle false gracefully. Consider using a fallback like 'application/octet-stream'.
| }) | |
| contentType: mimeType || 'application/octet-stream', |
| * @param {string} prefix - unused, kept for API compatibility | ||
| * @param {Object} appConfig - application config | ||
| * @returns {Promise<boolean>} true if files exist, false otherwise | ||
| */ |
There was a problem hiding this comment.
The fetchDispatcher is a module-level singleton. This means it persists across tests and potentially across different runtime configurations if the module is cached. While lazy initialization is fine, consider documenting that fetchDispatcher is intentionally a singleton and cannot be reset without clearing the module cache.
| */ | |
| // Module-level singleton: intentionally shared across all requests for connection reuse. | |
| // To reset (e.g., in tests), clear the require cache for this module. | |
| let fetchDispatcher |
| EnvHttpProxyAgent: jest.fn(() => mockProxyDispatcher) | ||
| })) | ||
|
|
||
| const { EnvHttpProxyAgent } = require('undici') |
There was a problem hiding this comment.
The EnvHttpProxyAgent singleton test may be flaky across test suites because fetchDispatcher is a module-level variable that persists between test files. The test asserts toHaveBeenCalledTimes(1) but if a previous test in another file already triggered initialization, the count would be wrong. Consider resetting the module between tests with jest.resetModules() or isolating the module.
|
closing in favor of new clean pr |
1 participant
Summary
Migrates static web deploy and undeploy from direct AWS S3 access (TVM / BYO credentials) to the App Builder deploy-service CDN API, using the user’s bearer token instead of short-lived S3 keys.
Storage layer (RemoteStorage)
Deploy / undeploy entry points
Removed / dependencies
Tests
Motivation
App Builder static hosting is moving behind deploy-service so clients no longer need S3 credentials or direct bucket access. This aligns aio-lib-web with that model and simplifies credential handling to a single OAuth/bearer flow the CLI already provides.
Breaking changes
Checklist: