10000 Bash script changes by george-cindea · Pull Request #4 · lfaoro/ssm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Bash script changes #4

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

Merged
merged 3 commits into from
Apr 26, 2025
Merged

Conversation

george-cindea
Copy link
Contributor

In this PR:

  • change simple if statements to inline for ease of read
  • removed -n flag in test, it is the default behavior
  • declare local variables in separate statement
  • change variable name from dir to directory (dir is a linux command, possibly not in all distros)
  • remove usage of $? - this is finicky and prone to error
  • LATEST_RELEASE_URL var is not used anywhere so it was commented
  • remove usage of pipe and change to here-strings
  • introduce regex test instead of comparison

@lfaoro
Copy link
Owner
lfaoro commented Apr 25, 2025

thank you @george-cindea for the script improvements, I left some code notes, resolve them and it will be merged -- truly appreciate your support!

@lfaoro lfaoro added enhancement New feature or request good first issue Good for newcomers labels Apr 25, 2025
@lfaoro lfaoro changed the base branch from main to ci April 25, 2025 16:01
Repository owner deleted a comment from allcontributors bot Apr 25, 2025
Repository owner deleted a comment from allcontributors bot Apr 25, 2025
@lfaoro lfaoro deleted the branch lfaoro:main April 25, 2025 16:08
@lfaoro lfaoro closed this Apr 25, 2025
@lfaoro lfaoro reopened this Apr 25, 2025
@lfaoro lfaoro changed the base branch from ci to main April 25, 2025 16:36
@lfaoro
Copy link
Owner
lfaoro commented Apr 25, 2025

@all-contributors please add @george-cindea for code

Copy link

@lfaoro

We had trouble processing your request. Please try again later.

Repository owner deleted a comment from allcontributors bot Apr 25, 2025
Repository owner deleted a comment from allcontributors bot Apr 25, 2025
scripts/get.sh Outdated
rm -f "$temp_check"
return 1
fi
rm -f "$dir/$(basename "$temp_check")"
rm -f "$directory/$(basename "$temp_check")"
Copy link
Owner

Choose a reason for hiding this comment

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

dir is short and clear, don't see the need to use directory

Copy link
Owner

Choose a reason for hiding this comment

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

test

Copy link
Owner

Choose a reason for hiding this comment

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

use path instead of dir

scripts/get.sh Outdated
esac
fi
}

# Configuration
APP_NAME=ssm
REPO="lfaoro/ssm"
LATEST_RELEASE_URL="https://github.com/${REPO}/releases/latest"
#Not used anywhere, if used externally, export it
Copy link
Owner

Choose a reason for hiding this comment

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

if unused, remove it

scripts/get.sh Outdated
error "download URL not accessible: ${DOWNLOAD_ARCHIVE_URL}"
fi

# Create temporary directory for extraction
TEMP_DIR=$(mktemp -d) || error "failed to create temporary directory"

# Download and extract the archive
if [ "${OS}" = "windows" ]; then
if [[ "${OS}" =~ "windows" ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

you can remove windows support, as win users use ssm via WSL2
we don't release ssm.exe since 0.2.1

@lfaoro
Copy link
Owner
lfaoro commented Apr 26, 2025

test again

@george-cindea
Copy link
Contributor Author

All done. don't forget to squash commits at/before merge :)

@lfaoro lfaoro merged commit 5b308ab into lfaoro:main Apr 26, 2025
@lfaoro
Copy link
Owner
lfaoro commented Apr 26, 2025

too late. i merged as is :))

@lfaoro
Copy link
Owner
lfaoro commented Apr 26, 2025

@all-contributors please add @george-cindea for code

Copy link

@lfaoro

We had trouble processing your request. Please try again later.

@george-cindea george-cindea deleted the dev/clean_up_sh_scripts branch April 27, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0