Closed fedesimeon closed 3 weeks ago
โฑ๏ธ Estimated effort to review: 2 ๐ต๐ตโชโชโช |
๐งช No relevant tests |
๐ No security concerns identified |
โก Key issues to review Version Check The macOS version check has been updated to include Sequoia (15.x), but the regex pattern might be too permissive. It allows any minor version number, which could potentially include future, unsupported versions. Force Installation The 'force' option has been added to the Homebrew Cask font installation. This might overwrite existing fonts without warning, potentially causing unexpected behavior in applications that rely on specific font versions. |
Category | Suggestion | Score |
Maintainability |
Use a more flexible version checking approach to simplify future updates___ **Consider using a more flexible version checking approach. Instead of hardcodingversion numbers, you could use a comparison operator to check if the macOS version is greater than or equal to a minimum supported version.** [bin/common/utils.sh [8-9]](https://github.com/sparkfabrik/sparkdock/pull/90/files#diff-0e519661e53f442cf4e168f2abea58548b5f1c9358f55fc54b6fc77363c56a33R8-R9) ```diff -if ! [[ $( sw_vers -productVersion ) =~ ^(15.[0-9]+|14.[0-9]+|13.[0-9]+|12.[0-9]+|11.[0-9]+) ]] ; then - print "${RED}Sorry, this script is supposed to be executed on macOS Big Sur (11.x), Monterey (12.x), Ventura (13.x), Sonoma (14.x) and Sequoia (15.x). Please use a supported version.${NC}" +if ! [[ "$( sw_vers -productVersion )" =~ ^([0-9]+) ]] || [ "${BASH_REMATCH[1]}" -lt 11 ]; then + print "${RED}Sorry, this script is supposed to be executed on macOS Big Sur (11.x) or later. Please use a supported version.${NC}" ``` Suggestion importance[1-10]: 8Why: The suggestion provides a more maintainable solution that will require less frequent updates as new macOS versions are released. | 8 |
Consistency |
Implement a consistent and flexible version checking approach across files___ **Similar to the suggestion for 'bin/common/utils.sh', consider using a more flexibleversion checking approach here as well for consistency across the codebase.** [bin/install.macos [8-9]](https://github.com/sparkfabrik/sparkdock/pull/90/files#diff-3155e8fd815072a66aada172156d432cfb8ca8ec96f2b7fd939a3d3a422afabcR8-R9) ```diff -if ! [[ $( sw_vers -productVersion ) =~ ^(15.[0-9]+|14.[0-9]+|13.[0-9]+|12.[0-9]+|11.[0-9]+) ]] ; then - print "${RED}Sorry, this script is supposed to be executed on macOS Big Sur (11.x), Monterey (12.x), Ventura (13.x), Sonoma (14.x) and Sequoia (15.x). Please use a supported version.${NC}" +if ! [[ "$( sw_vers -productVersion )" =~ ^([0-9]+) ]] || [ "${BASH_REMATCH[1]}" -lt 11 ]; then + print "${RED}Sorry, this script is supposed to be executed on macOS Big Sur (11.x) or later. Please use a supported version.${NC}" ``` Suggestion importance[1-10]: 8Why: This suggestion promotes consistency across the codebase and offers the same benefits of maintainability as the first suggestion. | 8 |
Best practice |
Replace force installation with a more controlled approach using ignore_errors___ **Theinstall_options: force might lead to unexpected behavior. Consider using state: present with ignore_errors: yes instead, which allows the task to continue even if some fonts fail to install.** [ansible/macos/macos/base.yml [107-113]](https://github.com/sparkfabrik/sparkdock/pull/90/files#diff-35e06808caf91480daa0d30bc6b6a1e4c425376bb5d3196d021dca2357bdd621R107-R113) ```diff community.general.homebrew_cask: name: - font-droid-sans-mono-nerd-font - font-inconsolata-nerd-font - font-fira-code-nerd-font state: present - install_options: force +ignore_errors: yes ``` Suggestion importance[1-10]: 7Why: The suggestion offers a safer approach to font installation, allowing the script to continue even if some fonts fail to install, which is generally better than forcing installation. | 7 |
PR Type
Enhancement
Description
Changes walkthrough ๐
utils.sh
Add support for macOS Sequoia in version check
bin/common/utils.sh
checkMacosVersion
function to include macOS Sequoia (15.x)versions
base.yml
Force option added to font installation
ansible/macos/macos/base.yml - Added 'force' install option for Homebrew Cask font installation
install.macos
Add support for macOS Sequoia in installation script
bin/install.macos
checkMacosVersion
function to include macOS Sequoia (15.x)versions