Skip to content
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

*: replace the bazelisk, disk and repo cache with sticky disks #3

Merged
merged 1 commit into from
Dec 15, 2024

Conversation

adityamaru
Copy link

@adityamaru adityamaru commented Dec 14, 2024

This change teaches restoreCache to get and mount a unique stickydisk per key and mount it at the provided path. We require a stickydisk per path and so we hash the path and add it to the user configured "cache key". All other semantics around versioning of the cache and respecting the base cache key remain the same.

On post, we teach the action to unmount and commit the relevant stickydisks. For the moment we leave the "external" cache alone. In a follow-up we can move that to be backed by stickydisks too.

The stickydisk.js file is heavily inspired by the useblacksmith/stickydisk implementation.


Important

Replaces existing cache mechanism with sticky disks, updating mounting and unmounting logic, configuration, and adding tests.

  • Behavior:
    • Replace restoreCache with loadStickyDisk in index.js to mount sticky disks per cache path.
    • post.js unmounts and commits sticky disks using unmountAndCommitStickyDisk.
    • External cache handling remains unchanged.
  • Sticky Disk Implementation:
    • stickydisk.js provides functions to manage sticky disks, including mountStickyDisk and unmountAndCommitStickyDisk.
    • Utilizes createStickyDiskClient from util.js for sticky disk service interaction.
  • Configuration:
    • config.js updated to support sticky disk cache configuration.
    • package.json includes @buf/blacksmith_vm-agent.connectrpc_es for sticky disk operations.
  • Testing:
    • New test suite restore-cache.test.js added to test sticky disk functionality.
    • jest.config.js updated to include coverage for stickydisk.js.
  • Misc:
    • Minor fix in config.js for diskCacheConfig variable check.
    • CI workflow in .github/workflows/ci.yml updated to include a test job.

This description was created by Ellipsis for ecb4f37. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 7e07ea7 in 1 minute and 31 seconds

More details
  • Looked at 423 lines of code in 6 files
  • Skipped 8 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. config.js:44
  • Draft comment:
    The change from diskCacheName !== 'true' to diskCacheConfig !== 'true' is a bug fix. The original condition was incorrect as it was checking the wrong variable. This change ensures the logic aligns with the intended behavior.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. index.js:150
  • Draft comment:
    Using setTimeout with an async function is incorrect as it does not await the async function. Consider refactoring to ensure the async function is awaited properly.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_PHoPQPG1NLdBKSUN


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.

util.js Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 2ff84e4 in 57 seconds

More details
  • Looked at 937 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. post.js:14
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending asynchronous operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. util.js:9
  • Draft comment:
    Consider using asynchronous methods instead of lstatSync to avoid blocking the event loop, especially if this function is used in a performance-sensitive context.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    Ensure to clear the timeout if the operation completes before the timeout duration to avoid unnecessary operations.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. index.js:151
  • Draft comment:
    Consider using a promise-based approach with setTimeout for better control over asynchronous execution.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In index.js, the loadStickyDisk function uses setTimeout with a callback function. This can lead to unexpected behavior if the callback is not executed in the intended context. Consider using setTimeout with a promise-based approach for better control.

Workflow ID: wflow_1rXi9mjrjZnnDNvK


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on d32fc67 in 55 seconds

More details
  • Looked at 942 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. post.js:14
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. stickydisk.js:106
  • Draft comment:
    Consider reusing a single client instance for createStickyDiskClient() to avoid creating a new client for each call.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The commitStickydisk and cleanupStickyDiskWithoutCommit functions in stickydisk.js both use createStickyDiskClient() to create a new client for each call. This could be optimized by reusing a single client instance if possible.
3. stickydisk.js:46
  • Draft comment:
    Ensure the device variable is properly sanitized to prevent command injection vulnerabilities when using execAsync.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. index.js:154
  • Draft comment:
    Ensure that the files specified for glob.hashFiles are correct and that the hashing process is efficient for large file sets.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In index.js, the loadStickyDisk function uses glob.hashFiles to generate a hash. Ensure that the files being hashed are correctly specified and that the hashing process is efficient for large file sets.

Workflow ID: wflow_ZICKE5FviSMhcoSB


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.

try {
// Check if path is mounted
try {
const { stdout: mountOutput } = await execAsync(`mount | grep ${path}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using grep to check if a path is mounted is not portable across different OS. Consider using a cross-platform method.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 7276e91 in 51 seconds

More details
  • Looked at 941 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. post.js:13
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending asynchronous operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. stickydisk.js:21
  • Draft comment:
    Consider using a configuration object or module to manage environment variables instead of directly accessing process.env. This improves testability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The getStickyDisk function in stickydisk.js uses process.env to access environment variables. It would be more robust to use a configuration object or a dedicated configuration module to manage these variables, making the code more testable and maintainable.
3. stickydisk.js:164
  • Draft comment:
    Consider making the retry delay in unmountAndCommitStickyDisk configurable to allow flexibility in handling different environments or conditions.
  • Reason this comment was not posted:
    Confidence changes required: 40%
    The unmountAndCommitStickyDisk function in stickydisk.js uses a loop with retries for unmounting. It would be beneficial to add a delay between retries to avoid rapid consecutive attempts, which is already done but could be more configurable.
4. index.js:150
  • Draft comment:
    Consider using a structured backoff strategy, like exponential backoff, instead of a random delay to handle contention more effectively.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The loadStickyDisk function in index.js uses setTimeout with a random delay to reduce contention. This approach can be improved by using a more structured backoff strategy, such as exponential backoff, to handle contention more effectively.
5. __tests__/restore-cache.test.js:34
  • Draft comment:
    Consolidate the jest.mock setup for @actions/core to avoid repetition and improve clarity.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The jest.mock for @actions/core in restore-cache.test.js is repeated twice, which is unnecessary and can lead to confusion. Consolidating the mock setup would improve clarity and maintainability.

Workflow ID: wflow_fZo4EhYog6Ox6aDe


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 0a80e72 in 1 minute and 38 seconds

More details
  • Looked at 942 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. post.js:13
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. stickydisk.js:83
  • Draft comment:
    Ensure to clear the timeout if the operation completes before the timeout duration to prevent unexpected behavior.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. stickydisk.js:106
  • Draft comment:
    Consider reusing a single client instance for createStickyDiskClient to reduce overhead, instead of creating a new client for each call.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In stickydisk.js, the commitStickydisk and cleanupStickyDiskWithoutCommit functions both use createStickyDiskClient to create a new client for each call. This can be optimized by reusing a single client instance if possible, to reduce overhead.
4. stickydisk.js:166
  • Draft comment:
    Ensure that inputs to shell commands are sanitized to prevent command injection vulnerabilities.
  • Reason this comment was not posted:
    Comment did not seem useful.
5. index.js:154
  • Draft comment:
    Ensure that the file patterns for glob.hashFiles are correctly specified and efficient, especially for large file sets.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_tn84Urt1F4MmR9Dg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on a347c1e in 1 minute and 35 seconds

More details
  • Looked at 942 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. __tests__/restore-cache.test.js:34
  • Draft comment:
    The '@actions/core' module is mocked twice in this file (lines 1 and 34). This is redundant and can lead to unexpected behavior. Consider consolidating these into a single mock definition.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. post.js:13
  • Draft comment:
    Using process.exit(0) can prevent pending asynchronous operations from completing. Consider allowing the Node.js process to exit naturally after cleanup.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    Consider clearing the timeout if the operation completes successfully before the 15-second timeout to prevent unnecessary aborts.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
4. stickydisk.js:106
  • Draft comment:
    The createStickyDiskClient() function is asynchronous but is not awaited. Consider awaiting this function to ensure the client is ready before proceeding.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment assumes createStickyDiskClient() is asynchronous, but there is no evidence in the provided code that it is. The function is used synchronously, suggesting it might not be asynchronous. Without further context or evidence, the comment seems speculative.
    I might be missing the implementation details of createStickyDiskClient(), which could indeed be asynchronous. However, based on the current file, there is no indication that it is asynchronous.
    Given the lack of evidence in the current file, it's reasonable to assume the function is not asynchronous unless proven otherwise. The comment should be removed unless further evidence is provided.
    Remove the comment as there is no strong evidence that createStickyDiskClient() is asynchronous and needs to be awaited.

Workflow ID: wflow_HgLDKsBZh0l88nEN


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.

try {
// Check if path is mounted
try {
const { stdout: mountOutput } = await execAsync(`mount | grep ${path}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path variable is used in shell commands without proper sanitization. Ensure that the path is controlled or sanitized to prevent command injection vulnerabilities.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 0a78627 in 1 minute and 10 seconds

More details
  • Looked at 944 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. util.js:18
  • Draft comment:
    lstatSync is used but not imported or defined in this file. Ensure it is imported or defined to avoid runtime errors.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. post.js:13
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process immediately, potentially skipping pending asynchronous operations. Let the process exit naturally unless necessary.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    Ensure to clear the timeout if the operation completes before the 15-second mark to prevent unexpected behavior.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. index.js:151
  • Draft comment:
    Using setTimeout with an async function can lead to unexpected behavior. Consider using a different approach to handle delays with async functions.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_5KCKj2xP9wsTtj5W


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.

stickydisk.js Outdated
}

try {
const client = await createStickyDiskClient();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createStickyDiskClient is not an async function, so await is unnecessary here. Remove await to avoid confusion.

break;
} catch (error) {
if (attempt === 10) {
throw error;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider logging the error if unmount fails after 10 attempts to aid in debugging.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on f1342f9 in 46 seconds

More details
  • Looked at 943 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. index.js:145
  • Draft comment:
    Consider removing the restoreCache function if it is no longer used or needed, to avoid confusion and potential misuse.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. post.js:2
  • Draft comment:
    Remove the unused import of config since it is no longer used in this file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The post.js file has removed the saveCache function but did not remove the import of config. This import is now unused and should be removed to clean up the code.
3. stickydisk.js:83
  • Draft comment:
    Consider using AbortSignal.timeout(15000) instead of setTimeout for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The stickydisk.js file uses setTimeout for aborting the controller, but it should use AbortSignal.timeout for better readability and maintainability.
4. stickydisk.js:21
  • Draft comment:
    Consider extracting environment variables into constants for better readability and maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The getStickyDisk function in stickydisk.js uses process.env directly multiple times. It would be better to extract these into variables for better readability and maintainability.
5. util.js:18
  • Draft comment:
    Ensure that lstatSync handles cases where the path might not exist to prevent unhandled exceptions.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The getFolderSize function in util.js uses lstatSync but does not handle the case where the path might not exist. This could lead to unhandled exceptions.

Workflow ID: wflow_xqm5MqPGHjqu5lmN


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 2bb489a in 48 seconds

More details
  • Looked at 1046 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. post.js:13
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. index.js:170
  • Draft comment:
    Avoid using setTimeout with an async function as the callback. This can lead to unhandled promise rejections. Consider refactoring to use a different approach for delaying execution.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    Ensure to clear the timeout if the operation completes before the timeout duration to prevent unnecessary aborts.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. stickydisk.js:131
  • Draft comment:
    Remove the unnecessary await before createStickyDiskClient() as it is not an async function.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_KLRNlkGgwhSAMTGF


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.

stickydisk.js Outdated Show resolved Hide resolved
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 84e4b43 in 47 seconds

More details
  • Looked at 1047 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. post.js:12
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending asynchronous operations. Let the process exit naturally instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. index.js:11
  • Draft comment:
    The mutex implementation lacks error handling, which could lead to deadlocks if an error occurs between lock and unlock. Consider adding error handling to ensure unlock is always called.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. stickydisk.js:164
  • Draft comment:
    Consider adding a mechanism to log or alert if unmounting fails after all retries, as the current implementation retries up to 10 times but doesn't handle consistent failures.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In stickydisk.js, the unmountAndCommitStickyDisk function retries unmounting up to 10 times with a delay. However, it doesn't handle the case where the unmounting consistently fails. Consider adding a mechanism to log or alert if unmounting fails after all retries.
4. index.js:173
  • Draft comment:
    Ensure that the files being hashed with glob.hashFiles are consistent and relevant to avoid unnecessary cache misses.
  • Reason this comment was not posted:
    Confidence changes required: 40%
    In index.js, the loadStickyDisk function uses glob.hashFiles to generate a hash for cache keys. Ensure that the files being hashed are consistent and relevant to avoid unnecessary cache misses.

Workflow ID: wflow_GWSbVaRF95fXsCsD


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.

* @param {string} device - Path to the block device
* @returns {Promise<string>} - Returns the device path
*/
async function maybeFormatBlockDevice(device) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of sudo in maybeFormatBlockDevice requires elevated privileges, which might not be available in all environments. Consider handling permission errors gracefully.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 53abca7 in 40 seconds

More details
  • Looked at 1026 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. index.js:168
  • Draft comment:
    The setTimeout function is used incorrectly here. It should not be awaited as it doesn't return a promise. Consider using a separate delay function for the timeout.
  await new Promise(resolve => setTimeout(resolve, delay));
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. post.js:12
  • Draft comment:
    Avoid using process.exit(0) as it can lead to abrupt termination of the process, potentially skipping important cleanup tasks. Let the process exit naturally.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    The setTimeout function is used incorrectly here. It should not be used with clearTimeout in this context. Consider using a promise-based delay function for better control.
await new Promise((resolve, reject) => {
  const timeoutId = setTimeout(() => reject(new Error('Timeout')), 15000);
  // Your async operation here
  clearTimeout(timeoutId);
  resolve();
});
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_5Fl3lIl62w3bVzZ6


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 5324ee1 in 46 seconds

More details
  • Looked at 1026 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. index.js:168
  • Draft comment:
    setTimeout should be used with a callback function, not as a promise. This could lead to unexpected behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment seems to misunderstand the use of setTimeout from 'timers/promises', which is intended to be used as a promise. The code appears to be using it correctly according to the Node.js documentation for 'timers/promises'.
    I might be missing some context about how setTimeout is expected to be used in this specific codebase, but based on standard Node.js usage, the code seems correct.
    The use of setTimeout from 'timers/promises' is standard and correct for promise-based asynchronous operations, so the comment is likely incorrect.
    The comment should be deleted because it incorrectly suggests a misuse of setTimeout, which is actually being used correctly as a promise.
2. post.js:12
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process before asynchronous operations complete. Let the process exit naturally.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. stickydisk.js:83
  • Draft comment:
    Ensure clearTimeout(timeoutId) is called in all code paths to prevent unexpected behavior if getStickyDisk fails.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. stickydisk.js:152
  • Draft comment:
    Using grep to check if a path is mounted is not portable. Consider using a more cross-platform method to check mount status.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment points out a potential issue with portability, which is a valid concern in cross-platform applications. However, the context of the application (e.g., if it's intended to run only on Unix-like systems) is not clear. Without strong evidence that the code is intended to be cross-platform, the comment might not be necessary.
    The comment assumes the code should be cross-platform without evidence. If the code is intended for Unix-like systems, the comment is not useful.
    The comment could be useful if the code is intended to be cross-platform, but without evidence of this requirement, it may not be necessary.
    The comment should be removed unless there is strong evidence that the code is intended to be cross-platform, which is not provided in the diff.

Workflow ID: wflow_ns0GrzD8bcEDNpAB


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.

index.js Outdated Show resolved Hide resolved
This change teaches restoreCache to get and mount a unique stickydisk
per key and mount it at the provided path. We require a stickydisk per
path and so we hash the path and add it to the user configured "cache key".
All other semantics around versioning of the cache and respecting the base
cache key remain the same.

On post, we teach the action to unmount and commit the relevant stickydisks.
For the moment we leave the "external" cache alone. In a follow-up we can move
that to be backed by stickydisks too.

The stickydisk.js file is heavily inspired by the useblacksmith/stickydisk
implementation.
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on ecb4f37 in 51 seconds

More details
  • Looked at 1038 lines of code in 9 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. post.js:12
  • Draft comment:
    Avoid using process.exit(0) as it can terminate the process abruptly, potentially skipping pending asynchronous operations. Let the process exit naturally after all tasks are completed.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. stickydisk.js:46
  • Draft comment:
    Ensure the environment has sufficient privileges to execute commands with sudo, as this can cause issues if not run with the necessary permissions.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. stickydisk.js:173
  • Draft comment:
    Consider logging error details on each retry attempt to aid in debugging transient unmount issues.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In stickydisk.js, the unmountAndCommitStickyDisk function retries unmounting up to 10 times with a delay. This is a good practice for handling transient issues, but it might be beneficial to log the error details on each retry to aid in debugging.
4. index.js:168
  • Draft comment:
    Consider implementing a backoff strategy instead of a random delay to handle contention more effectively.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In index.js, the loadStickyDisk function uses setTimeout with a random delay to reduce contention. While this can help, it might be more effective to implement a backoff strategy if contention is a significant issue.
5. index.js:179
  • Draft comment:
    Ensure the timeout set for AbortController is appropriate for the operations being performed to avoid premature abortion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In index.js, the loadStickyDisk function uses AbortController for timeout management. Ensure that the timeout is appropriate for the operations being performed, as too short a timeout might lead to premature abortion of valid operations.

Workflow ID: wflow_SOKfGKHk0DH7mJpR


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

ellipsis-dev bot commented Dec 15, 2024

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

@adityamaru adityamaru merged commit 81e923a into main Dec 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants