Skip to content
This repository was archived by the owner on Sep 26, 2025. It is now read-only.

Conversation

@dbarrosop
Copy link
Member

@dbarrosop dbarrosop commented Sep 25, 2025

PR Type

Enhancement


Description

  • Update upgrade command to parse cli@ tags

  • Introduce filterReleases to exclude old/prereleases

  • Switch GitHub API paths to use nhost/nhost repo

  • Enhance get.sh for version retrieval and URL encoding


File Walkthrough

Relevant files
Enhancement
upgrade.go
Version parsing in upgrade command                                             

cmd/software/upgrade.go

  • Imported strings package
  • Split latest.TagName on @ to extract version
  • Constructed download filename with parsed version
+9/-1     
sofware.go
Release filtering and API path update                                       

software/sofware.go

  • Added filterReleases method for version filtering
  • Updated GetReleases to call filterReleases
  • Switched API URL to https://api.github.com/repos/nhost/nhost/releases
  • Imported strings package
+35/-10 
get.sh
Install script repo and URL encoding update                           

get.sh

  • Changed REPO to nhost/nhost
  • Retrieved latest cli@ tag from GitHub releases
  • Encoded @ as %40 in download URL
+11/-9   

@github-actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Variable reference bug

In the else branch assigning release, it uses an uninitialized $release variable instead of the user-provided $version, leading to incorrect download URLs.

release="cli@$release"
Incorrect grep quoting

The grep \"cli\@ pattern is not properly quoted and may fail to match the cli@ tag in the JSON output. It should be wrapped or escaped correctly (e.g., grep '"cli@"').

release=$(curl --silent https://api.github.com/repos/nhost/nhost/releases\?per_page=100 | grep tag_name | grep \"cli\@ | head -n 1 | sed 's/.*"tag_name": "\([^"]*\)".*/\1/')
Use SplitN for tag parsing

Using strings.Split(latest.TagName, "@") can produce extra fields if the tag contains multiple @. Consider using strings.SplitN(latest.TagName, "@", 2) for predictability.

s := strings.Split(latest.TagName, "@")

@github-actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix undefined release variable

In the else branch, you reference $release before it's defined; use the version
variable to construct the release, e.g. release="cli@${version}".

get.sh [47]

-release="cli@$release"
+release="cli@${version}"
Suggestion importance[1-10]: 8

__

Why: Referencing $release before it’s defined causes a bug; using ${version} ensures the correct value is assigned.

Medium
Properly test empty version

Use -z to test for an empty string and quote $version to avoid word splitting, e.g.
if [ -z "$version" ].

get.sh [51]

-if [ ! $version ]; then
+if [ -z "$version" ]; then
Suggestion importance[1-10]: 6

__

Why: Using [ -z "$version" ] correctly checks for an empty string and prevents word-splitting issues in shell tests.

Low
General
Limit split to first delimiter

Use strings.SplitN with a limit of 2 to ensure you only split on the first @,
preventing unintended extra splits.

cmd/software/upgrade.go [47]

-s := strings.Split(latest.TagName, "@")
+s := strings.SplitN(latest.TagName, "@", 2)
Suggestion importance[1-10]: 6

__

Why: strings.SplitN with a limit avoids unintended splits if TagName contains multiple @, improving robustness.

Low

@dbarrosop dbarrosop merged commit 9325830 into main Sep 26, 2025
8 checks passed
@dbarrosop dbarrosop deleted the deprecate-repo branch September 26, 2025 06:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants