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
| const allResults = [] | ||
| if (!this._authToken) { | ||
| throw new Error('cannot upload files, Authorization is required') | ||
| } |
There was a problem hiding this comment.
Commented-out sleep/rate-limiting code should be removed or replaced with a real implementation if rate limiting is a concern.
| "@adobe/aio-lib-core-tvm": "^4", | ||
| "@aws-sdk/client-s3": "^3.1045.0", | ||
| "@adobe/aio-lib-env": "^3.0.1", | ||
| "@smithy/node-http-handler": "^4.0.2", |
There was a problem hiding this comment.
@smithy/node-http-handler is still listed as a dependency but its usage in remote-storage.js was commented out. If it's no longer needed, it should be removed from dependencies.
| throw new Error('config.app.hostname and config.ow.namespace are invalid') | ||
| } | ||
| if (await remoteStorage.folderExists('/', config)) { | ||
| await remoteStorage.emptyFolder('/', config) |
There was a problem hiding this comment.
The warning message about overwriting an existing deployment was removed. Users will no longer be informed before their deployment is overwritten, which could be surprising. Consider re-adding the log warning.
| await remoteStorage.emptyFolder('/', config) | |
| if (await remoteStorage.folderExists('/', config)) { | |
| if (log) { | |
| log('warning: an existing deployment will be overwritten') | |
| } | |
| await remoteStorage.emptyFolder('/', config) | |
| } |
| if (!config.app.hostname || !config.ow.namespace) { | ||
| throw new Error('config.app.hostname and config.ow.namespace are required') | ||
| } | ||
| if (!config.app.hostname.match(/^[a-zA-Z0-9.-]+$/) || !config.ow.namespace.match(/^[a-zA-Z0-9.-]+$/)) { |
There was a problem hiding this comment.
The regex /^[a-zA-Z0-9.-]+$/ for hostname validation disallows valid hostnames with underscores (though uncommon) and may be overly strict or too lenient depending on requirements. Additionally, the error message doesn't distinguish which field is invalid, making debugging harder.
| if (!config.app.hostname.match(/^[a-zA-Z0-9.-]+$/) || !config.ow.namespace.match(/^[a-zA-Z0-9.-]+$/)) { | |
| if (!config.app.hostname.match(/^[a-zA-Z0-9.-]+$/)) { | |
| throw new Error('config.app.hostname is invalid') | |
| } | |
| if (!config.ow.namespace.match(/^[a-zA-Z0-9_-]+$/)) { | |
| throw new Error('config.ow.namespace is invalid') | |
| } |
| const { NodeHttpHandler } = require('@smithy/node-http-handler') | ||
| const { ProxyAgent } = require('proxy-agent') | ||
| // const { NodeHttpHandler } = require('@smithy/node-http-handler') | ||
| // const { ProxyAgent } = require('proxy-agent') |
There was a problem hiding this comment.
The commented-out imports for NodeHttpHandler and ProxyAgent suggest proxy support was removed. If proxy support is intentionally dropped, ensure this is documented. If it should be preserved, the fetch API used as replacement does not automatically honor HTTP_PROXY/HTTPS_PROXY environment variables in Node.js without additional configuration (e.g., using a custom dispatcher with undici/node-fetch).
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
| const { getCliEnv, PROD_ENV } = require('@adobe/aio-lib-env') | ||
|
|
||
| const fileExtensionPattern = /\*\.[0-9a-zA-Z]+$/ | ||
| // or https://deploy-service.dev.app-builder.adp.adobe.io |
There was a problem hiding this comment.
[Re-raised] Commented-out code and stale comments ('// or https://...', '// or http://localhost:3000', etc.) should be removed before merging. These add noise and suggest incomplete cleanup.
| // or https://deploy-service.dev.app-builder.adp.adobe.io | |
| 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') |
| 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}`) |
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: