-
Notifications
You must be signed in to change notification settings - Fork 12
Deploy service #232
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
base: master
Are you sure you want to change the base?
Deploy service #232
Conversation
This reverts commit 27e548c.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request migrates from direct AWS S3 access to a deployment service API, representing a significant architectural change in how static web files are deployed and managed.
Changes:
- Replaced AWS S3 SDK with fetch-based HTTP API calls to a new deployment service
- Removed TVM (Token Vending Machine) credential management in favor of direct auth token usage via
auth_handler - Updated all deployment and undeployment logic to use the new API endpoints
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/remote-storage.js | Complete rewrite from S3 SDK to fetch-based HTTP API; removed proxy and credential handling, added new deployment service URL selection |
| lib/getS3Creds.js | Deleted - credential fetching no longer needed with new auth approach |
| src/deploy-web.js | Updated to use auth_handler.getAuthHeader() instead of S3 credentials; removed folder existence check and cleanup logic |
| src/undeploy-web.js | Updated to use auth_handler.getAuthHeader() instead of S3 credentials; changed to use '/' prefix for API calls |
| test/lib/remote-storage.test.js | Completely rewritten to test new HTTP API instead of S3 SDK; added environment URL selection tests |
| test/lib/getS3Creds.test.js | Deleted - tests for removed functionality |
| test/src/deploy-web.test.js | Updated all tests to mock new auth approach and removed S3-specific test scenarios |
| test/src/undeploy-web.test.js | Updated all tests to mock new auth approach and verify new API call patterns |
| test/jest.setup.js | Added new test fixtures for auth tokens and namespace configuration |
| package.json | Replaced @aws-sdk/client-s3 with @adobe/aio-lib-env |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| httpsAgent: agent | ||
| }) | ||
| // Call the list files endpoint (GET /files) - there is no GET /files/:key route | ||
| const response = await fetch(`${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files`, { |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL construction on line 54 directly interpolates appConfig.ow.namespace into the URL without any validation or sanitization. If the namespace contains special characters, spaces, or URL-unsafe characters, this could result in malformed URLs. Consider using encodeURIComponent(appConfig.ow.namespace) to properly encode the namespace for use in the URL path.
| const response = await fetch(`${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files`, { | |
| const response = await fetch(`${deploymentServiceUrl}/cdn-api/namespaces/${encodeURIComponent(appConfig.ow.namespace)}/files`, { |
| // file.name is the path relative to namespace (e.g., 'images/photo.jpg' or 'index.html') | ||
| // The server will prepend the namespace to create the S3 key: ${namespace}/${file.name} | ||
| const fileName = path.basename(file) | ||
| let relativeFilePath = filePath.replace(appConfig.ow.namespace, '') |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for constructing relativeFilePath on line 130 appears incorrect. The code replaces appConfig.ow.namespace from filePath, but filePath is the path prefix (like 'somefolder' from config.s3.folder) and is unlikely to contain the namespace. This replacement logic seems misplaced and could cause issues if the namespace string happens to match part of the file path. The purpose of this line is unclear and should be reviewed - it may be attempting to handle a case that doesn't actually occur in practice.
| let relativeFilePath = filePath.replace(appConfig.ow.namespace, '') | |
| let relativeFilePath = filePath |
| if (typeof prefix !== 'string') { | ||
| throw new Error('prefix must be a valid string') | ||
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The folderExists method comment on line 42 states that the prefix parameter is "unused, kept for API compatibility". However, the parameter is validated as a string on lines 47-49 and would throw an error if undefined. This is inconsistent - if the parameter is truly unused, the validation should be removed, or the comment should be updated to reflect its actual usage. Based on the implementation, it appears the prefix is indeed unused for the actual API call (line 54 doesn't use it), so the validation should probably be removed to match the stated intent.
| if (typeof prefix !== 'string') { | |
| throw new Error('prefix must be a valid string') | |
| } |
| "@adobe/aio-lib-env": "^3.0.1", | ||
| "@smithy/node-http-handler": "^4.0.2", |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AWS SDK dependency @aws-sdk/client-s3 has been removed from package.json (line 35 in the diff), but @smithy/node-http-handler (line 36) and proxy-agent (line 44) remain as dependencies. These packages were used for proxy support with the AWS SDK and are no longer needed with the fetch-based implementation. They should be removed to avoid unnecessary dependencies.
| if (!response.ok) { | ||
| return false | ||
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The folderExists method returns false when the fetch request fails (lines 60-62), which masks potential errors. A network error, authentication failure, or server error would all be interpreted as "folder doesn't exist". This could mislead users about the actual problem. Consider throwing an error for non-404 status codes or logging the error details, so that genuine errors aren't silently treated as "folder doesn't exist".
| if (!response.ok) { | |
| return false | |
| } | |
| // 404 means the namespace/folder does not exist -> return false | |
| if (response.status === 404) { | |
| return false | |
| } | |
| // Other non-OK responses indicate an actual error and should not be | |
| // silently treated as "folder doesn't exist" | |
| if (!response.ok) { | |
| throw new Error(`Failed to check if folder exists (status ${response.status} ${response.statusText})`) | |
| } |
| if (listedObjects.IsTruncated) { | ||
| await this.emptyFolder(prefix) | ||
| } | ||
| return response.ok |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The emptyFolder method returns false when the delete request fails (line 94), silently swallowing any errors. This makes it difficult to diagnose issues - a network error, authentication failure, or server error would all just return false with no indication of what went wrong. Consider throwing an error with details about the failure, or at minimum logging the error information before returning false.
| return response.ok | |
| if (!response.ok) { | |
| let errorDetails = '' | |
| try { | |
| const bodyText = await response.text() | |
| if (bodyText) { | |
| errorDetails = ` Response body: ${bodyText}` | |
| } | |
| } catch (e) { | |
| // ignore errors while reading response body; we still have status information | |
| } | |
| const message = `Failed to empty folder at '${url}'. Status: ${response.status} ${response.statusText}.${errorDetails}` | |
| logAndThrow(codes.STORAGE_ERROR, message) | |
| } | |
| return true |
| } | ||
| }) | ||
|
|
||
| await rs.uploadFile('fakeDir/index.js', 'fakeprefix', newConfig, fakeDistRoot, global.fakeAuthToken) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test calls uploadFile with a fifth authToken parameter that shouldn't exist. This parameter is unused and redundant since RemoteStorage already has the auth token as an instance variable (passed to constructor on line 669). This test should call uploadFile without the auth token parameter: await rs.uploadFile('fakeDir/index.js', 'fakeprefix', newConfig, fakeDistRoot)
| } | ||
| }) | ||
|
|
||
| await rs.uploadFile(filePath, 'fakeprefix', newConfig, fakeDistRoot, global.fakeAuthToken) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test calls uploadFile with a fifth authToken parameter that shouldn't exist. This parameter is unused and redundant since RemoteStorage already has the auth token as an instance variable. This test should call uploadFile without the auth token parameter: await rs.uploadFile(filePath, 'fakeprefix', newConfig, fakeDistRoot)
| const appConfig = createAppConfig() | ||
| delete appConfig.web // No web.response-headers | ||
|
|
||
| await rs.uploadFile('fakeDir/index.js', 'fakeprefix', appConfig, 'fakeDir', global.fakeAuthToken) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test calls uploadFile with a fifth authToken parameter that shouldn't exist. This parameter is unused and redundant since RemoteStorage already has the auth token as an instance variable. This test should call uploadFile without the auth token parameter: await rs.uploadFile('fakeDir/index.js', 'fakeprefix', appConfig, 'fakeDir')
| const response = await fetch(`${deploymentServiceUrl}/cdn-api/namespaces/${appConfig.ow.namespace}/files`, { | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: this._authToken | ||
| } | ||
| }) | ||
| if (!response.ok) { | ||
| return false | ||
| } | ||
|
|
||
| // https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/clients/client-s3/classes/s3.html#constructor | ||
| this.s3 = new S3(s3Config) | ||
| const files = await response.json() | ||
| // Check if there are any files (folder "exists" if it has content) | ||
| return Array.isArray(files) && files.length > 0 |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The folderExists method doesn't handle potential network errors that could occur during the fetch request. While line 63 calls response.json(), if the fetch itself throws (network error, DNS failure, etc.), the exception will propagate uncaught. Consider wrapping the fetch call in a try-catch block to provide better error handling and messaging for network-level failures.
Description
Switch to calling our own deploy-service instead of S3 directly
Related Issue
ACNA-3810
cdn-next milestone 4
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: