feat: add 'downloads add-url' command for custom tarball URLs (#59)#61
feat: add 'downloads add-url' command for custom tarball URLs (#59)#61renecannao merged 1 commit intomasterfrom
Conversation
Implements GitHub issue #59. Adds a new 'downloads add-url <URL>' subcommand that parses the tarball filename to auto-detect version, OS, arch, flavor, and minimal flag, validates URL accessibility via HTTP HEAD/GET, and persists the entry to the local tarball registry (~/.dbdeployer/tarball-list.json). - downloads/remote_registry.go: add ParseTarballUrlInfo, CheckRemoteUrl, and detectOSArchFromFilename helpers - cmd/downloads.go: add addTarballToCollectionFromUrl handler and downloadsAddUrlCmd cobra command with --OS, --arch, --flavor, --version, --short-version, --minimal, --overwrite, --skip-verify-url flags - globals/globals.go: add SkipVerifyUrlLabel constant - downloads/remote_registry_test.go: add TestParseTarballUrlInfo covering MySQL Linux/macOS, Percona minimal, and invalid filename cases
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as downloads add-url
participant Parser as ParseTarballUrlInfo
participant Validator as CheckRemoteUrl
participant Registry as Local Registry
User->>Cmd: downloads add-url <url> [flags]
Cmd->>Parser: ParseTarballUrlInfo(url)
Parser-->>Cmd: TarballDescription (auto-detected metadata)
Cmd->>Validator: CheckRemoteUrl(url)
Validator-->>Cmd: Content-Length, error (or fallback)
alt skip-verify-url not set
Cmd->>Validator: Validate URL accessibility
end
Cmd->>Registry: Merge entry (with overwrite check)
Registry-->>Cmd: Success or error (existing entry)
Cmd-->>User: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new command, add-url, which allows users to add remote tarballs to the local registry by providing a URL. The implementation includes auto-detection of tarball metadata from the filename, with support for manual overrides via flags, and performs a URL accessibility check. My review identified several areas for improvement: the CheckRemoteUrl function should use an HTTP client with a timeout and leverage resp.ContentLength directly to avoid potential issues; the ParseTarballUrlInfo function should be more lenient when auto-detection fails to allow for manual overrides; the OS detection logic needs to be more consistent with the project's naming conventions; and there is a discrepancy regarding the target registry file for custom tarballs that needs to be addressed to prevent potential data loss or conflicts.
| func CheckRemoteUrl(remoteUrl string) (int64, error) { | ||
| // Try HEAD first to avoid downloading the file | ||
| // #nosec G107 | ||
| resp, err := http.Head(remoteUrl) | ||
| if err == nil { | ||
| defer func(Body io.ReadCloser) { | ||
| _ = Body.Close() | ||
| }(resp.Body) | ||
| if resp.StatusCode == http.StatusOK { | ||
| var size int64 | ||
| for key := range resp.Header { | ||
| if strings.EqualFold(key, "Content-Length") && len(resp.Header[key]) > 0 { | ||
| size, _ = strconv.ParseInt(resp.Header[key][0], 10, 0) | ||
| } | ||
| } | ||
| return size, nil | ||
| } | ||
| } | ||
| // Fall back to GET if HEAD failed or returned non-200 | ||
| return checkRemoteUrl(remoteUrl) | ||
| } |
There was a problem hiding this comment.
The CheckRemoteUrl function uses the default HTTP client which has no timeout, potentially causing the CLI to hang on slow or unresponsive servers. Additionally, manually parsing the Content-Length header is unnecessary and error-prone (especially with bitSize set to 0 in ParseInt, which can overflow on 32-bit systems for files > 2GB). It is recommended to use a client with a timeout and leverage resp.ContentLength directly.
func CheckRemoteUrl(remoteUrl string) (int64, error) {
client := &http.Client{
Timeout: 10 * time.Second,
}
// Try HEAD first to avoid downloading the file
// #nosec G107
resp, err := client.Head(remoteUrl)
if err == nil {
defer func(Body io.ReadCloser) {
_ = Body.Close()
}(resp.Body)
if resp.StatusCode == http.StatusOK {
return resp.ContentLength, nil
}
}
// Fall back to GET if HEAD failed or returned non-200
return checkRemoteUrl(remoteUrl)
}| flavor, version, shortVersion, err := common.FindTarballInfo(fileName) | ||
| if err != nil { | ||
| return TarballDescription{}, fmt.Errorf("could not parse version from filename %q: %s", fileName, err) | ||
| } |
There was a problem hiding this comment.
Returning an error here when FindTarballInfo fails prevents the user from using the override flags (--version, --flavor, etc.) for URLs with non-standard filenames. Since the caller already validates the required fields after applying overrides, this function should be more lenient and allow the process to continue even if auto-detection fails.
| flavor, version, shortVersion, err := common.FindTarballInfo(fileName) | |
| if err != nil { | |
| return TarballDescription{}, fmt.Errorf("could not parse version from filename %q: %s", fileName, err) | |
| } | |
| flavor, version, shortVersion, _ := common.FindTarballInfo(fileName) |
| OS = runtime.GOOS | ||
| if OS == "darwin" { | ||
| OS = "Darwin" | ||
| } else if OS == "linux" { | ||
| OS = "Linux" | ||
| } | ||
| } |
There was a problem hiding this comment.
The default OS detection returns lowercase runtime.GOOS (e.g., "windows"), which is inconsistent with the explicit check for "Windows" (Title case) at line 635 and the general convention in this project for other operating systems like "Linux" and "Darwin".
default:
OS = runtime.GOOS
if strings.EqualFold(OS, "darwin") {
OS = "Darwin"
} else if strings.EqualFold(OS, "linux") {
OS = "Linux"
} else if strings.EqualFold(OS, "windows") {
OS = "Windows"
}| if err != nil { | ||
| return fmt.Errorf("error writing tarball list: %s", err) | ||
| } | ||
| fmt.Printf("Tarball below added to %s\n", downloads.TarballFileRegistry) |
There was a problem hiding this comment.
There is a discrepancy between the implementation and the PR description. The description states that custom tarballs are saved to ~/.dbdeployer/custom_tarballs.json, but the code uses downloads.TarballFileRegistry, which points to tarball-list.json. This means custom entries are mixed into the main registry file, which might lead to them being lost or causing conflicts if the main registry is updated or reset.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
downloads/remote_registry.go (1)
573-593: Consider adding a timeout to HTTP requests.The
http.Headand the fallbackhttp.Getuse the default HTTP client, which has no timeout. This can cause indefinite hangs when checking URLs for unresponsive servers, degrading user experience.♻️ Suggested fix: use a client with timeout
func CheckRemoteUrl(remoteUrl string) (int64, error) { // Try HEAD first to avoid downloading the file + client := &http.Client{Timeout: 30 * time.Second} // `#nosec` G107 - resp, err := http.Head(remoteUrl) + resp, err := client.Head(remoteUrl) if err == nil { defer func(Body io.ReadCloser) { _ = Body.Close() }(resp.Body)Apply the same timeout to the
checkRemoteUrlfunction for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downloads/remote_registry.go` around lines 573 - 593, The CheckRemoteUrl function (and its fallback checkRemoteUrl) currently use the default http client with no timeout; update both to use an http.Client with a sensible Timeout (e.g., a few seconds) and call client.Head / client.Get instead of http.Head / http.Get so requests cannot hang indefinitely; ensure you still close resp.Body in CheckRemoteUrl and mirror the same client/timeout usage in the checkRemoteUrl helper for consistency (reference functions: CheckRemoteUrl and checkRemoteUrl).cmd/downloads.go (1)
680-681: Pre-existing pattern: local registry file not loaded before modification.The code uses
downloads.DefaultTarballRegistry(the embedded registry) rather than first loading any existing~/.dbdeployer/tarball-list.json. This follows the same pattern asaddTarballToCollection,addRemoteTarballToCollection, andaddTarballToCollectionFromStdin.However, this means if a user previously ran
downloads importto customize their registry, runningadd-urlwill overwrite their file with the embedded list plus the new entry, potentially losing their customizations.This is pre-existing behavior, but worth noting for a future improvement.
♻️ Potential fix (for future consideration)
// Load existing collection - prefer local file if it exists var tarballCollection downloads.TarballCollection if downloads.TarballRegistryFileExist() { var err error tarballCollection, err = downloads.ReadTarballFileInfo() if err != nil { return fmt.Errorf("error reading existing tarball registry: %s", err) } } else { tarballCollection = downloads.DefaultTarballRegistry }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/downloads.go` around lines 680 - 681, The code currently assigns downloads.DefaultTarballRegistry to tarballCollection without attempting to load a user-local registry, which can overwrite user customizations; change the assignment in the add-url flow to first check downloads.TarballRegistryFileExist() and if true call downloads.ReadTarballFileInfo() (handling and returning any error) to populate tarballCollection, otherwise fall back to downloads.DefaultTarballRegistry—this mirrors the behavior used in addTarballToCollection, addRemoteTarballToCollection, and addTarballToCollectionFromStdin.downloads/remote_registry_test.go (1)
220-282: Good test coverage; consider adding MariaDB test case.The tests cover MySQL and Percona patterns well. Consider adding a MariaDB test case to ensure the flavor detection works correctly for MariaDB's naming convention (e.g.,
mariadb-10.6.12-linux-systemd-x86_64.tar.gz), as it's a supported flavor in dbdeployer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@downloads/remote_registry_test.go` around lines 220 - 282, Add a MariaDB test case to the TestParseTarballUrlInfo tests slice: insert a struct with name like "mariadb-linux-amd64", url e.g. "https://downloads.mariadb.org/f/mariadb-10.6.12/mariadb-10.6.12-linux-systemd-x86_64.tar.gz", wantName "mariadb-10.6.12-linux-systemd-x86_64.tar.gz", wantVersion "10.6.12", wantShort "10.6", wantOS "Linux", wantArch "amd64", wantFlavor "mariadb", and wantMinimal false so the flavor detection in TestParseTarballUrlInfo validates MariaDB naming; add it alongside the existing cases in the tests slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/downloads.go`:
- Around line 680-681: The code currently assigns
downloads.DefaultTarballRegistry to tarballCollection without attempting to load
a user-local registry, which can overwrite user customizations; change the
assignment in the add-url flow to first check
downloads.TarballRegistryFileExist() and if true call
downloads.ReadTarballFileInfo() (handling and returning any error) to populate
tarballCollection, otherwise fall back to downloads.DefaultTarballRegistry—this
mirrors the behavior used in addTarballToCollection,
addRemoteTarballToCollection, and addTarballToCollectionFromStdin.
In `@downloads/remote_registry_test.go`:
- Around line 220-282: Add a MariaDB test case to the TestParseTarballUrlInfo
tests slice: insert a struct with name like "mariadb-linux-amd64", url e.g.
"https://downloads.mariadb.org/f/mariadb-10.6.12/mariadb-10.6.12-linux-systemd-x86_64.tar.gz",
wantName "mariadb-10.6.12-linux-systemd-x86_64.tar.gz", wantVersion "10.6.12",
wantShort "10.6", wantOS "Linux", wantArch "amd64", wantFlavor "mariadb", and
wantMinimal false so the flavor detection in TestParseTarballUrlInfo validates
MariaDB naming; add it alongside the existing cases in the tests slice.
In `@downloads/remote_registry.go`:
- Around line 573-593: The CheckRemoteUrl function (and its fallback
checkRemoteUrl) currently use the default http client with no timeout; update
both to use an http.Client with a sensible Timeout (e.g., a few seconds) and
call client.Head / client.Get instead of http.Head / http.Get so requests cannot
hang indefinitely; ensure you still close resp.Body in CheckRemoteUrl and mirror
the same client/timeout usage in the checkRemoteUrl helper for consistency
(reference functions: CheckRemoteUrl and checkRemoteUrl).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05df877b-0fb5-425b-9e30-fc97ecc94b41
📒 Files selected for processing (4)
cmd/downloads.godownloads/remote_registry.godownloads/remote_registry_test.goglobals/globals.go
There was a problem hiding this comment.
Pull request overview
Adds a new dbdeployer downloads add-url <url> CLI flow to register remote tarball URLs into the local tarball registry, with filename-based metadata detection and optional URL accessibility verification.
Changes:
- Introduces
downloads add-urlcommand with override flags and optional URL verification. - Adds URL/filename parsing helpers for auto-detecting flavor/version/OS/arch/minimal.
- Adds unit tests for tarball URL parsing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
globals/globals.go |
Adds a new flag label for skipping URL verification. |
downloads/remote_registry.go |
Adds exported URL-check helper plus tarball URL parsing + OS/arch detection helpers. |
downloads/remote_registry_test.go |
Adds tests covering common tarball URL patterns (MySQL/Percona/macOS/minimal). |
cmd/downloads.go |
Implements new downloads add-url Cobra command and wires flags/registry write flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func ParseTarballUrlInfo(tarballUrl string) (TarballDescription, error) { | ||
| fileName := common.BaseName(tarballUrl) | ||
| if fileName == "" { | ||
| return TarballDescription{}, fmt.Errorf("could not determine filename from URL: %s", tarballUrl) | ||
| } |
There was a problem hiding this comment.
ParseTarballUrlInfo uses common.BaseName(tarballUrl) (filepath.Base) to extract the filename. For URLs with query strings (e.g. signed URLs like .../mysql-8.4.8.tar.xz?token=...) this will include the query and break FindTarballInfo parsing. Consider parsing with net/url and using path.Base(u.Path) (and/or stripping ? / #) before attempting tarball detection.
| func CheckRemoteUrl(remoteUrl string) (int64, error) { | ||
| // Try HEAD first to avoid downloading the file | ||
| // #nosec G107 | ||
| resp, err := http.Head(remoteUrl) | ||
| if err == nil { | ||
| defer func(Body io.ReadCloser) { | ||
| _ = Body.Close() | ||
| }(resp.Body) | ||
| if resp.StatusCode == http.StatusOK { | ||
| var size int64 | ||
| for key := range resp.Header { | ||
| if strings.EqualFold(key, "Content-Length") && len(resp.Header[key]) > 0 { | ||
| size, _ = strconv.ParseInt(resp.Header[key][0], 10, 0) | ||
| } | ||
| } | ||
| return size, nil | ||
| } | ||
| } | ||
| // Fall back to GET if HEAD failed or returned non-200 | ||
| return checkRemoteUrl(remoteUrl) |
There was a problem hiding this comment.
CheckRemoteUrl (and the underlying checkRemoteUrl fallback) uses the default http client (http.Head/http.Get) with no timeout, so downloads add-url can hang indefinitely on slow/stalled connections. Consider using an http.Client with a reasonable Timeout (or request context with deadline) and reusing it for both HEAD/GET attempts.
| if resp.StatusCode == http.StatusOK { | ||
| var size int64 | ||
| for key := range resp.Header { | ||
| if strings.EqualFold(key, "Content-Length") && len(resp.Header[key]) > 0 { | ||
| size, _ = strconv.ParseInt(resp.Header[key][0], 10, 0) | ||
| } | ||
| } | ||
| return size, nil |
There was a problem hiding this comment.
In CheckRemoteUrl, Content-Length extraction iterates headers and uses strconv.ParseInt(..., 10, 0). Two improvements: (1) use resp.ContentLength (already parsed by net/http) or Header.Get("Content-Length") instead of looping, and (2) parse with bitSize 64 to avoid int-size-dependent overflow. This also reduces duplicated logic vs checkRemoteUrl().
| if overrideMinimal { | ||
| tarballDesc.Minimal = true |
There was a problem hiding this comment.
The --minimal override currently can only force Minimal=true. If the filename auto-detect sets Minimal=true, there is no way to override it back to false (even --minimal=false won’t work) because the code only applies the override when overrideMinimal is true. Consider checking whether the flag was explicitly set (Flags().Lookup(...).Changed) and then assigning tarballDesc.Minimal = overrideMinimal.
| if overrideMinimal { | |
| tarballDesc.Minimal = true | |
| // Only override the Minimal field if the --minimal flag was explicitly set. | |
| if minimalFlag := cmd.Flags().Lookup("minimal"); minimalFlag != nil && minimalFlag.Changed { | |
| tarballDesc.Minimal = overrideMinimal |
| // Load existing collection and add the new entry | ||
| var tarballCollection = downloads.DefaultTarballRegistry | ||
|
|
||
| existingTarball, findErr := downloads.FindTarballByName(tarballDesc.Name) | ||
| if findErr == nil { | ||
| if overwrite { | ||
| var newList []downloads.TarballDescription | ||
| newList, err = downloads.DeleteTarball(tarballCollection.Tarballs, tarballDesc.Name) | ||
| if err != nil { | ||
| return fmt.Errorf("error removing existing tarball %s from list: %s", tarballDesc.Name, err) | ||
| } | ||
| tarballCollection.Tarballs = newList | ||
| } else { | ||
| ops.DisplayTarball(existingTarball) | ||
| fmt.Println() | ||
| return fmt.Errorf("tarball %s already in the list; use --overwrite to replace it", tarballDesc.Name) | ||
| } | ||
| } | ||
|
|
||
| tarballCollection.Tarballs = append(tarballCollection.Tarballs, tarballDesc) | ||
|
|
||
| err = downloads.WriteTarballFileInfo(tarballCollection) | ||
| if err != nil { | ||
| return fmt.Errorf("error writing tarball list: %s", err) | ||
| } | ||
| fmt.Printf("Tarball below added to %s\n", downloads.TarballFileRegistry) | ||
| ops.DisplayTarball(tarballDesc) |
There was a problem hiding this comment.
PR description says custom URLs are saved to ~/.dbdeployer/custom_tarballs.json and merged with the embedded registry at runtime, but this implementation writes to downloads.TarballFileRegistry (tarball-list.json) via WriteTarballFileInfo and relies on replacing DefaultTarballRegistry on startup. Either update the PR description/command help to match the existing tarball-list behavior, or implement a separate custom registry file + merge logic so users don’t pin themselves to an old full registry snapshot.
Summary
dbdeployer downloads add-url <url>--version,--OS,--arch,--flavor~/.dbdeployer/custom_tarballs.json)Closes #59
Test Plan
dbdeployer downloads add-url https://dev.mysql.com/get/Downloads/MySQL-8.4/mysql-8.4.8-linux-glibc2.17-x86_64.tar.xzSummary by CodeRabbit
Release Notes
downloads add-urlcommand to import tarballs from HTTP/HTTPS URLs