Add devbox support#67
Conversation
Also done small refactoring for zsh config
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| .PHONY: git | ||
|
|
||
| all: system git asdf terminal devops neovim ## Install and configure everything (default) | ||
| all: system nix devbox terminal #devops neovim ## Install and configure everything (default) |
There was a problem hiding this comment.
The all target depends on several other targets, including devbox and terminal. If any of these targets fail, the all target will also fail, but the error handling is not explicit. This can make it difficult to diagnose which part of the process failed.
Recommendation: Add explicit error handling or logging to each script being called to make it easier to diagnose failures. Alternatively, consider using a tool that provides better error reporting.
| @./scripts/git.sh configure | ||
|
|
||
| terminal: zsh ohmyzsh bat-configure lsd fzf delta-configure ripgrep shellcheck lazygit win32yank navi ## Setup the terminal | ||
| terminal: zsh ohmyzsh git starship-configure bat-configure delta-configure #lsd fzf ripgrep shellcheck lazygit win32yank navi ## Setup the terminal |
There was a problem hiding this comment.
The terminal target has a long list of dependencies, which can make it difficult to maintain and extend. If any of these dependencies are not required in all environments, it could lead to unnecessary complexity and longer execution times.
Recommendation: Break down the terminal target into smaller, more modular targets. This will improve maintainability and allow for more granular control over what gets executed.
| fi | ||
|
|
||
| info "[devbox] Install" | ||
| bash -c "$(curl -fsSL https://get.jetify.com/devbox)" "" --force |
There was a problem hiding this comment.
The script downloads and executes a remote script via curl and bash -c. This can be a significant security risk as it allows for the execution of potentially malicious code if the remote source is compromised.
Recommendation: Verify the integrity of the downloaded script before executing it. This can be done by checking the script's checksum or using a more secure method to install the software.
| fi | ||
|
|
||
| info "[nix] Install" | ||
| sh <(curl -L https://nixos.org/nix/install) --daemon --yes |
There was a problem hiding this comment.
The script downloads and executes a remote script directly using curl and sh. This is a significant security risk as the remote script could be modified by a malicious actor, leading to potential system compromise.
Recommendation: Download the script first, verify its integrity (e.g., using a checksum), and then execute it. This reduces the risk of executing malicious code.
| @@ -37,7 +36,6 @@ do_configure() { | |||
| done | |||
There was a problem hiding this comment.
The loop that iterates over the plugins array does not perform any actions. This is likely an oversight and could lead to confusion or errors in the future.
Recommended Solution:
Ensure that the loop performs the intended actions, or remove it if it is not needed.
| set -e | ||
|
|
||
| # shellcheck source=../scripts/util.sh | ||
| source "$(pwd)/scripts/util.sh" |
There was a problem hiding this comment.
The source "$(pwd)/scripts/util.sh" command assumes that the script is always run from the same directory where it is located. This can lead to issues if the script is executed from a different directory. A more robust approach would be to use the script's directory to construct the path to util.sh.
| do_configure "$@" | ||
| ;; | ||
| *) | ||
| error "$(basename "$0"): '$command' is not a valid command" |
There was a problem hiding this comment.
The error function is called when an invalid command is provided, but there is no definition or sourcing of an error function in the provided code. This will result in a command not found error. Ensure that the error function is defined or sourced from another script.
| export DEBIAN_FRONTEND=noninteractive | ||
| sudo apt-add-repository -y ppa:git-core/ppa | ||
| sudo apt-get update -qq | ||
| sudo apt-get install -qq -y "${packages[@]}" |
There was a problem hiding this comment.
Setting DEBIAN_FRONTEND=noninteractive can suppress important prompts and error messages from apt-get, which might lead to silent failures or missed configurations. It's crucial to ensure that this behavior is intended and that any potential issues are logged or handled appropriately. Consider adding error handling or logging to capture any issues that might arise during the package installation process.
|
|
||
| do_configure() { | ||
| info "[zsh] Install" | ||
| sudo apt-get install -qq -y zsh |
There was a problem hiding this comment.
The use of sudo apt-get install -qq -y zsh assumes that the script is being run with sufficient privileges and on a Debian-based system. This can lead to issues if the script is run on a different distribution or without the necessary permissions. Consider adding checks to verify the operating system and user privileges before attempting to install the package. Additionally, provide a fallback or informative message if the conditions are not met.
|
|
||
| # Apply sensisble zsh settings | ||
| source ~/dotfiles/zsh/zshopt | ||
| source $HOME/dotfiles/zsh/zshopt |
There was a problem hiding this comment.
The source $HOME/dotfiles/zsh/zshopt command does not check if the file exists before sourcing it. This could lead to errors if the file is missing.
Recommended Solution: Add a check to ensure the file exists before sourcing it, similar to the check used for extras on line 78.
| fi | ||
|
|
||
| info "[devbox] Install" | ||
| bash -c "$(curl -fsSL https://get.jetify.com/devbox)" "" --force |
There was a problem hiding this comment.
The script downloads and executes a remote script via curl and bash -c. This can be a significant security risk as it allows for the execution of potentially malicious code if the remote source is compromised.
Recommendation: Verify the integrity of the downloaded script before executing it. This can be done by checking the script's checksum or using a more secure method to install the software.
| fi | ||
|
|
||
| info "[devbox] Install" | ||
| bash -c "$(curl -fsSL https://get.jetify.com/devbox)" "" --force |
There was a problem hiding this comment.
The script downloads and executes a remote script via curl and bash -c. This can be a significant security risk as it allows for the execution of potentially malicious code if the remote source is compromised.
Recommendation: Verify the integrity of the downloaded script before executing it. This can be done by checking the script's checksum or using a more secure method to install the software.
| export DEBIAN_FRONTEND=noninteractive | ||
| sudo apt-add-repository -y ppa:git-core/ppa | ||
| sudo apt-get update -qq | ||
| sudo apt-get install -qq -y "${packages[@]}" |
There was a problem hiding this comment.
Setting DEBIAN_FRONTEND=noninteractive can suppress important prompts and error messages from apt-get, which might lead to silent failures or missed configurations. It's crucial to ensure that this behavior is intended and that any potential issues are logged or handled appropriately. Consider adding error handling or logging to capture any issues that might arise during the package installation process.
|
|
||
| bash | ||
| info "[devbox] Install" | ||
| bash -c "$(curl -fsSL https://get.jetify.com/devbox)" "" --force |
There was a problem hiding this comment.
The script downloads and executes a remote script via curl and bash -c. This can be a significant security risk as it allows for the execution of potentially malicious code if the remote source is compromised.
Recommendation: Verify the integrity of the downloaded script before executing it. This can be done by checking the script's checksum or using a more secure method to install the software.
| @./scripts/git.sh configure | ||
|
|
||
| terminal: zsh ohmyzsh bat-configure lsd fzf delta-configure ripgrep shellcheck lazygit win32yank navi ## Setup the terminal | ||
| terminal: zsh ohmyzsh git starship-configure bat-configure delta-configure direnv-configure #lsd fzf ripgrep shellcheck lazygit win32yank navi ## Setup the terminal |
There was a problem hiding this comment.
The terminal target has a long list of dependencies, which can make it difficult to maintain and extend. If any of these dependencies are not required in all environments, it could lead to unnecessary complexity and longer execution times.
Recommendation: Break down the terminal target into smaller, more modular targets. This will improve maintainability and allow for more granular control over what gets executed.
| set -e | ||
|
|
||
| # shellcheck source=../scripts/util.sh | ||
| source "$(pwd)/scripts/util.sh" |
There was a problem hiding this comment.
Sourcing a script using an absolute path constructed with pwd can lead to issues if the script is run from a different directory. This can make the script less portable and harder to use in different environments.
Recommended Solution:
Use a relative path or determine the script's directory dynamically using ${BASH_SOURCE[0]} to make the script more robust and portable.
| do_configure "$@" | ||
| ;; | ||
| *) | ||
| error "$(basename "$0"): '$command' is not a valid command" |
There was a problem hiding this comment.
The error function is used to handle invalid commands, but it is not defined within this script. This will lead to a runtime error if an invalid command is passed.
Recommended Solution:
Ensure that the error function is defined in the sourced util.sh script or define it within this script to handle errors gracefully.
CI Failure Feedback 🧐(Checks updated until commit bde9301)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: where Configuration options
See more information about the |
| ln -fs "$(pwd)/devbox/devbox.json" "$DEVBOX_GLOBAL_DIR/devbox.json" | ||
| # echo "eval '$(devbox global shellenv)'" >> ~/.zshrc | ||
| # source ~/.zshrc | ||
| # temporarily add the global packages to the current shell (this is already configured in oh-my-zsh) | ||
| source <(devbox global shellenv --init-hook) |
There was a problem hiding this comment.
The script assumes the existence and correct setup of certain files and directories (e.g., devbox.json and ~/.zshrc). This can lead to failures in environments where these are not present or have restricted permissions.
Recommendation: Add checks to ensure that necessary files and directories exist before attempting to use them. Provide clear error messages if the expected conditions are not met. This will enhance the robustness and user-friendliness of the script.
| do_configure "$@" | ||
| ;; | ||
| *) | ||
| error "$(basename "$0"): '$command' is not a valid command" |
There was a problem hiding this comment.
The error handling in the main function does not provide guidance on valid commands if an invalid command is entered. This could lead to user confusion.
Recommendation: Enhance the error message in line 45 to include a list of valid commands. This will improve the user experience by providing immediate guidance on how to correct the input.
| @@ -6,7 +6,12 @@ set -e | |||
| source "$(pwd)/scripts/util.sh" | |||
There was a problem hiding this comment.
The use of source "$(pwd)/scripts/util.sh" introduces a security risk by dynamically constructing the path to the sourced script based on the current working directory. This could lead to sourcing an unintended script if the working directory is not as expected. To mitigate this risk, consider using a fixed path relative to the script itself or validating the path before sourcing.
Recommended Solution:
Use a more reliable method to determine the script's directory, such as:
DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
source "$DIR/util.sh"| # TODO add check to create empty config only if config is missing | ||
| echo "# Empty config created by dotfiles (will be replaced by oh-my-zsh)" >> ~/.zshrc |
There was a problem hiding this comment.
The script directly appends an empty configuration to ~/.zshrc without checking if the file already exists or if the configuration is present. This could lead to multiple redundant entries or unintended overwriting of user configurations.
Recommended Solution:
Implement the check as indicated by the TODO comment to ensure that the configuration is only added if it does not exist. This can be achieved using a simple conditional check:
if [ ! -f ~/.zshrc ]; then
echo "# Empty config created by dotfiles (will be replaced by oh-my-zsh)" >> ~/.zshrc
fi1 participant
No description provided.