Deploy service cdn#245
Conversation
This reverts commit 27e548c.
…rs responsibility
![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 PR refactors the remote storage layer from direct AWS S3 SDK usage to a new deploy-service HTTP API, replacing TVM credentials with bearer token auth. The code is generally clean and well-structured with good test coverage. A few issues remain around the module-level singleton pattern for the fetch dispatcher, error message detail, and minor security/robustness concerns.
📝 5 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
| return aioLibEnv.getCliEnv() === aioLibEnv.PROD_ENV | ||
| ? 'https://deploy-service.app-builder.adp.adobe.io' | ||
| : 'https://deploy-service.stg.app-builder.corp.adp.adobe.io' | ||
| } |
There was a problem hiding this comment.
The fetchDispatcher module-level singleton is never reset between tests or process restarts. If EnvHttpProxyAgent initialization fails or the proxy environment changes at runtime, there's no way to refresh it. Additionally, this pattern makes the module harder to test in isolation since jest module resets won't clear it. Consider using a lazy factory pattern that at minimum allows resetting, or instantiate it per-request if the overhead is acceptable.
| } | |
| let fetchDispatcher | |
| const getFetchDispatcher = () => { | |
| if (!fetchDispatcher) { | |
| fetchDispatcher = new EnvHttpProxyAgent() | |
| } | |
| return fetchDispatcher | |
| } | |
| // Exported only for testing purposes | |
| module.exports._resetFetchDispatcher = () => { fetchDispatcher = undefined } |
| const deploymentServiceUrl = getDeploymentServiceUrl() | ||
| const url = prefix === '/' | ||
| ? `${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files/` | ||
| : `${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files/${prefix}` |
There was a problem hiding this comment.
The error message for a failed upload doesn't include the HTTP status code, only the status text. This makes debugging harder. Include response.status in the message, consistent with the folderExists error at line 84.
| : `${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files/${prefix}` | |
| throw new Error(`Failed to upload file: ${response.status} ${response.statusText}`) |
| this.s3 = new S3(s3Config) | ||
| let fetchDispatcher | ||
| const getFetchDispatcher = () => { | ||
| if (!fetchDispatcher) { |
There was a problem hiding this comment.
The deployServiceFetch function spreads init and then adds dispatcher, which means a caller-supplied dispatcher in init would be silently overridden. The spread order should put dispatcher before the spread, or document that it always overrides.
| if (!fetchDispatcher) { | |
| const deployServiceFetch = (url, init) => { | |
| return fetch(url, { dispatcher: getFetchDispatcher(), ...init }) | |
| } |
| @@ -30,19 +34,25 @@ const deployWeb = async (config, log) => { | |||
| throw new Error(`missing files in ${dist}, maybe you forgot to build your UI ?`) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The config?.ow?.auth_handler?.getAuthHeader() call could theoretically return a non-string truthy value or an empty string that passes the truthiness check but is an invalid bearer token. Consider validating that the result is a non-empty string.
| const bearerToken = await config?.ow?.auth_handler?.getAuthHeader() | |
| if (!bearerToken || typeof bearerToken !== 'string') { | |
| throw new Error('cannot deploy web, Authorization is required') | |
| } |
| method: 'GET', | ||
| headers: { | ||
| Authorization: authToken | ||
| } |
There was a problem hiding this comment.
The URL construction for emptyFolder when prefix is / results in a trailing slash (/files/). While this is intentional per the comment, it's fragile—if the server normalizes the URL and strips the trailing slash, the behavior would silently change to "delete nothing" or "delete a file named empty string". This design should be explicitly documented and ideally replaced with a dedicated query parameter or a separate endpoint path like /files?deleteAll=true to make the intent unambiguous.
| } | |
| // Use explicit query param to signal full namespace deletion instead of relying on trailing slash | |
| const url = prefix === '/' | |
| ? `${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files?deleteAll=true` | |
| : `${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files/${prefix}` |
chsrimanaswi
left a comment
chsrimanaswi
left a comment
There was a problem hiding this comment.
🤖 PR Reviewer
This is a substantial refactor replacing AWS S3 SDK with a custom deploy-service HTTP API using undici's fetch. The migration is clean and well-tested. Two previously raised issues remain unaddressed (missing status code in upload error message, and the dispatcher override order in deployServiceFetch), and the trailing slash URL fragility concern for emptyFolder is still present. The bearerToken type validation suggestion was partially addressed with a null check but not the non-string type guard.
🔄 3 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
chsrimanaswi
dismissed stale reviews from github-actions[bot] and themself
Superseded by new review
2 participants
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: