feat: parallel upload of test report #920
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
himanshu-dixit
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 7e237f5 in 9 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_9uzrNpu24DAO2QAb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
.github/workflows/run_js_test.yml
Outdated
| @@ -37,7 +37,7 @@ jobs: | |||
|
|
|||
| - name: Upload `coverage` folder to R2 | |||
| if: ${{ always() }} | |||
| uses: ryand56/r2-upload-action@latest | |||
| uses: himanshu-dixit/r2-upload-action-parallel@v1 | |||
There was a problem hiding this comment.
While changing to a parallel upload action is a good optimization, there are a few concerns:
-
The action change is only applied to the coverage folder upload but not to the HTML report upload (line 56). For consistency and optimal performance, consider updating both upload steps to use the parallel version.
-
Using a specific version (
@v1) is better than@latest, but please add:- Link to the action's source code repository
- Brief comment explaining the performance benefits
- Consider documenting any benchmarks/improvements seen
-
Consider adding error handling or fallback mechanism in case the parallel upload fails.
Code Review SummaryChanges Overview
Positive Aspects✅ Moving from Areas for Improvement
Recommendations
Overall AssessmentThe change is a positive optimization but needs some additional work for completeness and reliability. |
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 94ed5a6 in 7 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/run_js_test.yml:40
- Draft comment:
Ensure that the new versionv1.1ofhimanshu-dixit/r2-upload-action-parallelis compatible and doesn't introduce breaking changes compared tov1. - Reason this comment was not posted:
Confidence changes required:50%
The PR updates the action version from v1 to v1.1, which is a minor version update. It's important to ensure that the new version is compatible and doesn't introduce breaking changes.
Workflow ID: wflow_9S3U71htNbvbFc6E
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1d38206 in 8 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/run_js_test.yml:56
- Draft comment:
The PR description mentions updating the upload action for thecoveragefolder, but the same action is also used for thehtml-reportfolder. Consider updating the PR description to reflect this. - Reason this comment was not posted:
Confidence changes required:50%
The PR updates the action used for uploading the coverage folder to a parallel upload action. This change is correctly reflected in the workflow file. However, the PR description mentions updating the upload action for thecoveragefolder, but the same action is also used for thehtml-reportfolder. This should be clarified in the description.
Workflow ID: wflow_3NjCpkm04hGnWeAR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9cc20be in 9 seconds
More details
- Looked at
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/run_js_test.yml:40
- Draft comment:
Ensure that the action versionv1.2is compatible with your workflow requirements and doesn't introduce breaking changes. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions updating to version v1.2 of the action, but the diff shows the change from v1.1 to v1.2. This is consistent with the description, so no issue here.
Workflow ID: wflow_O9uxlt8LlMF1lVp5
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2ad2cc6 in 9 seconds
More details
- Looked at
52lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. .github/workflows/run_js_test.yml:40
- Draft comment:
The PR description mentions updating to version v1.2, but the code changes show an update to v1.3. Please update the PR description for consistency. - Reason this comment was not posted:
Confidence changes required:50%
The PR description mentions updating to version v1.2, but the code changes show an update to v1.3. This discrepancy should be addressed for clarity.
Workflow ID: wflow_RJgbW2rz55rJ03aH
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
himanshu-dixit
force-pushed
the
ft-faster-r2-upload
branch
from
ac007de to
d0931d7
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on d0931d7 in 41 seconds
More details
- Looked at
284lines of code in7files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/sdk/base.toolset.ts:171
- Draft comment:
Good job removing the console.log statement for cleaner production code. - Reason this comment was not posted:
Confidence changes required:10%
The console.log statement in base.toolset.ts was removed, which is a good practice for production code. No further action needed.
2. js/src/sdk/index.ts:49
- Draft comment:
Good practice to redact API keys in logs for security. - Reason this comment was not posted:
Confidence changes required:10%
The logger in index.ts now uses a redacted API key, which is a good security practice.
Workflow ID: wflow_v1X1wHVYYflotXUb
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
js/sample.ts
Outdated
| import {Composio} from "./src/index" | ||
|
|
||
| async function main() { | ||
| const composio = new Composio("u7ta9pbl3iz37qap6wdnk"); |
There was a problem hiding this comment.
Hardcoding API keys is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
| const composio = new Composio("u7ta9pbl3iz37qap6wdnk"); | |
| const composio = new Composio(process.env.COMPOSIO_API_KEY); |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8dedad5 in 10 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_MeGpVBGl58zRBM4I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const sanitizedConfig = { | ||
| ...this.workspaceConfig, | ||
| host: this.workspaceConfig.config.composioBaseURL, | ||
| composioAPIKey: this.workspaceConfig.config.composioAPIKey ? "REDACTED" : "NOT DEFINED" |
There was a problem hiding this comment.
Chore: Let's use constant values for "REDACTED" and "NOT DEFINED". It would be easy to reuse and change in future, if required
| */ | ||
| const getLogLevel = (): keyof typeof levels => { | ||
| const envLevel = getEnvVariable("COMPOSIO_DEBUG", "0"); | ||
| return envLevel === "1" ? 'debug' : 'info'; | ||
| const envLevel = getEnvVariable("COMPOSIO_LOGGING_LEVEL", "info"); |
There was a problem hiding this comment.
Let's use constants for this as well
…into ft-faster-r2-upload
Important
Enhances GitHub Actions for parallel uploads and improves logging by sanitizing API keys and using structured logging.
run_js_test.ymlto usehimanshu-dixit/r2-upload-action-parallel@v1.3for parallel uploads ofcoverageandjest html-reportersfolders to R2.ryand56/r2-upload-action@latestwithhimanshu-dixit/r2-upload-action-parallel@v1.3.factory.ts, modifies logging inWorkspaceFactory.new()to sanitizecomposioAPIKeyinworkspaceConfig.console.logwithlogger.infoinEntity.tsandindex.tsfor better logging practices.setAxiosClientConfiginconfig.tsto add request and response interceptors for logging API calls.sample.tsas a new file for testing or demonstration purposes.This description was created by
for 8dedad5. It will automatically update as commits are pushed.