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

Optimize find usage in script to reduce unnecessary commands #201

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2024

Optimize find usage in script to reduce unnecessary commands

  • Replaced grep and cut with more efficient find options
  • Used basename in find to directly extract filenames
  • Simplified tr usage to convert newlines into spaces

Summary by Sourcery

Optimize the build.sh script by enhancing the find command usage to reduce unnecessary commands and improve efficiency. Add error handling for ZFS pool creation and implement a timeout for exporting the ZFS pool to ensure reliability.

Bug Fixes:

  • Add error handling for ZFS pool creation to exit the script if the pool creation fails.
  • Implement a timeout mechanism for exporting the ZFS pool to ensure it completes within a specified time frame.

Enhancements:

  • Optimize the use of the find command in build.sh by replacing grep and cut with more efficient find options and using basename to directly extract filenames.
  • Simplify the tr command usage to convert newlines into spaces in the script.

Optimize find usage in script to reduce unnecessary commands

- Replaced grep and cut with more efficient find options
- Used basename in find to directly extract filenames
- Simplified tr usage to convert newlines into spaces
@ghost ghost requested review from a team as code owners September 9, 2024 21:05
Copy link
Contributor

sourcery-ai bot commented Sep 9, 2024

Reviewer's Guide by Sourcery

This pull request optimizes the build script by improving the efficiency of file searching and ZFS operations. The changes focus on reducing unnecessary commands and enhancing error handling.

File-Level Changes

Change Details Files
Optimized file search and filename extraction
  • Replaced grep and cut with more efficient find options
  • Used basename in find to directly extract filenames
  • Simplified tr usage to convert newlines into spaces
build.sh
Improved ZFS pool creation and management
  • Combined zpool create command with ZFS property settings
  • Added error checking for ZFS pool creation
  • Modified zfs send command to include properties (-p flag)
  • Implemented a timeout mechanism for ZFS pool export
build.sh
Enhanced error handling and script robustness
  • Added error checking for ZFS pool creation
  • Implemented a timeout mechanism for ZFS pool export
build.sh

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • Continue your discussion with Sourcery by replying directly to review comments.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @vimanuelt - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Good optimization. Consider adding a brief comment explaining the find command for clarity.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Optimize ZFS commands and improve error handling in build script

- Optimized `zpool create` by using `-O` options to set mountpoint and compression during pool creation, reducing redundant `zfs set` commands.

- Added error handling after `zpool create` to ensure the script exits if pool creation fails, preventing further steps from executing on a failed pool.

- Improved `zfs send` command by using the `-p` flag to preserve properties, optimizing performance for future operations.

- Enhanced the ZFS pool export check in the `boot` function by adding a timeout mechanism to avoid potential infinite loops, providing a clearer failure path.

- Overall improvements to script reliability and performance in managing ZFS operations.
@ericbsd
Copy link
Member

ericbsd commented Sep 9, 2024

@sourcery-ai review

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @vimanuelt - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding explanatory comments for the complex find command in the desktop_list assignment to improve readability.
  • In the boot() function, consider adding a force export or other explicit handling if the ZFS pool export fails after the timeout.
Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Copy link
Member

@ericbsd ericbsd left a comment

Choose a reason for hiding this comment

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

LGTM

@ericbsd ericbsd merged commit a75b07f into ghostbsd:master Sep 9, 2024
1 check passed
@ericbsd ericbsd assigned ghost Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant