fix restore command to match README documentation #471

youngminz · 2025-09-01T10:55:10Z

Description

This PR fixes the restore command to work exactly as documented in README.md, resolving the "requires at least 1 arg(s)" error and S3 path handling issues.

Problem

The restore command implementation didn't match README documentation:

Required at least one positional argument, causing errors when following README examples
S3 path handling incorrectly concatenated paths, resulting in NoSuchKey errors

Solution

Simplified and fixed the restore command:

Accept target from positional argument, --target flag, or DB_RESTORE_TARGET environment variable
Always use full path from URL (no filename splitting)
Fixed S3 path handling to correctly process full URLs

Code changes:

cmd/restore.go: Simplified target resolution logic
pkg/storage/s3/s3.go: Fixed path handling when source is empty

Supported Usage Patterns

All three methods now work correctly:

# 1. Positional argument (simplest)
./mysql-backup restore s3://bucket/path/file.tgz

# 2. Environment variable (README documented)
DB_RESTORE_TARGET=s3://bucket/path/file.tgz ./mysql-backup restore

# 3. Flag (explicit)
./mysql-backup restore --target=s3://bucket/path/file.tgz

Testing

Tested successfully with both S3 and local files:

S3 Tests

# All three methods work
$ ./mysql-backup restore s3://bucket/mysql-backups/db_backup_2025-09-01T04:56:43Z.tgz
INFO[0000] Restore complete

$ DB_RESTORE_TARGET=s3://bucket/mysql-backups/db_backup_2025-09-01T04:56:43Z.tgz ./mysql-backup restore
INFO[0000] Restore complete

$ ./mysql-backup restore --target=s3://bucket/mysql-backups/db_backup_2025-09-01T04:56:43Z.tgz
INFO[0000] Restore complete

Local File Tests

# All three methods work
$ ./mysql-backup restore /home/ubuntu/mysql-backup/db_backup_2025-09-01T04:56:43Z.tgz
INFO[0000] Restore complete

$ DB_RESTORE_TARGET=/home/ubuntu/mysql-backup/db_backup_2025-09-01T04:56:43Z.tgz ./mysql-backup restore
INFO[0000] Restore complete

$ ./mysql-backup restore --target=/home/ubuntu/mysql-backup/db_backup_2025-09-01T04:56:43Z.tgz
INFO[0000] Restore complete

Error Handling

# Missing target
$ ./mysql-backup restore
Error: target must be specified as argument, --target flag, or DB_RESTORE_TARGET environment variable

# File not found
$ ./mysql-backup restore s3://bucket/not-exists
Error: NoSuchKey

Fixes

Fixes Restore from s3://path/to/xxx.tgz does not work in docker-compose.yml #432 and restore command requires argument but README shows none #470 - "requires at least 1 arg(s)" error
Resolves S3 path handling issues mentioned in PR fixes path issue when reading dumps from s3 #435

Signed-off-by: Youngmin Koo <[email protected]>

youngminz · 2025-09-01T11:13:33Z

cmd/restore.go

+			}
+
+			// Always pass empty targetFile to use the full path from the URL
+			targetFile := ""


Instead of just setting targetFile := "" and passing empty strings around, should we completely remove the TargetFile field from the entire codebase?

This will not exactly work. The "target" really is the store, or location, while the "file" is the path in that location. The fact that you specify it as a single string is a convenience for the restore. Look at how dump works.

Which means that for this to work, you likely will need to separate the single URL into the two parts, file and store.

deitch

Thanks for this PR to clean things up; it is much appreciated.

I had a small change request in the priority for setting the target. You will need to have target and file within target as separate things to pass.

Lastly, cases for the s3 would help understand the issue.

deitch · 2025-09-03T16:15:24Z

cmd/restore.go

+
+			// Get target from args[0], --target flag, or DB_RESTORE_TARGET environment variable
+			var target string
+			if len(args) > 0 {


Can we change this order? The priority should be CLI flag over command line positional option over env var. I know the positional one is weird, but it has a long history, so I am hesitant to remove it.

deitch · 2025-09-03T16:17:44Z

cmd/restore.go

+			}
+
+			// Always pass empty targetFile to use the full path from the URL
+			targetFile := ""


This will not exactly work. The "target" really is the store, or location, while the "file" is the path in that location. The fact that you specify it as a single string is a convenience for the restore. Look at how dump works.

Which means that for this to work, you likely will need to separate the single URL into the two parts, file and store.

deitch · 2025-09-03T16:17:52Z

cmd/restore.go


 	flags := cmd.Flags()
 	flags.String("target", "", "full URL target to the backup that you wish to restore")
-	if err := cmd.MarkFlagRequired("target"); err != nil {


Good call here.

deitch · 2025-09-03T16:19:28Z

pkg/storage/s3/s3.go

+	// If source is empty, use the path from URL directly (for restore command)
+	// Otherwise, append source to the URL path (for dump command)
+	var objectPath string
+	if source == "" {


We had some issues a while back with the pathing of objects, that was cleaned up (I could find the PR+commit, if needed). Maybe something still lingers.

It would help if you can explain what combinations trigger the error?

deitch · 2025-09-03T16:19:42Z

pkg/storage/s3/s3.go

+	bucket := s.url.Hostname()
+	// If source is empty, use the path from URL directly (for restore command)
+	// Otherwise, append source to the URL path (for dump command)
+	var objectPath string


Good call on cleaning up the overload of path

youngminz · 2025-09-04T05:29:03Z

@deitch Thank you for the detailed review. After understanding the architectural design (separation of target/store and file), I realize my changes would break the existing interface - which feels too risky for a mature project with a long history.

I'd like to close this PR and create a new one that only updates the README.md to match the current behavior. This would fix #470 without any breaking changes. Would a documentation-only PR be acceptable? I think it's a safer approach.

Thanks for helping me understand the project better!

deitch · 2025-09-04T16:25:17Z

Would a documentation-only PR be acceptable? I think it's a safer approach.

Definitely. And it would be appreciated too.

fix restore command to support documented URL patterns

5b42a49

Signed-off-by: Youngmin Koo <[email protected]>

youngminz force-pushed the master branch from 5caa89c to 5b42a49 Compare September 1, 2025 10:57

youngminz commented Sep 1, 2025

View reviewed changes

deitch requested changes Sep 3, 2025

View reviewed changes

youngminz closed this Sep 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix restore command to match README documentation #471

fix restore command to match README documentation #471

Uh oh!

youngminz commented Sep 1, 2025

Uh oh!

youngminz Sep 1, 2025

Uh oh!

deitch Sep 3, 2025

Uh oh!

deitch left a comment

Uh oh!

deitch Sep 3, 2025

Uh oh!

deitch Sep 3, 2025

Uh oh!

deitch Sep 3, 2025

Uh oh!

deitch Sep 3, 2025

Uh oh!

deitch Sep 3, 2025

Uh oh!

youngminz commented Sep 4, 2025

Uh oh!

deitch commented Sep 4, 2025

Uh oh!

Uh oh!

fix restore command to match README documentation #471

fix restore command to match README documentation #471

Uh oh!

Conversation

youngminz commented Sep 1, 2025

Description

Problem

Solution

Simplified and fixed the restore command:

Code changes:

Supported Usage Patterns

Testing

S3 Tests

Local File Tests

Error Handling

Fixes

Uh oh!

youngminz Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

deitch left a comment

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

deitch Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

youngminz commented Sep 4, 2025

Uh oh!

deitch commented Sep 4, 2025

Uh oh!

Uh oh!