Skip to content

Conversation

@hanpham32
Copy link

@hanpham32 hanpham32 commented Mar 8, 2025

This PR introduces the following changes:

  • Remove the duplicate curl before Gaia cli installation
  • Improve user console logging readability
  • Add timestamp

I have tested the modified script.sh in a docker environment

Result:
image

Copy link
Contributor

juntao commented Mar 8, 2025

Hello, I am a code review agent on flows.network. Here are my reviews of changed source code files in this PR.


install.sh

Potential issues

  1. Incorrect backup_to_file Path Handling:

    • The line backup_to_file=$(cd "$backup_to_file" && pwd) is incorrect because backup_to_file should be a file path, not a directory. This command attempts to change into the backup file location as if it were a directory, which will fail.
  2. Unnecessary Directory Permissions:

    • The script creates directories with mode 777 (mkdir -p -m777 $gaianet_base_dir), which is overly permissive and poses security risks. Directories should have more restrictive permissions, such as 750.
  3. Redundant Architecture Check:

    • The script checks for the architecture twice (once for Linux and once for macOS). This redundancy can be consolidated into a single check to improve readability and maintainability.

Summary of changes

-• Improved Logging: Introduced timestamped logging with color-coding for INFO, WARNING, ERROR, SUCCESS, and WAITING messages.
Removed Duplicate curl Commands: Consolidated duplicate curl function calls into a single check_curl function.
Reordered Variable Definitions: Moved ANSI escape code definitions below the argument parsing section for better organization.

@hanpham32 hanpham32 changed the title fix(install.sh): improve script logging & remove duplicate curl feat(install.sh): improve script logging & remove duplicate curl Mar 8, 2025
@hanpham32
Copy link
Author

I can also update the gaianet.sh script to match the style of this install.sh logging if needed :^)

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