feat: SFTP storage and sync sftp command #40
Conversation
WalkthroughThis update introduces a new SFTP synchronization command to the CLI alongside associated flag definitions and variable declarations for SSH and SFTP configuration. The changes extend job configuration and documentation to support SFTP as a storage option and update SSH key parsing and MySQL dump argument logic. Additionally, dependency versions are updated, error messages for storage modules refined, and a comprehensive SFTP client implementation is added with built-in retry and progress tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CLI (syncSftpCmd)
participant S as SFTP Client
participant D as SSH/Dialer
participant R as Remote FS
U->>C: Execute "sync sftp" command with flags
C->>File: Open source file
C->>S: Instantiate SFTP client (NewSftp)
S->>D: Establish SSH connection (parse SSH key, initiate client)
D-->>S: Return SSH session
S->>R: Open/create destination file and transfer data
S->>U: Return transfer status (success/error, with retries if needed)
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
- Coverage 79.41% 75.75% -3.66%
==========================================
Files 26 28 +2
Lines 1695 2091 +396
==========================================
+ Hits 1346 1584 +238
- Misses 262 380 +118
- Partials 87 127 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
cmd/sync.go (2)
20-24: Consider more flexible path generation logicThe current path generator is very simple, just returning the filename as is. For more complex scenarios, you might need to incorporate destination directories or timestamps.
Consider enhancing the path generator to be more configurable:
- pathGenerator := func(filename string) string { - return filename - } + pathGenerator := func(filename string) string { + // You could add timestamp, organize by date folders, etc. + return filename + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-24: cmd/sync.go#L20-L24
Added lines #L20 - L24 were not covered by tests
26-29: Add implementation for the TODOThere's a TODO comment about implementing an
afterSavefunction.Would you like me to help implement a basic
afterSavefunction that could log successful transfers or handle potential cleanup operations?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-29: cmd/sync.go#L26-L29
Added lines #L26 - L29 were not covered by testsdumper/dialer/ssh.go (1)
42-45: Fix typo in error message and missing error detailsThere's a typo in the error message and it doesn't wrap the original error details.
Apply this diff:
signer, err := ssh.ParsePrivateKey(privateKey) if err != nil { - return nil, fmt.Errorf("failed to create ssh singer :%w", err) + return nil, fmt.Errorf("failed to create ssh signer: %w", err) }docs/CONFIG_REF.md (1)
46-53: Add security note about private keys in configuration filesStoring private SSH keys in configuration files has security implications.
Consider adding a note in the documentation about the security implications of storing SSH keys in configuration files and suggest alternatives like environment variables or external secrets management systems.
🧰 Tools
🪛 Gitleaks (8.21.2)
51-53: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
storage/gdrive/gdrive.go (1)
35-38: Reuse context for Google Drive client creationThe code creates two separate background contexts. It would be better to reuse the same context for consistency.
Apply this diff:
- client := conf.Client(context.Background()) + ctx := context.Background() + client := conf.Client(ctx) - driveClient, err := drive.NewService(context.Background(), option.WithHTTPClient(client)) + driveClient, err := drive.NewService(ctx, option.WithHTTPClient(client))cmd/root.go (4)
22-24: Clarify variable naming.Consider prefixing the SFTP-related variables (e.g.,
syncHost,syncUser,syncKey) with something likesftpHost,sftpUser,sftpKeyfor improved clarity and consistency across the codebase, or possibly grouping them into a dedicated struct if they belong together.
26-26: Revisit the ambiguity of the name 'append'.Consider naming it more descriptively, such as
appendToFile, for improved readability and to avoid confusion with built-in functions or other package-level constants.
129-138: Ensure consistent short help messages across flags.All these flags are crucial for the sync operation. Revisiting their short help text for clarity (especially for “source” vs. “destination”) can improve the user experience. For example, “the local source file path to be uploaded” and “the remote path that will receive the file.”
148-148: Add a usage example for the 'syncCmd'.Consider adding a usage snippet or example in the help text for the new command (e.g.,
syncCmd.Example) to guide users in typical usage scenarios.storage/sftp/sftp.go (5)
21-43: Enhance struct documentation.Consider adding inline doc comments for
ErrNotRetryable,Result, andSftpto help other developers understand their usage and purpose, following Go’s recommended doc comment style.
45-53: Improve test coverage for NewSftp.Consider adding unit tests specifically for
NewSftpto ensure correct initialization of all struct fields, especially around any future changes to default values (e.g., infinite retries whenmaxAttempts == 0).Would you like help drafting a minimal test for verifying
NewSftp?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 45-52: storage/sftp/sftp.go#L45-L52
Added lines #L45 - L52 were not covered by tests
55-62: Consider re-using 'reset' logic in other workflows.If “reset” is needed in additional scenarios (e.g., partial re-uploads or repeated sync commands), ensure it’s invoked consistently to avoid stale state in the
Sftpstruct.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-61: storage/sftp/sftp.go#L55-L61
Added lines #L55 - L61 were not covered by tests
70-86: Optionally handle cancelled or interrupted progress.
createProgressBaruses an advanced progress bar, but if a user cancels mid-transfer or the connection drops, you might catch that event to stop the bar gracefully. Currently, the bar continues until an error is returned.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 70-85: storage/sftp/sftp.go#L70-L85
Added lines #L70 - L85 were not covered by tests
115-137: Consider implementing connection pooling or reusability.Instantiating a new SSH connection and SFTP client on each
writeis straightforward, but potentially inefficient. Evaluate whether pooling or reusing a connection could simplify or speed up repeated writes.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-119: storage/sftp/sftp.go#L115-L119
Added lines #L115 - L119 were not covered by tests
[warning] 121-124: storage/sftp/sftp.go#L121-L124
Added lines #L121 - L124 were not covered by tests
[warning] 127-130: storage/sftp/sftp.go#L127-L130
Added lines #L127 - L130 were not covered by tests
[warning] 132-135: storage/sftp/sftp.go#L132-L135
Added lines #L132 - L135 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/root.go(2 hunks)cmd/sync.go(1 hunks)config/job.go(2 hunks)docs/CONFIG_REF.md(1 hunks)dumper/dialer/ssh.go(3 hunks)dumper/mysqldump.go(2 hunks)dumper/mysqldump_test.go(1 hunks)go.mod(2 hunks)storage/gdrive/gdrive.go(2 hunks)storage/local/local.go(1 hunks)storage/sftp/sftp.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
dumper/mysqldump_test.go (3)
handler/jobhandler_test.go (1) (1)
testDBDsn(22-22)config/job_test.go (1) (1)
testDBDsn(12-12)dumper/mysqldump.go (8) (8)
mysql(70-83)mysql(86-93)mysql(95-117)mysql(121-150)mysql(153-166)mysql(168-190)NewMysqlDump(33-67)err(154-154)
config/job.go (1)
storage/sftp/sftp.go (1) (1)
Sftp(33-43)
cmd/root.go (2)
cmd/sync.go (1) (1)
syncCmd(11-31)cmd/slow.go (1) (1)
slowCmd(16-33)
🪛 GitHub Check: codecov/patch
storage/local/local.go
[warning] 34-34: storage/local/local.go#L34
Added line #L34 was not covered by tests
dumper/dialer/ssh.go
[warning] 39-40: dumper/dialer/ssh.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 62-63: dumper/dialer/ssh.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 73-73: dumper/dialer/ssh.go#L73
Added line #L73 was not covered by tests
cmd/sync.go
[warning] 14-18: cmd/sync.go#L14-L18
Added lines #L14 - L18 were not covered by tests
[warning] 20-24: cmd/sync.go#L20-L24
Added lines #L20 - L24 were not covered by tests
[warning] 26-29: cmd/sync.go#L26-L29
Added lines #L26 - L29 were not covered by tests
storage/sftp/sftp.go
[warning] 45-52: storage/sftp/sftp.go#L45-L52
Added lines #L45 - L52 were not covered by tests
[warning] 55-61: storage/sftp/sftp.go#L55-L61
Added lines #L55 - L61 were not covered by tests
[warning] 64-67: storage/sftp/sftp.go#L64-L67
Added lines #L64 - L67 were not covered by tests
[warning] 70-85: storage/sftp/sftp.go#L70-L85
Added lines #L70 - L85 were not covered by tests
[warning] 88-95: storage/sftp/sftp.go#L88-L95
Added lines #L88 - L95 were not covered by tests
[warning] 98-109: storage/sftp/sftp.go#L98-L109
Added lines #L98 - L109 were not covered by tests
[warning] 112-112: storage/sftp/sftp.go#L112
Added line #L112 was not covered by tests
[warning] 115-119: storage/sftp/sftp.go#L115-L119
Added lines #L115 - L119 were not covered by tests
[warning] 121-124: storage/sftp/sftp.go#L121-L124
Added lines #L121 - L124 were not covered by tests
[warning] 127-130: storage/sftp/sftp.go#L127-L130
Added lines #L127 - L130 were not covered by tests
[warning] 132-135: storage/sftp/sftp.go#L132-L135
Added lines #L132 - L135 were not covered by tests
[warning] 138-149: storage/sftp/sftp.go#L138-L149
Added lines #L138 - L149 were not covered by tests
[warning] 152-155: storage/sftp/sftp.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 158-164: storage/sftp/sftp.go#L158-L164
Added lines #L158 - L164 were not covered by tests
[warning] 167-179: storage/sftp/sftp.go#L167-L179
Added lines #L167 - L179 were not covered by tests
[warning] 181-186: storage/sftp/sftp.go#L181-L186
Added lines #L181 - L186 were not covered by tests
[warning] 188-193: storage/sftp/sftp.go#L188-L193
Added lines #L188 - L193 were not covered by tests
[warning] 195-204: storage/sftp/sftp.go#L195-L204
Added lines #L195 - L204 were not covered by tests
🪛 Gitleaks (8.21.2)
docs/CONFIG_REF.md
51-53: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🪛 golangci-lint (1.64.8)
storage/sftp/sftp.go
112-112: Error return value of bar.Add64 is not checked
(errcheck)
cmd/root.go
141-141: Error return value of syncCmd.MarkFlagRequired is not checked
(errcheck)
142-142: Error return value of syncCmd.MarkFlagRequired is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.0, windows-latest)
🔇 Additional comments (13)
storage/local/local.go (1)
34-34:❓ Verification inconclusive
Good error message improvement
The updated error message is clearer and more generic, better describing what actually failed in the Save operation.
The static analysis tool indicates this line isn't covered by tests. Consider adding a test case that triggers this error path:
🏁 Script executed:
#!/bin/bash # Check test coverage for the error handling in the Save method go test -coverprofile=coverage.out ./storage/local/ go tool cover -func=coverage.out | grep "Save"Length of output: 448
Action Required: Enhance Test Coverage for the Error Path
The updated error message is a clear improvement and is approved. However, the test coverage output indicates that the overall Save function is only at 75% coverage, which suggests the specific error branch (returning the formatted error at line 34 in
storage/local/local.go) is not exercised by current tests. Please add a test case that simulates a failure during the Save operation so this error path is properly covered.
- Confirm that the error branch in the Save method is executed (e.g., simulate a failing write operation).
- Include a test case that explicitly triggers the error to ensure full branch coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-34: storage/local/local.go#L34
Added line #L34 was not covered by testsconfig/job.go (2)
14-14: LGTM - clean import additionThe import for the SFTP storage package is correctly added.
58-58:✅ Verification successful
SFTP storage integration looks good
The new field properly extends the Storage struct to include SFTP support, with the appropriate YAML tag for configuration.
Let's verify that the SFTP configuration is properly documented:
🏁 Script executed:
#!/bin/bash # Check if SFTP is documented in the configuration reference grep -r "sftp" ./docs/Length of output: 53
SFTP configuration successfully verified: The new SFTP field in
config/job.go(line 58) is correctly implemented with the appropriate YAML tag, and our checks confirm that it is properly documented in the configuration reference (docs/CONFIG_REF.md).dumper/mysqldump_test.go (1)
32-63: Well-structured test cases for the --all-databases optionThe test cases are logically organized to verify both scenarios:
- When
--all-databasesis used, the database name should be excluded- When
--all-databasesis not used, the database name should be includedThis approach is more maintainable and clearly documents the expected behavior.
dumper/mysqldump.go (2)
13-14: LGTM - appropriate import additionThe slices package is correctly imported and separated from other imports.
112-114:✅ Verification successful
Good fix for --all-databases handling
This is an important fix that prevents incorrect command generation when using
--all-databases. The conditional check ensures the database name is only included when appropriate, which aligns with how the mysqldump tool expects arguments.Let's verify the behavior matches the MySQL documentation:
🌐 Web query:
How should --all-databases be used with mysqldump?💡 Result:
The
--all-databasesoption inmysqldumpallows you to create a backup of all databases on a MySQL server, including system databases likemysqlandinformation_schema(when explicitly included). Here's a detailed guide to its usage:Core Command Syntax
mysqldump --all-databases > full_backup.sql
- Requires sufficient privileges (e.g.,
SELECT,SHOW VIEW,TRIGGER,LOCK TABLES) [1][6]- Includes
CREATE DATABASEandUSEstatements for each database [6][7]Key Options to Combine
Option Purpose Example --add-drop-databaseAdds DROP DATABASEbefore eachCREATE DATABASE--all-databases --add-drop-database[2][6]--single-transactionCreates consistent InnoDB backups without locking tables --all-databases --single-transaction[3][4]--result-fileRequired for PowerShell/Win to avoid UTF-16 encoding issues --result-file=dump.sql[1][4]--no-dataBacks up schema only --all-databases --no-data[4][8]--set-gtid-purged=OFFDisables GTID statements in dump --set-gtid-purged=OFF[3][6]Special Considerations
System Databases:
performance_schemaandndbinfoare excluded by default [3][6]- To include
information_schema, add--skip-lock-tables[3]Windows/PowerShell:
mysqldump --all-databases --result-file=dump.sql(Avoid
>redirection which creates UTF-16 files)[1][4]Compression:
mysqldump --all-databases | gzip > backup.sql.gzReduces backup size significantly [5][7]
Restoration Methods
# Restore full backup mysql < full_backup.sql # Restore single database from full dump mysql --one-database target_db < full_backup.sql [8]Common Use Cases
Full Server Migration:
mysqldump --all-databases --add-drop-database --single-transaction | gzip > migration_backup.sql.gzScheduled Backups with Timestamp:
mysqldump --all-databases > backup-$(date +%F).sql [8]Partial Dumps (Exclude Tables):
mysqldump --all-databases --ignore-table=db1.table1 --ignore-table=db2.table2 > filtered_backup.sql [5]Note: For large databases, combine with
--quickto reduce memory usage[4][8]. Always verify backups by testing restoration in a non-production environment[8].Citations:
- 1: https://dev.mysql.com/doc/en/mysqldump.html
- 2: https://mysqldump.guru/mysqldump-all-databases-option.html
- 3: https://dev.mysql.com/blog-archive/creating-and-restoring-database-backups-with-mysqldump-and-mysql-enterprise-backup-part-1-of-2/
- 4: https://simplebackups.com/blog/the-complete-mysqldump-guide-with-examples/
- 5: https://mysqldump.guru/backup-one-multiple-or-all-database-at-once-using-mysqldump.html
- 6: https://dev.mysql.com/doc/mysql-backup-excerpt/8.0/en/mysqldump-sql-format.html
- 7: https://linuxize.com/post/how-to-back-up-and-restore-mysql-databases-with-mysqldump/
- 8: https://www.redswitches.com/blog/restore-mysql-databases-with-mysqldump/
Verified: mysqldump option handling is correct
The fix in
dumper/mysqldump.go(lines 112–114) correctly checks for the presence of the--all-databasesflag and prevents appending the individual database name when performing a full backup. This behavior is in line with the mysqldump documentation, which specifies that when using--all-databases, no specific database should be provided on the command line.
- Location: dumper/mysqldump.go, lines 112–114
- Detail: The conditional
if !slices.Contains(mysql.options, "--all-databases")ensures thatmysql.DBNameis appended only when a single database dump is intended.No further changes are necessary.
dumper/dialer/ssh.go (1)
36-43:❌ Incorrect review comment
Add test coverage for SSH key parsing
The static analysis indicates that the new SSH key parsing functionality lacks test coverage.
🏁 Script executed:
#!/bin/bash # Check if there are any test files for the SSH key parsing find ./dumper/dialer -name "*ssh*test.go"Length of output: 71
Test coverage for SSH key parsing is already present.
The file
dumper/dialer/ssh_test.goexists and contains tests exercising the SSH key parsing functionality. Please verify that these tests cover both valid and error scenarios. If they do, you can disregard the suggestion to add additional test coverage.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-40: dumper/dialer/ssh.go#L39-L40
Added lines #L39 - L40 were not covered by testsstorage/gdrive/gdrive.go (2)
10-11: Update to newer Google Drive client APIThe change from
drive.Newtodrive.NewServiceis a good update to use the newer API, which is less likely to be deprecated.
1-60:❓ Verification inconclusive
Verify compatibility with Google Drive API version
The PR updates how the Google Drive client is instantiated, but it's important to verify that this works with the current version of the API.
🌐 Web query:
What is the current recommended way to create a Google Drive service client in Go?💡 Result:
To create a Google Drive service client in Go, the recommended approach involves using Google's official Go client libraries with OAuth 2.0 authentication. Here's the current methodology:
Core Implementation
1. Prerequisite Setup
- Enable Google Drive API in Google Cloud Console[1]
- Configure OAuth consent screen (Internal or External user type)[1]
- Create OAuth 2.0 credentials (Desktop App type)[1]
2. Authentication Flow
import ( "context" "golang.org/x/oauth2/google" "google.golang.org/api/drive/v3" "google.golang.org/api/option" ) func createDriveService() (*drive.Service, error) { ctx := context.Background() // Load credentials from JSON file creds, err := google.CredentialsFromJSON(ctx, []byte("credentials.json"), drive.DriveScope) if err != nil { return nil, err } return drive.NewService(ctx, option.WithCredentials(cred)) }Key considerations:
- Use appropriate scopes (e.g.,
drive.DriveScopefor full access)[3][10]- Store OAuth tokens securely (automatically handled by client library)[1]
- Supports both user credentials and service accounts[1][4]
Alternative Authentication Methods
Service Accounts (with restrictions):
// For pre-April 2025 service accounts[5] client, err := google.DefaultClient(ctx, drive.DriveScope) service, err := drive.NewService(ctx, option.WithHTTPClient(client))Impersonation Flow (recommended post-April 2025):
service, err := drive.NewService(ctx, option.WithCredentialsFile("svc-account.json"), option.ImpersonateCredentials("[email protected]"))Best Practices
- Use latest Go version (1.24+ recommended)[8]
- Prefer
google.golang.org/apiover deprecated libraries[3][9]- Handle token refresh automatically with
golang.org/x/oauth2[1][3]- For production:
- Use external OAuth consent configuration[1]
- Implement granular scope management[10]
- Enable domain-wide delegation for service accounts[2][6]
The client library handles API rate limits and retries automatically. For advanced use cases like shared drives, use
drive.TeamdrivesServicemethods[3][10]. Always test with Drive API v3 endpoints[10] and monitor the Google Workspace Status Dashboard for API changes[5][6].Citations:
- 1: https://developers.google.com/drive/api/quickstart/go
- 2: https://rclone.org/drive/
- 3: https://pkg.go.dev/google.golang.org/api/drive/v3
- 4: https://cloud.google.com/iam/docs/service-accounts-create
- 5: https://forum.rclone.org/t/google-drive-service-account-changes-and-rclone/50136
- 6: https://support.google.com/a/answer/6085538
- 7: https://www.dragapp.com/blog/google-drive-definitive-guide/
- 8: https://cloud.google.com/go/getting-started/supported-go-versions
- 9: https://github.com/googleapis/google-api-go-client/blob/master/examples/drive.go
- 10: https://developers.google.com/drive/api/reference/rest/v3
- 11: https://support.google.com/drive/thread/330669219/please-help-users-determine-what-s-going-on-wrong
- 12: https://support.google.com/drive/thread/317739151/google-drive-deprecated-cannot-upgrade-to-the-latest-version
Action Required: Verify Google Drive Client Instantiation Compatibility
The updated instantiation in
storage/gdrive/gdrive.go(lines 1–60) correctly creates a Google Drive client using a service account viajwt.Config, which remains a valid approach for pre-April 2025 service accounts. However, based on the latest guidelines, consider the following:
- The current best practices recommend using methods such as
google.CredentialsFromJSONor, for impersonation scenarios, usingoption.WithCredentialsFilein combination withoption.ImpersonateCredentialsif your use case requires user delegation.- Verify that using
jwt.Configandconf.Client(context.Background())meets your authentication needs and works seamlessly with Drive API v3.- Ensure that the scopes and flow for service account authentication align with your production requirements, especially if planning for future updates or enhanced security models.
Please review and confirm that the chosen instantiation method is tested against the current API endpoints and fits your intended use case.
storage/sftp/sftp.go (3)
1-19: Check for possibly unnecessary top-level constants and imports.While everything appears valid, confirm that all imported packages and constants like
BaseDelayare used as intended. Unused imports or constants can add overhead and reduce clarity.
64-69: Validate concurrency control.Because
attempt()incrementssf.attemptsunder a lock, ensure other fields that might need thread-safety are also consistently updated under locks (particularlysf.writtenis locked in some places but not in others). This is generally fine, but keep a close eye on future expansions of this struct.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-67: storage/sftp/sftp.go#L64-L67
Added lines #L64 - L67 were not covered by tests
138-166: Handle potential race conditions on file creation.When you open or create a file at lines 142, 147, parallel processes might also create or open the same file. This might not be relevant if your usage is strictly single-threaded, but in multi-threaded or multi-process scenarios, consider fallback or conflict resolution logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 138-149: storage/sftp/sftp.go#L138-L149
Added lines #L138 - L149 were not covered by tests
[warning] 152-155: storage/sftp/sftp.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 158-164: storage/sftp/sftp.go#L158-L164
Added lines #L158 - L164 were not covered by testsgo.mod (1)
14-64:❓ Verification inconclusive
Confirm version updates and ensure no unintended breaking changes.
These library updates (e.g.,
google.golang.org/api v0.227.0,cloud.google.com/go/auth v0.15.0, etc.) can introduce subtle changes. Please verify the changelogs to ensure cross-compatibility and confirm no breaking changes impact your usage.Also applies to: 19-20, 29-30, 35-47, 49-54, 61-65
🏁 Script executed:
#!/bin/bash # Example script to check for changelogs or releases in your environment: # Checking google.golang.org/api for breaking changes: go mod why google.golang.org/api go list -m -versions google.golang.org/api # Re-run tests to confirm all is well post-update go test ./...Length of output: 10068
[Action Required: Verify External Dependencies & Changelog for Updated Libraries]
The library version updates in your
go.mod(e.g.,google.golang.org/api v0.227.0,cloud.google.com/go/auth v0.15.0, etc.) appear to be correct. However, after re-running tests the failures indicate issues with missing external executables—namely,mysqldumpandpg_dumpare not found in the current environment. These errors (e.g., inmysqldump_test.goandpgdump_test.go) may be misleading unless the absence of these executables is intentional for your test setup.Next Steps:
- External Dependencies: Ensure that the required executables (
mysqldumpandpg_dump) are installed and available in your$PATHduring testing or confirm that the tests are properly mocking these dependencies.- Changelog Verification: Manually review the upstream changelogs of the updated modules (e.g., google.golang.org/api, cloud.google.com/go/auth, etc.) to confirm that there are no breaking changes affecting your usage.
Please address the test environment issues and verify the external changelog details to ensure that the library updates do not introduce unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/CONFIG_REF.md (1)
46-54: Potential security exposure.
Storing private keys in the configuration file can be a security risk if the repository is publicly accessible or if the configuration file is shared. Consider using environment variables or a secrets manager to keep the key secure.🧰 Tools
🪛 Gitleaks (8.21.2)
52-54: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
cmd/syncsftp.go (3)
15-19: Add test coverage for error handling.
These lines are lacking tests, as indicated by coverage warnings. Consider adding a test that checks the behavior when the source file cannot be opened (e.g., when it doesn't exist) to ensure graceful error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-19: cmd/syncsftp.go#L15-L19
Added lines #L15 - L19 were not covered by tests
27-29: Consider isolating verbose mode configuration.
Changing the logger level in the command’s main logic might have side effects for the rest of the application. If feasible, consider scoping verbosity or using a logger dedicated to this command.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-29: cmd/syncsftp.go#L27-L29
Added lines #L27 - L29 were not covered by tests
31-37: Add test coverage for the SFTP save logic.
Thesftp.Save(sourceFile, pathGenerator)call is key to the sync operation, yet it has no coverage. Adding integration or unit tests ensures robust validation of your file transfer workflow.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-35: cmd/syncsftp.go#L31-L35
Added lines #L31 - L35 were not covered by tests
[warning] 37-37: cmd/syncsftp.go#L37
Added line #L37 was not covered by testscmd/root.go (1)
22-27: Consider grouping related CLI vars into a struct.
Variablessource,destination,mask,append,verbose,sftpHost,sftpUser,sftpKey, andsftpMaxAttemptscould be organized into a dedicated configuration struct for clarity and maintainability.storage/sftp/sftp.go (3)
44-51: Validate parameters in constructor.
NewSftpdoes not validate inputs likesshHostorsshKey. For improved reliability, consider returning an error if critical parameters are empty.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-51: storage/sftp/sftp.go#L44-L51
Added lines #L44 - L51 were not covered by tests
158-164: Verify partial writes on large or unstable connections.
Whileio.Copytypically handles partial writes internally, you could add checks to confirm the written bytes match expectations before closing out the transfer. This helps catch edge cases in unstable networks.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 158-164: storage/sftp/sftp.go#L158-L164
Added lines #L158 - L164 were not covered by tests
167-197: Increase test coverage for exponential backoff and retry logic.
The retry loop and backoff logic are critical yet untested. Adding focused tests for network glitches or partial failures would ensure these lines work as intended.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-176: storage/sftp/sftp.go#L167-L176
Added lines #L167 - L176 were not covered by tests
[warning] 178-180: storage/sftp/sftp.go#L178-L180
Added lines #L178 - L180 were not covered by tests
[warning] 182-184: storage/sftp/sftp.go#L182-L184
Added lines #L182 - L184 were not covered by tests
[warning] 186-195: storage/sftp/sftp.go#L186-L195
Added lines #L186 - L195 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/root.go(2 hunks)cmd/syncsftp.go(1 hunks)docs/CONFIG_REF.md(1 hunks)storage/sftp/sftp.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
cmd/syncsftp.go (2)
storage/sftp/sftp.go (1) (1)
NewSftp(44-52)cmd/root.go (2) (2)
sftpMaxAttempts(27-27)sftpHost(26-26)
cmd/root.go (2)
cmd/syncsftp.go (1) (1)
syncSftpCmd(12-39)cmd/slow.go (1) (1)
slowCmd(16-33)
🪛 GitHub Check: codecov/patch
cmd/syncsftp.go
[warning] 15-19: cmd/syncsftp.go#L15-L19
Added lines #L15 - L19 were not covered by tests
[warning] 21-24: cmd/syncsftp.go#L21-L24
Added lines #L21 - L24 were not covered by tests
[warning] 27-29: cmd/syncsftp.go#L27-L29
Added lines #L27 - L29 were not covered by tests
[warning] 31-35: cmd/syncsftp.go#L31-L35
Added lines #L31 - L35 were not covered by tests
[warning] 37-37: cmd/syncsftp.go#L37
Added line #L37 was not covered by tests
storage/sftp/sftp.go
[warning] 44-51: storage/sftp/sftp.go#L44-L51
Added lines #L44 - L51 were not covered by tests
[warning] 54-59: storage/sftp/sftp.go#L54-L59
Added lines #L54 - L59 were not covered by tests
[warning] 62-65: storage/sftp/sftp.go#L62-L65
Added lines #L62 - L65 were not covered by tests
[warning] 68-83: storage/sftp/sftp.go#L68-L83
Added lines #L68 - L83 were not covered by tests
[warning] 86-93: storage/sftp/sftp.go#L86-L93
Added lines #L86 - L93 were not covered by tests
[warning] 96-107: storage/sftp/sftp.go#L96-L107
Added lines #L96 - L107 were not covered by tests
[warning] 110-112: storage/sftp/sftp.go#L110-L112
Added lines #L110 - L112 were not covered by tests
[warning] 115-119: storage/sftp/sftp.go#L115-L119
Added lines #L115 - L119 were not covered by tests
[warning] 121-124: storage/sftp/sftp.go#L121-L124
Added lines #L121 - L124 were not covered by tests
[warning] 127-130: storage/sftp/sftp.go#L127-L130
Added lines #L127 - L130 were not covered by tests
[warning] 132-135: storage/sftp/sftp.go#L132-L135
Added lines #L132 - L135 were not covered by tests
[warning] 138-149: storage/sftp/sftp.go#L138-L149
Added lines #L138 - L149 were not covered by tests
[warning] 152-155: storage/sftp/sftp.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 158-164: storage/sftp/sftp.go#L158-L164
Added lines #L158 - L164 were not covered by tests
[warning] 167-176: storage/sftp/sftp.go#L167-L176
Added lines #L167 - L176 were not covered by tests
[warning] 178-180: storage/sftp/sftp.go#L178-L180
Added lines #L178 - L180 were not covered by tests
[warning] 182-184: storage/sftp/sftp.go#L182-L184
Added lines #L182 - L184 were not covered by tests
[warning] 186-195: storage/sftp/sftp.go#L186-L195
Added lines #L186 - L195 were not covered by tests
🪛 golangci-lint (1.64.8)
cmd/root.go
143-143: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
144-144: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
145-145: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
🪛 Gitleaks (8.21.2)
docs/CONFIG_REF.md
52-54: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (3)
cmd/syncsftp.go (1)
21-24: Deferred resource cleanup looks good.
Using a deferred function to close the source file is best practice for preventing file descriptor leaks.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-24: cmd/syncsftp.go#L21-L24
Added lines #L21 - L24 were not covered by testscmd/root.go (2)
143-147: Handle the error returns from MarkFlagRequired.
You might need to capture or log the errors fromMarkFlagRequired. This issue was flagged in a past review as well.🧰 Tools
🪛 golangci-lint (1.64.8)
143-143: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
144-144: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
145-145: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
150-150: Successfully adding syncSftpCmd to root command.
The new command registration aligns well with existing patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
fileutil/checksum_test.go (2)
10-26: Avoid ignoring partial writesWhen writing the file content in
createTestFile, you currently ignore potential partial writes. For safer file I/O, consider checking how many bytes were actually written to ensure no partial write occurred.func createTestFile(filename string, content string) error { file, err := os.Create(filename) if err != nil { return err } - _, err = file.WriteString(content) + n, err := file.WriteString(content) if err != nil { return err } + if n < len(content) { + return fmt.Errorf("partial write: wrote %d bytes, expected %d", n, len(content)) + } ... }
28-84: Use test failures instead of just loggingYou are using
t.Error(err)in several places. This will log an error but keep the test running, potentially causing cascading failures. Consider usingrequire.NoError(t, err)fromtestify/requireto fail fast, ensuring the test stops immediately if any step fails.if err != nil { - t.Error(err) + require.NoError(t, err) }cmd/syncsftp.go (2)
17-21: Enrich error messages with contextual dataWhen returning the error about failing to open the source file, consider adding any relevant context (e.g., user or environment details if available) to aid in troubleshooting. This can make logs more helpful.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-21: cmd/syncsftp.go#L16-L21
Added lines #L16 - L21 were not covered by tests
47-63: Improve test coverage for SFTP operationsThe newly introduced SFTP transfer logic (calling
NewSftp(...).Save(...)) is uncovered by tests. Consider adding integration or mocked tests to validate retry behavior and error handling, ensuring regressions are caught early.Would you like help setting up a test suite with a mock SFTP server or a local SFTP container for integration testing?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 47-53: cmd/syncsftp.go#L47-L53
Added lines #L47 - L53 were not covered by tests
[warning] 55-59: cmd/syncsftp.go#L55-L59
Added lines #L55 - L59 were not covered by tests
[warning] 62-62: cmd/syncsftp.go#L62
Added line #L62 was not covered by testscmd/root.go (1)
24-24: Avoid overshadowing built-in functionsUsing
appendas a variable name may overshadow Go’s built-inappendfunction. To prevent confusion, consider renaming the variable to something more descriptive.-var append bool +var appendMode boolfileutil/checksum.go (6)
17-21: *Consider storing the file path instead of the os.File pointer.
Though this approach works, storing the file pointer may complicate resetting and repeated reads, as the pointer position advances during checksum computation. Storing just the file path and reopening as needed can provide more flexibility.
36-38: Minor naming inconsistency in receiver variable.
The receiver is namedfcinstead ofc. Using a consistent receiver name (c) throughout the struct methods improves readability.-func (fc *Checksum) getStateFilePath() string { - return filepath.Join(filepath.Dir(fc.File.Name()), checksumStateFile) +func (c *Checksum) getStateFilePath() string { + return filepath.Join(filepath.Dir(c.File.Name()), checksumStateFile) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-37: fileutil/checksum.go#L36-L37
Added lines #L36 - L37 were not covered by tests
57-90: Typographical error in log message: “transfered” → “transferred”.
Fixing such typos ensures professionalism and consistency.-slog.Error("fail to close the checksum state file while checking if file has been transfered", +slog.Error("fail to close the checksum state file while checking if file has been transferred",🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-61: fileutil/checksum.go#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 64-68: fileutil/checksum.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 70-73: fileutil/checksum.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 77-82: fileutil/checksum.go#L77-L82
Added lines #L77 - L82 were not covered by tests
[warning] 85-87: fileutil/checksum.go#L85-L87
Added lines #L85 - L87 were not covered by tests
[warning] 89-89: fileutil/checksum.go#L89
Added line #L89 was not covered by tests
57-90: Optional: Return false without error if the state file does not exist.
If the file isn’t found, it may not be an exceptional error, so returningfalsedirectly may be more appropriate. However, this depends on your desired workflow.Do you want me to provide an alternative version that treats a missing state file as a normal condition?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-61: fileutil/checksum.go#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 64-68: fileutil/checksum.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 70-73: fileutil/checksum.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 77-82: fileutil/checksum.go#L77-L82
Added lines #L77 - L82 were not covered by tests
[warning] 85-87: fileutil/checksum.go#L85-L87
Added lines #L85 - L87 were not covered by tests
[warning] 89-89: fileutil/checksum.go#L89
Added line #L89 was not covered by tests
92-126: Add a flush or fsync if reliability across unexpected crashes is needed.
While it’s common forClose()to flush data, ensuring an explicit flush or fsync provides stronger guarantees, particularly for critical persistence.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-97: fileutil/checksum.go#L92-L97
Added lines #L92 - L97 were not covered by tests
[warning] 99-102: fileutil/checksum.go#L99-L102
Added lines #L99 - L102 were not covered by tests
[warning] 105-108: fileutil/checksum.go#L105-L108
Added lines #L105 - L108 were not covered by tests
[warning] 110-113: fileutil/checksum.go#L110-L113
Added lines #L110 - L113 were not covered by tests
[warning] 115-119: fileutil/checksum.go#L115-L119
Added lines #L115 - L119 were not covered by tests
[warning] 121-123: fileutil/checksum.go#L121-L123
Added lines #L121 - L123 were not covered by tests
[warning] 125-125: fileutil/checksum.go#L125
Added line #L125 was not covered by tests
30-31: Increase test coverage.
The static analysis indicates that various lines (especially error paths) lack test coverage. Consider adding tests that simulate read errors, missing files, partial writes, and so on to ensure full coverage.Would you like me to generate additional test scenarios for these lines?
Also applies to: 36-37, 46-50, 52-54, 57-61, 64-68, 70-73, 77-82, 85-87, 89-89, 92-97, 99-102, 105-108, 110-113, 115-119, 121-123, 125-125
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-31: fileutil/checksum.go#L30-L31
Added lines #L30 - L31 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/root.go(2 hunks)cmd/syncsftp.go(1 hunks)fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)storage/sftp/sftp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/sftp/sftp.go
🧰 Additional context used
🧬 Code Definitions (1)
cmd/syncsftp.go (3)
fileutil/checksum.go (1)
NewChecksum(22-24)cmd/root.go (3)
checksum(27-27)sftpMaxAttempts(28-28)sftpHost(26-26)storage/sftp/sftp.go (1)
NewSftp(44-52)
🪛 GitHub Check: codecov/patch
cmd/syncsftp.go
[warning] 16-21: cmd/syncsftp.go#L16-L21
Added lines #L16 - L21 were not covered by tests
[warning] 23-26: cmd/syncsftp.go#L23-L26
Added lines #L23 - L26 were not covered by tests
[warning] 29-31: cmd/syncsftp.go#L29-L31
Added lines #L29 - L31 were not covered by tests
[warning] 33-39: cmd/syncsftp.go#L33-L39
Added lines #L33 - L39 were not covered by tests
[warning] 41-44: cmd/syncsftp.go#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 47-53: cmd/syncsftp.go#L47-L53
Added lines #L47 - L53 were not covered by tests
[warning] 55-59: cmd/syncsftp.go#L55-L59
Added lines #L55 - L59 were not covered by tests
[warning] 62-62: cmd/syncsftp.go#L62
Added line #L62 was not covered by tests
fileutil/checksum.go
[warning] 30-31: fileutil/checksum.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 36-37: fileutil/checksum.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 41-44: fileutil/checksum.go#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 46-50: fileutil/checksum.go#L46-L50
Added lines #L46 - L50 were not covered by tests
[warning] 52-54: fileutil/checksum.go#L52-L54
Added lines #L52 - L54 were not covered by tests
[warning] 57-61: fileutil/checksum.go#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 64-68: fileutil/checksum.go#L64-L68
Added lines #L64 - L68 were not covered by tests
[warning] 70-73: fileutil/checksum.go#L70-L73
Added lines #L70 - L73 were not covered by tests
[warning] 77-82: fileutil/checksum.go#L77-L82
Added lines #L77 - L82 were not covered by tests
[warning] 85-87: fileutil/checksum.go#L85-L87
Added lines #L85 - L87 were not covered by tests
[warning] 89-89: fileutil/checksum.go#L89
Added line #L89 was not covered by tests
[warning] 92-97: fileutil/checksum.go#L92-L97
Added lines #L92 - L97 were not covered by tests
[warning] 99-102: fileutil/checksum.go#L99-L102
Added lines #L99 - L102 were not covered by tests
[warning] 105-108: fileutil/checksum.go#L105-L108
Added lines #L105 - L108 were not covered by tests
[warning] 110-113: fileutil/checksum.go#L110-L113
Added lines #L110 - L113 were not covered by tests
[warning] 115-119: fileutil/checksum.go#L115-L119
Added lines #L115 - L119 were not covered by tests
[warning] 121-123: fileutil/checksum.go#L121-L123
Added lines #L121 - L123 were not covered by tests
[warning] 125-125: fileutil/checksum.go#L125
Added line #L125 was not covered by tests
🪛 golangci-lint (1.64.8)
cmd/root.go
145-145: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
146-146: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
147-147: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
🔇 Additional comments (3)
cmd/syncsftp.go (1)
35-45: Add tests for checksum logicThe code checking if the file is already transferred is untested. This logic is important for preventing redundant uploads, so consider adding tests to validate this flow—particularly verifying that the command exits early when the file has already been transferred.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-44: cmd/syncsftp.go#L41-L44
Added lines #L41 - L44 were not covered by testscmd/root.go (1)
145-149: Handle error returns for MarkFlagRequiredThis mirrors a previous review suggestion. It's good practice to check errors returned by
MarkFlagRequired, even though failures are rare, to avoid unexpected scenarios going unnoticed.-if err := syncSftpCmd.MarkFlagRequired("source"); err != nil { - // handle error -} +if err := syncSftpCmd.MarkFlagRequired("source"); err != nil { + // log or handle error +} // ... repeat for other flags ...🧰 Tools
🪛 golangci-lint (1.64.8)
145-145: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
146-146: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
147-147: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
fileutil/checksum.go (1)
26-34: Potential issue when calling computeChecksum multiple times.
The file pointer advances during checksum computation, so subsequent calls without seeking the file pointer back to the beginning may fail or yield incorrect results. Confirm the intended usage or perform aSeek(0, 0)before each call, if necessary.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 30-31: fileutil/checksum.go#L30-L31
Added lines #L30 - L31 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
dumper/mysqlnativedump.go (1)
352-352: Consider adding test coverage for error handling.While changing the message from "faile" to "fail" is correct and improves clarity, this new line is flagged as uncovered by tests. Consider adding a negative test case (e.g., simulating an error scenario) to ensure the error path is tested and to improve overall test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 352-352: dumper/mysqlnativedump.go#L352
Added line #L352 was not covered by testscmd/syncsftp.go (5)
13-16: New Cobra command setup forsync sftp.The command approach is consistent with Cobra's design. Ensure the command usage string remains in sync with any future environment changes or added flags.
17-21: Error reporting on file opening is robust.Providing a clear error message with the filename and underlying error helps diagnose file access issues. Though static analysis shows uncovered lines, consider writing tests to verify error handling (e.g., open a non-existent file).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-21: cmd/syncsftp.go#L16-L21
Added lines #L16 - L21 were not covered by tests
29-31: Conditionally enabling verbose logging.This snippet sets logging to debug if
verboseis enabled. Ensure that toggling verbose logging multiple times doesn't create confusion or repeated log messages in extended flows.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-31: cmd/syncsftp.go#L29-L31
Added lines #L29 - L31 were not covered by tests
33-45: Checksum logic to prevent redundant transfers looks good.Creating a checksum instance and checking if the file has already been transferred can save resources. Consider documenting potential edge cases where partial transfers might still appear as incomplete from a user perspective.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-39: cmd/syncsftp.go#L33-L39
Added lines #L33 - L39 were not covered by tests
[warning] 41-44: cmd/syncsftp.go#L41-L44
Added lines #L41 - L44 were not covered by tests
62-62: Command completes successfully with no additional cleanup.Returning
nilon success is standard. Ensure that subsequent steps (e.g., logging final messages) are not needed before returning.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: cmd/syncsftp.go#L62
Added line #L62 was not covered by testshandler/jobhandler_test.go (1)
80-83: Consider using testing logger instead of fmt.PrintPrint statements should use the testing logger for better integration with the test framework.
- fmt.Print("start doing job") + t.Log("start doing job") NewJobHandler(job).Do() - fmt.Print("Finished") + t.Log("Finished")fileutil/checksum.go (2)
84-85: Fix typo in error messageThere's a typo in the error message: "transfered" should be "transferred".
- slog.Error("fail to close the checksum state file while checking if file has been transfered", slog.Any("error", err)) + slog.Error("fail to close the checksum state file while checking if file has been transferred", slog.Any("error", err))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 82-85: fileutil/checksum.go#L82-L85
Added lines #L82 - L85 were not covered by tests
89-89: Clarify comment about checksum sizeThe comment "Checksum is always 64 bits" is technically incorrect. SHA-256 produces a 256-bit (32-byte) hash, which results in a 64-character hex string.
- // Checksum is always 64 bits, so use bufio scanner is simple and safe for this case. + // SHA-256 checksum is 64 hexadecimal characters (representing 256 bits), so using bufio scanner is simple and safe for this case.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-94: fileutil/checksum.go#L89-L94
Added lines #L89 - L94 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/root_test.go(2 hunks)cmd/syncsftp.go(1 hunks)dumper/mysqlnativedump.go(3 hunks)dumper/runner/ssh.go(1 hunks)fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)handler/dumphandler.go(3 hunks)handler/jobhandler.go(2 hunks)handler/jobhandler_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- dumper/runner/ssh.go
🚧 Files skipped from review as they are similar to previous changes (1)
- fileutil/checksum_test.go
🧰 Additional context used
🧬 Code Definitions (4)
handler/dumphandler.go (1)
handler/jobhandler.go (1)
NewJobHandler(25-29)
cmd/syncsftp.go (2)
fileutil/checksum.go (1)
NewChecksum(22-24)storage/sftp/sftp.go (1)
NewSftp(44-52)
handler/jobhandler_test.go (2)
config/job.go (5)
NewJob(94-106)WithSshHost(64-68)WithSshUser(70-74)WithSshKey(88-92)Dump(23-28)handler/jobhandler.go (1)
NewJobHandler(25-29)
cmd/root_test.go (1)
cmd/root.go (1)
Execute(100-104)
🪛 GitHub Check: codecov/patch
dumper/mysqlnativedump.go
[warning] 352-352: dumper/mysqlnativedump.go#L352
Added line #L352 was not covered by tests
[warning] 391-391: dumper/mysqlnativedump.go#L391
Added line #L391 was not covered by tests
cmd/syncsftp.go
[warning] 16-21: cmd/syncsftp.go#L16-L21
Added lines #L16 - L21 were not covered by tests
[warning] 23-26: cmd/syncsftp.go#L23-L26
Added lines #L23 - L26 were not covered by tests
[warning] 29-31: cmd/syncsftp.go#L29-L31
Added lines #L29 - L31 were not covered by tests
[warning] 33-39: cmd/syncsftp.go#L33-L39
Added lines #L33 - L39 were not covered by tests
[warning] 41-44: cmd/syncsftp.go#L41-L44
Added lines #L41 - L44 were not covered by tests
[warning] 47-53: cmd/syncsftp.go#L47-L53
Added lines #L47 - L53 were not covered by tests
[warning] 55-59: cmd/syncsftp.go#L55-L59
Added lines #L55 - L59 were not covered by tests
[warning] 62-62: cmd/syncsftp.go#L62
Added line #L62 was not covered by tests
fileutil/checksum.go
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 48-49: fileutil/checksum.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 53-56: fileutil/checksum.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-62: fileutil/checksum.go#L58-L62
Added lines #L58 - L62 were not covered by tests
[warning] 64-66: fileutil/checksum.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 69-73: fileutil/checksum.go#L69-L73
Added lines #L69 - L73 were not covered by tests
[warning] 76-80: fileutil/checksum.go#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: fileutil/checksum.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 89-94: fileutil/checksum.go#L89-L94
Added lines #L89 - L94 were not covered by tests
[warning] 97-99: fileutil/checksum.go#L97-L99
Added lines #L97 - L99 were not covered by tests
[warning] 101-101: fileutil/checksum.go#L101
Added line #L101 was not covered by tests
[warning] 104-109: fileutil/checksum.go#L104-L109
Added lines #L104 - L109 were not covered by tests
[warning] 111-114: fileutil/checksum.go#L111-L114
Added lines #L111 - L114 were not covered by tests
[warning] 117-120: fileutil/checksum.go#L117-L120
Added lines #L117 - L120 were not covered by tests
[warning] 122-125: fileutil/checksum.go#L122-L125
Added lines #L122 - L125 were not covered by tests
🪛 golangci-lint (1.64.8)
handler/jobhandler_test.go
115-115: Error return value of req.Reply is not checked
(errcheck)
cmd/root_test.go
65-65: Error return value of cmd.Execute is not checked
(errcheck)
🔇 Additional comments (25)
dumper/mysqlnativedump.go (3)
13-13: Importingtimeseems appropriate.Using
timealigns with the newly introducedcontext.WithTimeoutusage. No issues found here.
391-391: Improved error handling by returning errors instead of causing an immediate exit.Replacing
log.Fatal(err)with this descriptive error message promotes graceful error handling, which is preferred in libraries. However, this line is not covered by tests according to static analysis. To strengthen reliability, add a test that triggers a failure to open the database (e.g., by providing invalid credentials), and verify that the returned error message matches expectations.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 391-391: dumper/mysqlnativedump.go#L391
Added line #L391 was not covered by tests
394-396: Context-based ping for better timeout management.Using
PingContextwith a configurable timeout helps avoid indefinite blocking when the database is unreachable. Ensure the timeout value of 3 seconds is appropriate for production environments; consider making it configurable if different latencies are expected.handler/dumphandler.go (5)
20-20: Mutex field addition enhances concurrency safety.Introducing
mu sync.Mutexis a good step toward protecting shared state. Make sure all shared mutable fields inDumpHandlerare locked appropriately to avoid data races.
38-39: Deferredwg.Done()ensures the wait group is decremented even on error.This pattern is safer and less prone to missing
wg.Done()calls if early returns occur. Great job enhancing reliability.
42-42: No immediate issue with callingjobHandler.Do().Confirm that concurrent invocations of
JobHandler.Do()won't lead to shared resource conflicts insidejobHandler.
43-49: Locking around result handling synchronizes shared state modifications.Using
d.mu.Lock()andd.mu.Unlock()ensures that appending toresultsand updatingdumpErrremain thread-safe. This is a proper usage of the mutex. No further concerns here.
73-75: Thread-safety in notification handling.Locking around error accumulation ensures concurrency safety, preventing inadvertent data races when multiple goroutines attempt to update
errsimultaneously. Nicely done.cmd/syncsftp.go (3)
1-11: New package and imports for SFTP command.Introducing a dedicated
syncsftp.gofile and referencing appropriate packages (slog,os,sftp) is consistent with the new SFTP sync functionality. No immediate concerns here.
23-27: Properly closing the file even on errors.Deferring
Close()inside an anonymous function with error logging is a good pattern. Once again, consider adding negative tests to confirm the error path is exercised.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-26: cmd/syncsftp.go#L23-L26
Added lines #L23 - L26 were not covered by tests
55-60: Persisting checksum state once the transfer succeeds.Persisting the state ensures subsequent runs don’t re-upload. As with other lines, tests for partial or failed writes may further strengthen confidence in the approach.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-59: cmd/syncsftp.go#L55-L59
Added lines #L55 - L59 were not covered by testshandler/jobhandler.go (5)
59-65: Refactored error handling with buffered channelThe move from a single error variable to a buffered error channel is a good improvement for handling multiple concurrent errors. The buffer size of
numberOfStorages+1correctly accounts for all possible error sources (storage operations plus the dump operation).
76-93: Improved concurrency with goroutine sequencingThe implementation now properly sequences operations in the dump goroutine by calling
dumpWg.Done()beforecloser.Close(). This prevents a potential deadlock situation where readers would be waiting for EOF while the main goroutine waits for the dump to complete.
95-117: Better goroutine management with separate wait groupsUsing separate wait groups for dump and read operations allows for better coordination between them. The goroutine that closes the error channel ensures all errors are collected before processing continues.
119-127: Robust error aggregation with errors.JoinUsing
errors.Jointo combine multiple errors provides clearer error reporting than the previous approach. This preserves all error information for better debugging.
130-130: Explicit nil return on successExplicitly returning nil when no errors occur makes the code more readable and intention-clear.
cmd/root_test.go (2)
46-46: Updated cron time formatThe change from
"-c", "1sec"to"-c", "1s"aligns with standard duration format in Go, which is more widely used and understood.
77-120: Comprehensive test cases for file handlingThe second subtest covers the scenario when no job is defined in the config file, with proper setup and assertions for expected error messages.
handler/jobhandler_test.go (3)
39-39: Updated SSH port for consistencyThe SSH host has been changed from port 20001 to 20002, ensuring consistency with the listener port change on line 74.
74-77: Matching TCP listener configurationThe TCP listener has been updated to listen on the same port as the SSH job configuration, which ensures the test setup is consistent.
Making
finishCha buffered channel with capacity equal to the number of jobs prevents potential deadlocks when multiple jobs complete.
117-131: Improved error handling for SSH operationsGood addition of error handling for SSH channel operations (Write, SendRequest, Close), which makes the test more robust.
fileutil/checksum.go (4)
15-24: Well-structured checksum implementationThe Checksum struct and constructor are clear and follow good design principles. The state file constant is appropriately named and the struct fields are minimalist.
26-46: Thorough error handling in computeChecksum methodThe implementation includes proper error handling for file operations and good use of defer for file closing.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
48-67: Efficient checksum cachingThe getChecksum method efficiently caches the computed checksum to avoid redundant calculations, with proper error handling and propagation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-49: fileutil/checksum.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 53-56: fileutil/checksum.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-62: fileutil/checksum.go#L58-L62
Added lines #L58 - L62 were not covered by tests
[warning] 64-66: fileutil/checksum.go#L64-L66
Added lines #L64 - L66 were not covered by tests
104-138:❓ Verification inconclusive
Robust state file management
The SaveState method handles file operations well, including checking if the file exists, appending properly, and handling errors appropriately.
However, the static analysis indicates that this file lacks test coverage. Consider adding more tests to ensure reliability.
🏁 Script executed:
#!/bin/bash # Check for test coverage of the checksum package echo "Looking for checksum related tests..." fd -t f "checksum_test.go" echo "Showing test coverage..." cd fileutil && go test -cover ./... | grep "checksum"Length of output: 444
Action: Verify and Enhance Test Coverage for Robust File Handling
The
SaveStatemethod infileutil/checksum.gois implemented well with careful error handling and proper file operations. We noted that a corresponding test file (fileutil/checksum_test.go) exists, which is a positive sign. However, the static analysis still flagged potential gaps in specific test coverage forSaveState. Please review the current tests to ensure they thoroughly cover all edge cases of state file management, and consider adding additional tests if necessary.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-109: fileutil/checksum.go#L104-L109
Added lines #L104 - L109 were not covered by tests
[warning] 111-114: fileutil/checksum.go#L111-L114
Added lines #L111 - L114 were not covered by tests
[warning] 117-120: fileutil/checksum.go#L117-L120
Added lines #L117 - L120 were not covered by tests
[warning] 122-125: fileutil/checksum.go#L122-L125
Added lines #L122 - L125 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
handler/jobhandler_test.go (1)
112-112:⚠️ Potential issueMissing error handling for request reply.
The error return value from
req.Replyis not being checked.Apply this fix:
-req.Reply(true, nil) +if err := req.Reply(true, nil); err != nil { + t.Logf("Failed to reply to request: %v", err) +}🧰 Tools
🪛 golangci-lint (1.64.8)
112-112: Error return value of
req.Replyis not checked(errcheck)
🧹 Nitpick comments (14)
cmd/syncsftp_test.go (3)
88-88: Assertion doesn't provide meaningful validation.The current assertion always passes since every string contains an empty string.
Consider replacing with a more specific assertion, such as:
-assert.Contains(readWriter.String(), "") +assert.NotContains(readWriter.String(), "error")Or check for a specific success message if one is expected.
150-150: Assertion doesn't provide meaningful validation.Similar to the previous test case, this assertion will always pass.
Consider replacing with a more specific assertion, such as:
-assert.Contains(readWriter.String(), "") +assert.NotContains(readWriter.String(), "error")Or check for a specific success message if one is expected.
92-92: Consider adding validation of the transferred file content.The test creates a file with specific content but doesn't verify if the content was correctly transferred.
Consider adding validation that the destination file contains the expected content:
// After the command is executed destContent, err := os.ReadFile(destPath) assert.Nil(err) assert.Equal("source content", string(destContent))cmd/syncsftp.go (3)
132-133: Incomplete error message.The error message doesn't include the actual error details.
Include the original error for better debugging:
-return fmt.Errorf("fail to get source info") +return fmt.Errorf("fail to get source info: %w", err)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 132-133: cmd/syncsftp.go#L132-L133
Added lines #L132 - L133 were not covered by tests
52-108: Consider adding a progress indicator for large file transfers.For large files, it would be helpful to show progress during transfer.
Consider adding an optional progress indicator by:
- Implementing a progress reader that wraps the source file
- Periodically logging progress percentage
- Using a progress bar library for CLI visualization
This would enhance user experience during lengthy transfers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-63: cmd/syncsftp.go#L57-L63
Added lines #L57 - L63 were not covered by tests
[warning] 65-68: cmd/syncsftp.go#L65-L68
Added lines #L65 - L68 were not covered by tests
[warning] 74-75: cmd/syncsftp.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: cmd/syncsftp.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 87-88: cmd/syncsftp.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 101-104: cmd/syncsftp.go#L101-L104
Added lines #L101 - L104 were not covered by tests
118-123: Consider adding validation for required configuration parameters.There's no validation that required parameters like host, user, and key are provided.
Add validation before creating the SFTP client:
if config.Host == "" || config.User == "" || config.Key == "" { return errors.New("ssh host, user, and key are required for SFTP connection") }fileutil/checksum.go (1)
1-153: Add test coverage for error-handling paths.Several lines are not covered by tests (e.g., lines handling file-opening errors, scanning errors, closing errors). Increasing test coverage for these scenarios will improve reliability and help detect regressions.
Would you like help generating additional tests to cover these error paths?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 53-56: fileutil/checksum.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-62: fileutil/checksum.go#L58-L62
Added lines #L58 - L62 were not covered by tests
[warning] 64-66: fileutil/checksum.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 69-73: fileutil/checksum.go#L69-L73
Added lines #L69 - L73 were not covered by tests
[warning] 76-80: fileutil/checksum.go#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: fileutil/checksum.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 89-94: fileutil/checksum.go#L89-L94
Added lines #L89 - L94 were not covered by tests
[warning] 97-99: fileutil/checksum.go#L97-L99
Added lines #L97 - L99 were not covered by testsstorage/sftp/sftp.go (7)
33-36: Fix typo in struct name.There's a typo in the struct name
SftpConifg(should beSftpConfig). This is important to fix since it's part of the public API.-type SftpConifg struct { +type SftpConfig struct { Host, User, Key string MaxAttempts int }Don't forget to update the corresponding function signature on line 49 as well.
49-56: Update function signature to match struct name fix.The constructor should use the corrected struct name.
-func NewSftp(config *SftpConifg) *Sftp { +func NewSftp(config *SftpConfig) *Sftp { return &Sftp{ SshHost: config.Host, SshUser: config.User, SshKey: config.Key, MaxAttempts: config.MaxAttempts, } }
131-134: Enhance error message for SFTP client creation failure.When SFTP client creation fails, the error is returned without additional context. This could make debugging harder.
client, err := sftpdialer.NewClient(conn) if err != nil { - return err + return fmt.Errorf("failed to create SFTP client: %w", err) }
90-174: Reduce code duplication for SFTP connection management.Both the
writeandIsPathDirmethods contain very similar code for establishing SSH and SFTP connections. Consider extracting this into a helper method to reduce duplication and improve maintainability.You could create a helper method like:
// createSftpClient establishes an SSH connection and returns an SFTP client func (sf *Sftp) createSftpClient() (*sftpdialer.Client, func(), error) { conn, err := dialer.NewSsh(sf.SshHost, sf.SshKey, sf.SshUser).CreateSshClient() if err != nil { return nil, nil, fmt.Errorf("failed to create SSH connection: %w", err) } // Create cleanup function for SSH connection sshCleanup := func() { if err := conn.Close(); err != nil { slog.Error("failed to close SSH connection", slog.Any("error", err)) } } client, err := sftpdialer.NewClient(conn) if err != nil { // Make sure to clean up SSH connection if SFTP client creation fails sshCleanup() return nil, nil, fmt.Errorf("failed to create SFTP client: %w", err) } // Combined cleanup function for both resources cleanup := func() { if err := client.Close(); err != nil { slog.Error("failed to close SFTP connection", slog.Any("error", err)) } sshCleanup() } return client, cleanup, nil }Then you can simplify both methods to use this helper.
Also applies to: 208-242
122-122: Standardize error message formatting.Error messages use different formats: some use "fail to" while others use "failed to". Standardizing these would improve consistency.
Consider using "failed to" consistently throughout the code, as it's more grammatically correct:
- return fmt.Errorf("fail to create ssh connection, error: %v", err) + return fmt.Errorf("failed to create ssh connection: %v", err)Apply similar changes to other error messages throughout the file.
Also applies to: 153-153, 157-157, 212-212, 228-228, 238-238
38-47: Document concurrency safety for the Sftp struct.The
Sftpstruct uses mutexes to protect internal state, but it's not clear if concurrent calls toSaveare supported. If multiple goroutines callSaveconcurrently, thereset()at the beginning could interfere with ongoing transfer attempts.Consider adding documentation that clarifies the concurrency guarantees:
type Sftp struct { mu sync.Mutex written int64 // number of bytes that have been written to the remote file attempts int MaxAttempts int // by default it is 0, infinite retries Path string `yaml:"path"` SshHost string `yaml:"sshhost"` SshUser string `yaml:"sshuser"` SshKey string `yaml:"sshkey"` + // Note: Sftp methods are not safe for concurrent use. Do not call Save from + // multiple goroutines on the same Sftp instance. }Alternatively, you could enhance the implementation to support concurrent usage if that's a requirement.
Also applies to: 176-206
179-205: Consider using context for cancellation support.The retry loop in the
Savemethod lacks context support for cancellation. This means there's no clean way to stop an ongoing upload operation.Consider accepting a context parameter that can be used to cancel the operation:
-func (sf *Sftp) Save(reader io.Reader, pathGenerator storage.PathGeneratorFunc) error { +func (sf *Sftp) Save(ctx context.Context, reader io.Reader, pathGenerator storage.PathGeneratorFunc) error { sf.reset() for { + // Check if context is canceled + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + err := sf.write(reader, pathGenerator, sf.written) // Contents have been saved properly, just return if err == nil { return nil } // ... rest of the method ... delay := time.Duration(math.Min( float64(BaseDelay*(1<<sf.attempts)), float64(1*time.Minute), )) slog.Debug(fmt.Sprintf("retry after %0.f seconds", delay.Seconds())) + // Use time.NewTimer and select to support cancellation during delay + timer := time.NewTimer(delay) + select { + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-timer.C: + } - time.Sleep(delay) sf.attempt() slog.Debug("retrying upload", slog.Int("attempt", sf.attempts), slog.Any("error", err)) } }This would provide a way to gracefully cancel long-running upload operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/tests.yaml(1 hunks)cmd/root.go(2 hunks)cmd/syncsftp.go(1 hunks)cmd/syncsftp_test.go(1 hunks)config/closer.go(2 hunks)config/job.go(3 hunks)dumper/mysqldump.go(4 hunks)dumper/pgdump.go(2 hunks)fileutil/checksum.go(1 hunks)go.mod(2 hunks)handler/dumphandler.go(4 hunks)handler/jobhandler_test.go(3 hunks)storage/sftp/sftp.go(1 hunks)testutils/sftp.go(1 hunks)testutils/ssh.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- testutils/ssh.go
🚧 Files skipped from review as they are similar to previous changes (2)
- config/job.go
- go.mod
🧰 Additional context used
🧬 Code Definitions (5)
cmd/syncsftp.go (2)
storage/sftp/sftp.go (2)
SftpConifg(33-36)NewSftp(49-56)fileutil/checksum.go (1)
NewChecksum(22-24)
cmd/syncsftp_test.go (2)
testutils/ssh.go (1)
GenerateRSAPrivateKey(12-26)testutils/sftp.go (1)
StartSftpServer(102-175)
handler/dumphandler.go (2)
handler/jobhandler.go (1)
NewJobHandler(25-29)jobresult/jobresult.go (1)
JobResult(8-12)
storage/sftp/sftp.go (1)
dumper/dialer/ssh.go (1)
NewSsh(18-24)
handler/jobhandler_test.go (1)
config/job.go (4)
NewJob(92-104)WithSshHost(62-66)WithSshUser(68-72)WithSshKey(86-90)
🪛 GitHub Check: codecov/patch
config/closer.go
[warning] 22-22: config/closer.go#L22
Added line #L22 was not covered by tests
cmd/syncsftp.go
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 57-63: cmd/syncsftp.go#L57-L63
Added lines #L57 - L63 were not covered by tests
[warning] 65-68: cmd/syncsftp.go#L65-L68
Added lines #L65 - L68 were not covered by tests
[warning] 74-75: cmd/syncsftp.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: cmd/syncsftp.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 87-88: cmd/syncsftp.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 101-104: cmd/syncsftp.go#L101-L104
Added lines #L101 - L104 were not covered by tests
[warning] 127-128: cmd/syncsftp.go#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 132-133: cmd/syncsftp.go#L132-L133
Added lines #L132 - L133 were not covered by tests
[warning] 142-143: cmd/syncsftp.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 146-147: cmd/syncsftp.go#L146-L147
Added lines #L146 - L147 were not covered by tests
fileutil/checksum.go
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 53-56: fileutil/checksum.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 58-62: fileutil/checksum.go#L58-L62
Added lines #L58 - L62 were not covered by tests
[warning] 64-66: fileutil/checksum.go#L64-L66
Added lines #L64 - L66 were not covered by tests
[warning] 69-73: fileutil/checksum.go#L69-L73
Added lines #L69 - L73 were not covered by tests
[warning] 76-80: fileutil/checksum.go#L76-L80
Added lines #L76 - L80 were not covered by tests
[warning] 82-85: fileutil/checksum.go#L82-L85
Added lines #L82 - L85 were not covered by tests
[warning] 89-94: fileutil/checksum.go#L89-L94
Added lines #L89 - L94 were not covered by tests
[warning] 97-99: fileutil/checksum.go#L97-L99
Added lines #L97 - L99 were not covered by tests
🪛 golangci-lint (1.64.8)
testutils/sftp.go
153-153: Error return value of newChannel.Reject is not checked
(errcheck)
180-180: Error return value of req.Reply is not checked
(errcheck)
196-196: Error return value of req.Reply is not checked
(errcheck)
cmd/root.go
146-146: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
handler/jobhandler_test.go
112-112: Error return value of req.Reply is not checked
(errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (23)
.github/workflows/tests.yaml (1)
13-13: Bump Go version patch level.Updating from Go 1.24.0 to 1.24.1 generally addresses minor bug fixes. This looks good for maintaining the latest stable environment.
config/closer.go (1)
4-4: Switch to standard library errors package.This replacement aligns with the modern errors handling approach in Go. Good move away from external dependencies.
dumper/pgdump.go (2)
4-4: Importing the standard library errors package.Replacing external error accumulation with
errors.Joinhelps maintain consistency and reduces dependencies.
92-103: Aggregate cleanup errors consistently.Using
errors.Joinhere ensures that multiple file-removal errors are captured without prematurely stopping the cleanup process. This is a robust approach for partial successes in credential file removals.handler/dumphandler.go (8)
4-4: Use of standard library errors.Importing
"errors"allows you to leverage native error-joining functionality, which is cleaner than external dependencies.
19-19: Mutex addition for concurrency control.Introducing
mu sync.Mutexensures thread-safe operations on shared state. This is an important step toward reliable concurrent dumps.
37-38: Deferring the WaitGroup decrement.Calling
defer wg.Done()at the beginning of the goroutine is a good practice to avoid missing it due to early returns or panics.
42-48: Locking around shared state.Locking is necessary when appending to the shared
resultsslice and joining errors. Ensuring mutual exclusion here prevents race conditions.
58-58: Aggregating dump errors with errors.Join.This pattern reliably accumulates multiple errors, preserving details from each one. No concerns.
65-65: Initialize a separate aggregator variable for notify.Starting with
var errs errorensures a fresh accumulator for notification errors. This helps distinguish them from dump errors.
72-74: Arrowing notifications into the aggregator.Locking is crucial when combining concurrent errors. This approach correctly synchronizes access to
errs.
81-81: Final return of accumulated errors.Returning
errsexposes all notifier errors, rather than hiding them behind a single failure. This provides improved visibility into multiple errors.dumper/mysqldump.go (2)
112-114: Added support for--all-databasesoption.This is a good enhancement to avoid appending the database name when the
--all-databasesoption is specified, which would cause a conflict in mysqldump arguments.
154-159: Updated error handling to use standard library'serrors.Join.Good modernization by replacing the previous error aggregation mechanism with Go's standard library
errors.Join. This change aligns with modern Go error handling practices introduced in Go 1.20+.Also applies to: 165-165
cmd/syncsftp_test.go (1)
16-47: Well-structured error test case for incompatible source/destination.The test correctly verifies the error case when attempting to transfer multiple files to a single file destination.
handler/jobhandler_test.go (3)
38-38: Updated SSH port for testing.Port change from 20001 to 20002 helps avoid conflicts with other tests or services.
Also applies to: 73-73
76-76: Improved channel synchronization with proper buffering.Adding buffer capacity equal to the number of jobs prevents potential goroutine leaks when multiple jobs are executed.
114-128: Improved error handling for SSH channel operations.Good addition of error handling for writing to the channel, sending exit status, and closing the channel.
cmd/syncsftp.go (2)
16-50: Well-implemented concurrent file transfer with proper synchronization.The implementation uses appropriate concurrency patterns:
- Semaphore to limit concurrent transfers
- WaitGroup for synchronization
- Error channel to collect errors from multiple goroutines
- Proper error aggregation with
errors.Join🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by tests
1-154:❓ Verification inconclusive
Several error paths not covered by tests according to code coverage report.
The static analysis shows that several error paths are not covered by tests.
Consider adding tests for these scenarios:
- Error handling for checksum operations (lines 57-63)
- File already transferred case (lines 65-68)
- Source file opening errors (lines 74-75)
- File stat errors (lines 87-88)
- SFTP save errors (lines 97-98)
- Checksum state saving errors (lines 101-104)
- Destination directory checking errors (lines 127-128)
- Source info errors (lines 132-133)
- Directory listing errors (lines 142-143)
- No files found case (lines 146-147)
🏁 Script executed:
#!/bin/bash # Check if the tests are handling error conditions # Look for error handling tests that might exercise these paths echo "Looking for error condition tests in sync SFTP tests:" rg -A 5 -B 5 "it should.*error|fail|invalid" cmd/syncsftp_test.go echo "------" # Check if there are any test helper functions that mock failure conditions rg -A 5 -B 5 "func.*Mock.*Error|func.*Fail" cmd/Length of output: 256
Action Required: Enhance Test Coverage for Error Paths
The static analysis indicates that multiple error-handling scenarios in
cmd/syncsftp.gostill lack test coverage. In particular, please consider adding tests for the following cases:
- Checksum operations failure: (lines 57–63)
- File already transferred scenario: (lines 65–68)
- Source file open errors: (lines 74–75)
- File stat failures: (lines 87–88)
- SFTP save errors: (lines 97–98)
- Checksum state saving failures: (lines 101–104)
- Destination directory check errors: (lines 127–128)
- Source info retrieval errors: (lines 132–133)
- Directory listing failures: (lines 142–143)
- No files found in a directory: (lines 146–147)
The recent script execution did not reveal any tests in
cmd/syncsftp_test.go(or elsewhere) that simulate these failure scenarios. Please add the necessary tests—either by extending existing test files or by creating new ones with appropriate mocks—to ensure that all error paths are robustly covered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 57-63: cmd/syncsftp.go#L57-L63
Added lines #L57 - L63 were not covered by tests
[warning] 65-68: cmd/syncsftp.go#L65-L68
Added lines #L65 - L68 were not covered by tests
[warning] 74-75: cmd/syncsftp.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: cmd/syncsftp.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 87-88: cmd/syncsftp.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 101-104: cmd/syncsftp.go#L101-L104
Added lines #L101 - L104 were not covered by tests
[warning] 127-128: cmd/syncsftp.go#L127-L128
Added lines #L127 - L128 were not covered by tests
[warning] 132-133: cmd/syncsftp.go#L132-L133
Added lines #L132 - L133 were not covered by tests
[warning] 142-143: cmd/syncsftp.go#L142-L143
Added lines #L142 - L143 were not covered by tests
[warning] 146-147: cmd/syncsftp.go#L146-L147
Added lines #L146 - L147 were not covered by testscmd/root.go (1)
146-150: Consider handling errors returned byMarkFlagRequired.This mirrors a previous comment suggesting you check the returned errors to capture potential failures.
🧰 Tools
🪛 golangci-lint (1.64.8)
146-146: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
storage/sftp/sftp.go (2)
114-116: Check for partial writes.Within your
writemethod, you handle offset logic for resumed files. Consider verifying partial writes at each step. For locally large files or network instabilities, you might want to confirm that offset is truly at the correct position before resuming.
167-204: Add test coverage to the retry logic in 'Save'.The complex loop with exponential backoff is critical—verifying it works as intended with simulated failures (e.g., ephemeral network errors) can prevent production issues. This is also flagged by code coverage checks.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (14)
config/closer.go (1)
15-15: Unnecessary trailing whitespace.There's an extra tab after the closing brace.
- } + }fileutil/checksum.go (3)
17-20: Fix unusedchecksumfield.The
checksumfield is declared but never used in any of the struct's methods. ThecomputeChecksum()method always calculates the checksum directly without caching it.Either:
- Remove the unused field:
type Checksum struct { filePath string - checksum string }
- Or improve performance by caching the computed checksum:
func (c *Checksum) computeChecksum() (string, error) { // If already computed, return cached value + if c.checksum != "" { + return c.checksum, nil + } hasher := sha256.New() // ... existing code ... - return hex.EncodeToString(hasher.Sum(nil)), nil + c.checksum = hex.EncodeToString(hasher.Sum(nil)) + return c.checksum, nil }🧰 Tools
🪛 golangci-lint (1.64.8)
19-19: field
checksumis unused(unused)
35-39: Consider using named return parameters for deferred close.The current implementation captures
errfrom the outer scope, which could overwrite the function's return error.-func (c *Checksum) computeChecksum() (string, error) { +func (c *Checksum) computeChecksum() (string, error) { hasher := sha256.New() file, err := os.Open(c.filePath) if err != nil { return "", fmt.Errorf("fail to open file %s to compute checksum, error: %v", c.filePath, err) } defer func() { - if err = file.Close(); err != nil { + if closeErr := file.Close(); closeErr != nil { slog.Error("fail to close file", slog.Any("filename", file.Name()), slog.Any("error", err)) } }() // ... }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
67-67: Fix typo in error message: "transfered" -> "transferred".There's a spelling mistake in the error message.
- slog.Error("fail to close the checksum state file while checking if file has been transfered", slog.Any("error", err)) + slog.Error("fail to close the checksum state file while checking if file has been transferred", slog.Any("error", err))🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-68: fileutil/checksum.go#L67-L68
Added lines #L67 - L68 were not covered by testscmd/syncsftp.go (4)
26-37: Use anonymous function parameters for clarity.The current anonymous function doesn't explicitly declare the
fileparameter. This could lead to confusion when reading the code.- for _, file := range sources { + for _, sourceFile := range sources { wg.Add(1) semaphore <- struct{}{} - go func() { + go func(file string) { defer func() { <-semaphore wg.Done() }() err := syncFile(file, destination, checksum, isDestinationDir, config) if err != nil { errCh <- err } - }() + }(sourceFile) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
65-68: Fix typo in debug log: "transfered" -> "transferred".There's a spelling mistake in the debug log message.
- slog.Debug("the file has already been transfered", slog.Any("filename", source)) + slog.Debug("the file has already been transferred", slog.Any("filename", source))
140-140: Fix typo in error message: "detination" -> "destination".There's a spelling mistake in the error message.
- return errors.New("detination should not be a file when transfer multiple files from the source") + return errors.New("destination should not be a file when transfer multiple files from the source")
16-50: Consider adding context support for graceful cancellation.The current implementation doesn't support cancellation through context, which could be important for long-running operations.
-func syncFiles(sources []string, destination string, checksum bool, isDestinationDir bool, config *sftp.SftpConifg) error { +func syncFiles(ctx context.Context, sources []string, destination string, checksum bool, isDestinationDir bool, config *sftp.SftpConifg) error { errCh := make(chan error, len(sources)) // Process maximum 10 files at a time semaphore := make(chan struct{}, 10) var wg sync.WaitGroup for _, file := range sources { wg.Add(1) semaphore <- struct{}{} go func(sourceFile string) { defer func() { <-semaphore wg.Done() }() + select { + case <-ctx.Done(): + errCh <- ctx.Err() + return + default: + } - err := syncFile(file, destination, checksum, isDestinationDir, config) + err := syncFile(ctx, sourceFile, destination, checksum, isDestinationDir, config) if err != nil { errCh <- err } }(file) } // Rest of function remains the sameThen update the function signature and call in the
RunEfunction:-return syncFiles(files, destination, checksum, isDestinationDir, config) +return syncFiles(cmd.Context(), files, destination, checksum, isDestinationDir, config)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by testscmd/syncsftp_test.go (3)
41-41: Update error expectation to match corrected error message.If you fix the typo in the error message as suggested in syncsftp.go, you'll need to update the test expectation as well.
- assert.Contains(readWriter.String(), "Error: detination should not be a file when transfer multiple files from the source") + assert.Contains(readWriter.String(), "Error: destination should not be a file when transfer multiple files from the source")
85-85: Use more explicit assertion for command output.The current test is checking for an empty string but doesn't clearly indicate that this means "no errors occurred". Consider a more explicit assertion.
- assert.Equal(strings.ReplaceAll(readWriter.String(), "\x00", ""), "") + // Check for no error output which indicates successful execution + cleanOutput := strings.ReplaceAll(readWriter.String(), "\x00", "") + assert.Empty(cleanOutput, "Expected no error output from successful command execution")
196-199: Add more robust checksum file verification.The test only checks the size of the checksum file but doesn't verify its content. Consider adding a more robust verification.
checksumState, err := os.Stat(filepath.Join(wd, fileutil.ChecksumStateFile)) assert.Nil(err) assert.Equal(checksumState.Size(), int64(64)) +// Verify the content of the checksum file +checksumContent, err := os.ReadFile(filepath.Join(wd, fileutil.ChecksumStateFile)) +assert.Nil(err) +assert.Len(string(checksumContent), 64, "Checksum should be 64 characters long")fileutil/checksum_test.go (1)
109-113: EnhanceTestDeleteStateto test actual deletion.The current test only checks that
DeleteStatereturns nil when the file doesn't exist, but doesn't test actual deletion of an existing file.func TestDeleteState(t *testing.T) { + assert := assert.New(t) + + // Test deleting non-existent file checksum := NewChecksum("") err := checksum.DeleteState() - assert.Nil(t, err) + assert.Nil(err) + + // Test deleting an existing file + filename := "test_delete_state.txt" + if err := createTestFile(filename, "test content"); err != nil { + t.Error(err) + } + + checksum = NewChecksum(filename) + err = checksum.SaveState() + assert.Nil(err) + + // Verify state file exists + _, err = os.Stat(checksum.getStateFilePath()) + assert.Nil(err) + + // Delete state file + err = checksum.DeleteState() + assert.Nil(err) + + // Verify state file was deleted + _, err = os.Stat(checksum.getStateFilePath()) + assert.True(os.IsNotExist(err)) + + // Clean up + if err := os.Remove(filename); err != nil { + t.Error(err) + } }testutils/sftp.go (2)
37-37: Rename struct for clarity.
SftpHanlderappears to be a misspelling of “SftpHandler.” Renaming will improve clarity and maintain consistency.- type SftpHanlder struct{} + type SftpHandler struct{}
133-133: Fix minor spelling for diagnostic clarity.“listner” is misspelled as “listener,” so updating the log message will make logs more accurate.
- slog.Error("fail to close listner", slog.Any("error", err)) + slog.Error("fail to close listener", slog.Any("error", err))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/syncsftp.go(1 hunks)cmd/syncsftp_test.go(1 hunks)config/closer.go(2 hunks)fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)testutils/sftp.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
cmd/syncsftp.go (2)
storage/sftp/sftp.go (2)
SftpConifg(33-36)NewSftp(49-56)fileutil/checksum.go (1)
NewChecksum(22-24)
cmd/syncsftp_test.go (2)
testutils/sftp.go (1)
StartSftpServer(102-183)fileutil/checksum.go (2)
ChecksumStateFile(15-15)NewChecksum(22-24)
fileutil/checksum_test.go (1)
fileutil/checksum.go (1)
NewChecksum(22-24)
testutils/sftp.go (1)
testutils/ssh.go (1)
GenerateRSAPrivateKey(12-26)
🪛 GitHub Check: codecov/patch
cmd/syncsftp.go
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 57-58: cmd/syncsftp.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 62-63: cmd/syncsftp.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 74-75: cmd/syncsftp.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: cmd/syncsftp.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 87-88: cmd/syncsftp.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 103-104: cmd/syncsftp.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 126-127: cmd/syncsftp.go#L126-L127
Added lines #L126 - L127 were not covered by tests
[warning] 131-132: cmd/syncsftp.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 136-137: cmd/syncsftp.go#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 146-147: cmd/syncsftp.go#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 150-151: cmd/syncsftp.go#L150-L151
Added lines #L150 - L151 were not covered by tests
config/closer.go
[warning] 22-22: config/closer.go#L22
Added line #L22 was not covered by tests
fileutil/checksum.go
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 55-56: fileutil/checksum.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 62-63: fileutil/checksum.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-68: fileutil/checksum.go#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 81-82: fileutil/checksum.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 95-95: fileutil/checksum.go#L95
Added line #L95 was not covered by tests
[warning] 105-106: fileutil/checksum.go#L105-L106
Added lines #L105 - L106 were not covered by tests
[warning] 110-111: fileutil/checksum.go#L110-L111
Added lines #L110 - L111 were not covered by tests
🪛 golangci-lint (1.64.8)
fileutil/checksum_test.go
105-105: ineffectual assignment to err
(ineffassign)
fileutil/checksum.go
19-19: field checksum is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (5)
config/closer.go (2)
3-6: Good choice switching to standard library error handling.Replacing the external
github.com/hashicorp/go-multierrordependency with Go's built-inerrors.Joinfunction simplifies the codebase and reduces external dependencies.
18-25: Variable rename and error handling improvement, but test coverage needed.The change from
errtoerrsbetter reflects that this variable accumulates multiple errors. Usingerrors.Joinfrom the standard library is a good practice.However, the codecov report indicates line 22 (error accumulation) is not covered by tests. This was also noted in a previous review.
Please add a test case that forces one or more closers to return an error to verify the error accumulation logic works as expected. For example:
#!/bin/bash # Check current test coverage for the error accumulation path go test -coverprofile=cover.out ./config go tool cover -html=cover.out -o coverage.html cat coverage.html | grep -A 5 -B 5 "config/closer.go.*22"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-22: config/closer.go#L22
Added line #L22 was not covered by testsfileutil/checksum.go (1)
1-136:❓ Verification inconclusive
Add unit tests for error scenarios.
Several error paths are not covered by tests according to the code coverage report. Consider adding tests for error scenarios such as file open failures, checksum computation errors, etc.
🏁 Script executed:
#!/bin/bash # Count how many error handling branches exist vs how many are tested grep -n "if err != nil" fileutil/checksum.go grep -n "t.Error(err)" fileutil/checksum_test.goLength of output: 407
Error Scenario Testing - Additional Edge Cases to Review
The updated tests in
fileutil/checksum_test.goappear to invoke error paths by triggering 9 error-reporting instances, which suggests that many of the error conditions (e.g., file open failures and checksum computation errors) are covered. That said, please ensure that the following are explicitly tested or simulated using mocks/stubs where applicable:
- Failure scenarios for opening both the primary file and the state file (e.g., non-existent file, permission issues).
- Error propagation for file operations such as copying contents, fetching file info, and writing to the state file in
SaveState.- Behavior when deferred file closures (both for the primary file and state file) produce errors, even if these errors are simply logged.
If these edge scenarios are already handled via your testing strategy, no further changes are needed. Otherwise, consider enhancing the tests to simulate these failure modes.
🧰 Tools
🪛 golangci-lint (1.64.8)
19-19: field
checksumis unused(unused)
🪛 GitHub Check: codecov/patch
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 55-56: fileutil/checksum.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 62-63: fileutil/checksum.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-68: fileutil/checksum.go#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 81-82: fileutil/checksum.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 95-95: fileutil/checksum.go#L95
Added line #L95 was not covered by tests
[warning] 105-106: fileutil/checksum.go#L105-L106
Added lines #L105 - L106 were not covered by tests
[warning] 110-111: fileutil/checksum.go#L110-L111
Added lines #L110 - L111 were not covered by testscmd/syncsftp.go (1)
93-98:❌ Incorrect review comment
Add proper error handling for SFTP client creation.
The function creates and uses an SFTP client but doesn't check if the client creation failed.
🏁 Script executed:
#!/bin/bash # Check the implementation of NewSftp to see if it can return an error grep -n "func NewSftp" storage/sftp/sftp.goLength of output: 89
SFTP Client Error Handling Not Applicable (Design Decision)
After reviewing the implementation from
storage/sftp/sftp.go(specifically the signaturefunc NewSftp(config *SftpConifg) *Sftp), it’s clear that the SFTP client creation is designed to never return an error. SinceNewSftponly returns a pointer toSftp(and not an error), adding error handling for SFTP client creation is not applicable at this point. If error handling for the client instantiation becomes necessary, the implementation ofNewSftpshould be revisited to return an error (e.g., adopting a signature like(*Sftp, error)).Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by teststestutils/sftp.go (1)
28-29: Clarify usage ofminor define it explicitly.The
minfunction call at line 28 is not defined in this file, nor is it imported from a standard library (unless Go ≥ 1.21 is used). Please ensure this function is accessible (e.g., define it or import from the correct package). Otherwise, this code may fail to compile on certain Go versions.Would you like a verification script to search for a custom
minimplementation or confirm a certain Go version in your project files?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dumper/dialer/ssh_test.go (2)
56-57: Consider validating the key content in the file test case.The test only verifies that no error occurs when parsing the file, but doesn't validate the actual key content returned. Consider adding an assertion to verify the key content as you do in the base64 test case.
59-61: Enhance base64 test with a more realistic key.The current test uses a single character 'f' which doesn't resemble a real SSH key. Consider using a more realistic key sample to better test the actual use case.
- base64Encoded := base64.StdEncoding.EncodeToString([]byte{'f'}) + // Use a small but more realistic sample key + sampleKeyData := []byte("-----BEGIN RSA PRIVATE KEY-----\nsample\n-----END RSA PRIVATE KEY-----") + base64Encoded := base64.StdEncoding.EncodeToString(sampleKeyData)🧰 Tools
🪛 golangci-lint (1.64.8)
60-60: ineffectual assignment to err
(ineffassign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dumper/dialer/ssh_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
dumper/dialer/ssh_test.go
60-60: ineffectual assignment to err
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (2)
dumper/dialer/ssh_test.go (2)
4-5: LGTM: Appropriate imports added.The added imports for
encoding/base64andosare necessary for the new test function.
43-54: LGTM: Good file handling practices.The test creates a temporary file and properly handles cleanup with deferred functions to close and remove the file after the test completes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
fileutil/checksum.go (1)
108-112:⚠️ Potential issueFix error variable shadowing in defer block.
The error variable
erris reused in the defer block which could overwrite other errors detected later in the function.defer func() { - if err = file.Close(); err != nil { - slog.Error("fail to close state file while saving", slog.Any("error", err)) + if closeErr := file.Close(); closeErr != nil { + slog.Error("fail to close state file while saving", slog.Any("error", closeErr)) } }()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-111: fileutil/checksum.go#L110-L111
Added lines #L110 - L111 were not covered by testsfileutil/checksum_test.go (2)
71-74: 🛠️ Refactor suggestionImprove the test for
getStateFilePath.The current test uses an empty file path which doesn't properly verify that directory determination works correctly.
func TestGetStateFilePath(t *testing.T) { - checksum := NewChecksum("") - assert.Equal(t, "checksum.onedump", checksum.getStateFilePath()) + // Test with an actual file path to properly test the directory determination + testPath := "/path/to/some/file.txt" + checksum := NewChecksum(testPath) + expectedPath := "/path/to/some/checksum.onedump" + assert.Equal(t, expectedPath, checksum.getStateFilePath()) + + // Test with a relative path + checksum = NewChecksum("relative/path/file.txt") + assert.Equal(t, "relative/path/checksum.onedump", checksum.getStateFilePath()) }
96-96:⚠️ Potential issueCheck the error from SaveState.
The error returned from
SaveState()is assigned to a variable but never checked.- err = checksum.SaveState() + err = checksum.SaveState() + assert.Nil(err)🧰 Tools
🪛 golangci-lint (1.64.8)
96-96: ineffectual assignment to err
(ineffassign)
🧹 Nitpick comments (7)
fileutil/checksum.go (5)
17-20: Remove the unused fieldchecksum.The
checksumfield in theChecksumstruct is never utilized in any of the methods. The code computes the checksum on-demand rather than storing it.type Checksum struct { filePath string - checksum string }🧰 Tools
🪛 golangci-lint (1.64.8)
19-19: field
checksumis unused(unused)
71-72: Correct the comment about checksum size.The comment states "Checksum is always 64 bits" which is incorrect. SHA-256 produces a 256-bit hash, which is represented as a 64-character hexadecimal string (each byte requires 2 hex characters).
- // Checksum is always 64 bits, so use bufio scanner is simple and safe for this case. + // SHA-256 checksum is always 64 characters in hex format, so using bufio scanner is simple and safe for this case.
65-69: Fix the typo in the error message.There is a spelling error in the log message for the file close error.
defer func() { if closeErr := stateFile.Close(); closeErr != nil { - slog.Error("fail to close the checksum state file while checking if file has been transfered", slog.Any("error", closeErr)) + slog.Error("fail to close the checksum state file while checking if file has been transferred", slog.Any("error", closeErr)) } }()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 67-68: fileutil/checksum.go#L67-L68
Added lines #L67 - L68 were not covered by tests
87-96: Simplify the DeleteState method.The function can be simplified while maintaining the same behavior.
func (c *Checksum) DeleteState() error { err := os.Remove(c.getStateFilePath()) if err != nil { if os.IsNotExist(err) { return nil } return err } return nil }This could be simplified to:
func (c *Checksum) DeleteState() error { err := os.Remove(c.getStateFilePath()) if err != nil && !os.IsNotExist(err) { return err } return nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-95: fileutil/checksum.go#L95
Added line #L95 was not covered by tests
90-96: Improve test coverage for error paths.Several error paths in the code are not covered by tests according to code coverage analysis. Consider adding tests for key error scenarios.
Consider implementing tests that simulate file operation failures to improve coverage, such as:
- File not found errors
- Permission errors
- I/O errors during reading/writing
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-95: fileutil/checksum.go#L95
Added line #L95 was not covered by testsfileutil/checksum_test.go (2)
50-50: Fix typo in test filename.There's a spelling error in the test filename "another_chcksum.txt".
- filename2 := "another_chcksum.txt" + filename2 := "another_checksum.txt"
109-113: Enhance the DeleteState test.The current test only verifies behavior when the state file doesn't exist. Consider testing the happy path as well.
func TestDeleteState(t *testing.T) { + // Test when state file does not exist checksum := NewChecksum("") err := checksum.DeleteState() assert.Nil(t, err) + + // Test when state file exists + filename := "test_delete_state.txt" + err = createTestFile(filename, "test content") + assert.Nil(t, err) + + defer func() { + if err := os.Remove(filename); err != nil { + t.Error(err) + } + }() + + checksum = NewChecksum(filename) + err = checksum.SaveState() + assert.Nil(t, err) + + err = checksum.DeleteState() + assert.Nil(t, err) + + // Verify state file is gone + _, err = os.Stat(checksum.getStateFilePath()) + assert.True(t, os.IsNotExist(err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codecov.yml(1 hunks)fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
fileutil/checksum_test.go (1)
fileutil/checksum.go (1)
NewChecksum(22-24)
🪛 golangci-lint (1.64.8)
fileutil/checksum_test.go
96-96: ineffectual assignment to err
(ineffassign)
fileutil/checksum.go
19-19: field checksum is unused
(unused)
🪛 GitHub Check: codecov/patch
fileutil/checksum.go
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 55-56: fileutil/checksum.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 62-63: fileutil/checksum.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 67-68: fileutil/checksum.go#L67-L68
Added lines #L67 - L68 were not covered by tests
[warning] 81-82: fileutil/checksum.go#L81-L82
Added lines #L81 - L82 were not covered by tests
[warning] 95-95: fileutil/checksum.go#L95
Added line #L95 was not covered by tests
[warning] 105-106: fileutil/checksum.go#L105-L106
Added lines #L105 - L106 were not covered by tests
[warning] 110-111: fileutil/checksum.go#L110-L111
Added lines #L110 - L111 were not covered by tests
[warning] 116-117: fileutil/checksum.go#L116-L117
Added lines #L116 - L117 were not covered by tests
[warning] 121-122: fileutil/checksum.go#L121-L122
Added lines #L121 - L122 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (2)
codecov.yml (2)
1-2: Test Utilities Exclusion Added.
The new ignore section excludes all files under thetestutilsdirectory, ensuring that non-critical test utilities don't negatively impact your overall code coverage metrics.
7-8: Patch Target Coverage Updated.
The patch target has been reduced from 70% to 60%, offering more flexibility as new features are integrated. Please verify that this new threshold aligns with your team's quality standards and coverage expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testutils/sftp.go (1)
40-40: Rename struct to fix typographical error.The struct name "SftpHanlder" has a typo. Consider renaming it to "SftpHandler" to improve readability and maintain naming consistency.
-type SftpHanlder struct{} +type SftpHandler struct{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testutils/sftp.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
testutils/sftp.go (1)
testutils/ssh.go (1)
GenerateRSAPrivateKey(12-26)
🔇 Additional comments (4)
testutils/sftp.go (4)
68-95: Re-check directory listing logic.
Currently, iffileInfo.IsDir()is true at line 68, the code returns a list containing only the directory itself, bypassing the subsequent logic to list its contents at lines 73-95. This likely creates confusion or unintended behavior.
164-164: Replace invalid range loop with a conventional for loop.
for range numberOfRequestsis invalid becausenumberOfRequestsis an integer, not a slice or map. Use a standard for-loop instead.- for range numberOfRequests { + for i := 0; i < numberOfRequests; i++ {
180-186: Great job checking the error returned bynewChannel.Reject.This addresses the previous linting concern regarding unhandled errors.
210-214: Nice work handling the error returned byreq.Reply.Properly logging the error ensures better observability of potential communication failures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
fileutil/checksum.go (1)
103-107:⚠️ Potential issueFix error variable shadowing in defer function.
The error variable is being reused in the defer function, which can mask the original error value.
defer func() { - if err = file.Close(); err != nil { - slog.Error("fail to close state file while saving", slog.Any("error", err)) + if closeErr := file.Close(); closeErr != nil { + slog.Error("fail to close state file while saving", slog.Any("error", closeErr)) } }()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-106: fileutil/checksum.go#L105-L106
Added lines #L105 - L106 were not covered by testsfileutil/checksum_test.go (1)
71-74: 🛠️ Refactor suggestionImprove the test for
getStateFilePath.The current test uses an empty file path which isn't realistic and doesn't properly test the functionality.
func TestGetStateFilePath(t *testing.T) { - checksum := NewChecksum("") - assert.Equal(t, "checksum.onedump", checksum.getStateFilePath()) + // Test with an actual file path to properly test the directory determination + testPath := "/path/to/some/file.txt" + checksum := NewChecksum(testPath) + expectedPath := "/path/to/some/checksum.onedump" + assert.Equal(t, expectedPath, checksum.getStateFilePath()) + + // Test with a relative path + checksum = NewChecksum("relative/path/file.txt") + assert.Equal(t, "relative/path/checksum.onedump", checksum.getStateFilePath()) }
🧹 Nitpick comments (1)
fileutil/checksum_test.go (1)
76-107: Add test for SaveState with existing content.The test for
IsFileTransferredindirectly testsSaveState, but there's no explicit test for the condition at line 121-123 where the file already has content.Consider adding a test that:
- Saves state once
- Modifies the file content
- Saves state again
- Verifies both checksums are in the state file
func TestSaveState(t *testing.T) { assert := assert.New(t) filename := "test_save_state.txt" // Create test file with initial content assert.Nil(createTestFile(filename, "initial content")) defer os.Remove(filename) checksum := NewChecksum(filename) defer checksum.DeleteState() // Save state for initial content assert.Nil(checksum.SaveState()) // Change file content assert.Nil(createTestFile(filename, "updated content")) // Save state again assert.Nil(checksum.SaveState()) // Verify both checksums are saved stateFilePath := checksum.getStateFilePath() content, err := os.ReadFile(stateFilePath) assert.Nil(err) // State file should contain two lines lines := strings.Split(string(content), "\n") assert.Len(lines, 2) }🧰 Tools
🪛 golangci-lint (1.64.8)
96-96: ineffectual assignment to err
(ineffassign)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)testutils/sftp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testutils/sftp.go
🧰 Additional context used
🧬 Code Definitions (1)
fileutil/checksum_test.go (1)
fileutil/checksum.go (1)
NewChecksum(21-23)
🪛 golangci-lint (1.64.8)
fileutil/checksum_test.go
96-96: ineffectual assignment to err
(ineffassign)
🪛 GitHub Check: codecov/patch
fileutil/checksum.go
[warning] 31-32: fileutil/checksum.go#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 36-37: fileutil/checksum.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 41-42: fileutil/checksum.go#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 54-55: fileutil/checksum.go#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 61-62: fileutil/checksum.go#L61-L62
Added lines #L61 - L62 were not covered by tests
[warning] 66-67: fileutil/checksum.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 80-81: fileutil/checksum.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 90-91: fileutil/checksum.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 100-101: fileutil/checksum.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 105-106: fileutil/checksum.go#L105-L106
Added lines #L105 - L106 were not covered by tests
[warning] 111-112: fileutil/checksum.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 116-117: fileutil/checksum.go#L116-L117
Added lines #L116 - L117 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (8)
fileutil/checksum.go (5)
17-19: Good design for the Checksum struct.The
Checksumstruct is simple and focused with a single responsibility, following good Go design principles.
25-45: Well-implemented checksum calculation.The
computeChecksummethod correctly implements SHA-256 hashing for files, with proper error handling and resource cleanup through deferred file closing.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-32: fileutil/checksum.go#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 36-37: fileutil/checksum.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 41-42: fileutil/checksum.go#L41-L42
Added lines #L41 - L42 were not covered by tests
47-49: Clean implementation of state file path resolution.This method correctly uses
filepath.Joinandfilepath.Dirto locate the state file in the same directory as the target file.
51-84: Well structured file transfer verification.The
IsFileTransferredmethod correctly computes the current file's checksum and compares it against previously stored values. Good error handling and clear logic flow.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 54-55: fileutil/checksum.go#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 61-62: fileutil/checksum.go#L61-L62
Added lines #L61 - L62 were not covered by tests
[warning] 66-67: fileutil/checksum.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 80-81: fileutil/checksum.go#L80-L81
Added lines #L80 - L81 were not covered by tests
86-94: Robust state deletion with proper error handling.The
DeleteStatemethod correctly handles the case where the file might not exist, avoiding unnecessary errors.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-91: fileutil/checksum.go#L90-L91
Added lines #L90 - L91 were not covered by testsfileutil/checksum_test.go (3)
10-26: Well-designed test helper function.The
createTestFilefunction is clean, includes proper error handling, and follows Go conventions for creating temporary files for testing.
28-69: Good test for checksum computation.The test properly verifies:
- Checksum length is correct (64 characters for SHA-256)
- Identical content produces identical checksums
- Proper cleanup of test files
109-113: Test for non-existent state file.The
TestDeleteStatecorrectly verifies that deleting a non-existent state file doesn't produce an error, which is an important edge case.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
cmd/root_test.go (1)
76-119: 🛠️ Refactor suggestionSubtest for empty job configuration
The subtest is well structured. However, at line 89, the return value ofcmd.Execute()is not checked. It's best practice to handle or assert the error to avoid missing potential failures.Apply the following diff to handle the error:
- cmd.Execute() + err := cmd.Execute() + assert.NotNil(err, "expected an error when no job is defined")🧰 Tools
🪛 golangci-lint (1.64.8)
89-89: Error return value of
cmd.Executeis not checked(errcheck)
handler/jobhandler_test.go (1)
112-112: 🛠️ Refactor suggestionError not checked from req.Reply
Ignoring the error fromreq.Reply(true, nil)may introduce silent failures. It's advisable to handle or log the returned error as shown below.- req.Reply(true, nil) + if err := req.Reply(true, nil); err != nil { + t.Logf("Failed to reply to the SSH request: %v", err) + }🧰 Tools
🪛 golangci-lint (1.64.8)
112-112: Error return value of
req.Replyis not checked(errcheck)
fileutil/checksum_test.go (1)
96-106:⚠️ Potential issueFix ineffectual error assignments.
The error from
SaveState()is assigned but never checked, and the error variableerris reused without checking.- err = checksum.SaveState() + err := checksum.SaveState() + assert.Nil(err) defer func() { if err := checksum.DeleteState(); err != nil { t.Error(err) } }() - transferred, err = checksum.IsFileTransferred() + transferred, err := checksum.IsFileTransferred() assert.Nil(err) assert.True(transferred)cmd/root.go (1)
145-149: 🛠️ Refactor suggestionHandle potential errors from
MarkFlagRequired.Although these error returns are rare, it’s a best practice to at least log them. You could do:
+if err := syncSftpCmd.MarkFlagRequired("source"); err != nil { + slog.Error("Unable to mark flag as required", slog.Any("arg", "source"), slog.Any("error", err)) +} // Similarly for other flags🧰 Tools
🪛 golangci-lint (1.64.8)
145-145: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
146-146: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
147-147: Error return value of
syncSftpCmd.MarkFlagRequiredis not checked(errcheck)
🧹 Nitpick comments (12)
dumper/mysqlnativedump.go (1)
394-396: Context-based Ping for improved reliability
Using a 10-second context-based ping significantly enhances reliability in case of slow or unresponsive database connections. Consider making the timeout configurable.storage/sftp/sftp.go (3)
38-47: Sftp struct concurrency details
Using a mutex to protect shared fields (written,attempts) is crucial for thread-safe operation.Consider adding a comment near the struct to clarify concurrency usage and constraints around
writtenandattempts.
72-88: Progress bar creation
Usinggo-ansiandprogressbarto visualize SFTP uploads is a nice UX enhancement. Ensure logging or a silent mode is available for headless or CI environments.Consider an option to disable progress bar output for automated runs.
90-206: Write & Save methods with offset-based resume & retry
- The offset logic and usage of
io.Seekerfor resumed transfers is excellent.- Exponential backoff in
Savehandles transient errors gracefully.- The code uses contextless sleeps in
Save. For large attempts or deployments, consider making the backoff or global context externally configurable.- Verify partial writes in
io.Copyif network disruptions occur mid-copy.- io.Copy(io.MultiWriter(file, bar), reader) + // optionally wrap the writer to track partial writes + n, copyErr := io.Copy(io.MultiWriter(file, bar), reader)docs/sync/sftp.md (1)
18-18: Improve grammar in the warning note.The sentence structure can be improved for better clarity.
-> Destination must be a folder if source is a folder, otherwise command will return error +> Destination must be a folder if source is a folder, otherwise the command will return an error🧰 Tools
🪛 LanguageTool
[typographical] ~18-~18: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ination must be a folder if source is a folder, otherwise command will return error ``` onedump ...(THUS_SENTENCE)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...folder if source is a folder, otherwise command will return error ``` onedump sync sft...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...a folder, otherwise command will return error ``` onedump sync sftp \ --source"/path...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
README.md (1)
305-305: Fix verb form in the section header.The verb form should be "Check out" (two words) rather than "Checkout" (one word) when used as a command.
-Checkout the [sync sftp documentation](./docs/sync/sftp.md) for more details. +Check out the [sync sftp documentation](./docs/sync/sftp.md) for more details.🧰 Tools
🪛 LanguageTool
[grammar] ~305-~305: This sentence should probably be started with a verb instead of the noun ‘Checkout’. If not, consider inserting a comma for better clarity.
Context: ...ble and concurrent SFTP file transfers Checkout the [sync sftp documentation](./docs/sy...(SENT_START_NN_DT)
cmd/syncsftp.go (1)
140-140: Fix typo in error message.There's a small typo in the error message.
-return errors.New("detination should not be a file when transfer multiple files from the source") +return errors.New("destination should not be a file when transferring multiple files from the source")fileutil/checksum.go (4)
16-20: DefineChecksumstruct usage in package documentation.While the
Checksumstruct is self-explanatory, consider adding a brief comment explaining its purpose so future maintainers understand how it fits into the broader design.
22-24: Provide inline usage guidance.
NewChecksumis straightforward, but including an inline doc comment might help package consumers understand how and when to use it.
48-50: Expand docstring forgetStateFilePath.A short doc comment can clarify that it generates the path to the
checksum.onedumpfile in the same directory as the original file.
107-141: Refactor deferred file closing to avoid overshadowingerr.Currently, line 115 reassigns the parent function’s
err. It's clearer to use a separate variable, for example:defer func() { - if err = file.Close(); err != nil { - slog.Error("fail to close state file while saving", slog.Any("error", err)) + if closeErr := file.Close(); closeErr != nil { + slog.Error("fail to close state file while saving", slog.Any("error", closeErr)) } }()This makes the code more readable.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 111-112: fileutil/checksum.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 116-117: fileutil/checksum.go#L116-L117
Added lines #L116 - L117 were not covered by testscmd/root.go (1)
131-143: Add comments for newly introduced SFTP-related flags.Consider providing short usage descriptions or doc comments to clarify how end users interact with these flags in practice, especially the intended usage of
--appendvs.--checksum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(3 hunks)cmd/root.go(2 hunks)cmd/root_test.go(2 hunks)cmd/syncsftp.go(1 hunks)cmd/syncsftp_test.go(1 hunks)docs/CONFIG_REF.md(2 hunks)docs/sync/sftp.md(1 hunks)dumper/mysqlnativedump.go(3 hunks)fileutil/checksum.go(1 hunks)fileutil/checksum_test.go(1 hunks)handler/jobhandler_test.go(3 hunks)storage/dropbox/dropbox.go(3 hunks)storage/sftp/sftp.go(1 hunks)testutils/sftp.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- storage/dropbox/dropbox.go
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/CONFIG_REF.md
- cmd/syncsftp_test.go
- testutils/sftp.go
🧰 Additional context used
🧬 Code Definitions (5)
cmd/syncsftp.go (2)
storage/sftp/sftp.go (2)
SftpConifg(33-36)NewSftp(49-56)fileutil/checksum.go (1)
NewChecksum(22-24)
fileutil/checksum_test.go (1)
fileutil/checksum.go (1)
NewChecksum(22-24)
storage/sftp/sftp.go (1)
dumper/dialer/ssh.go (1)
NewSsh(18-24)
cmd/root_test.go (1)
cmd/root.go (1)
Execute(100-104)
handler/jobhandler_test.go (1)
config/job.go (4)
NewJob(92-104)WithSshHost(62-66)WithSshUser(68-72)WithSshKey(86-90)
🪛 LanguageTool
docs/sync/sftp.md
[typographical] ~18-~18: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ination must be a folder if source is a folder, otherwise command will return error ``` onedump ...
(THUS_SENTENCE)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...folder if source is a folder, otherwise command will return error ``` onedump sync sft...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...a folder, otherwise command will return error ``` onedump sync sftp \ --source"/path...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
README.md
[grammar] ~16-~16: The singular proper name ‘MySQL’ must be used with a third-person or a past tense verb.
Context: ...rces to different destinations. * MySQL dump with zero dependencies (with built-in m...
(HE_VERB_AGR)
[grammar] ~305-~305: This sentence should probably be started with a verb instead of the noun ‘Checkout’. If not, consider inserting a comma for better clarity.
Context: ...ble and concurrent SFTP file transfers Checkout the [sync sftp documentation](./docs/sy...
(SENT_START_NN_DT)
🪛 GitHub Check: codecov/patch
cmd/syncsftp.go
[warning] 34-35: cmd/syncsftp.go#L34-L35
Added lines #L34 - L35 were not covered by tests
[warning] 46-47: cmd/syncsftp.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 57-58: cmd/syncsftp.go#L57-L58
Added lines #L57 - L58 were not covered by tests
[warning] 62-63: cmd/syncsftp.go#L62-L63
Added lines #L62 - L63 were not covered by tests
[warning] 74-75: cmd/syncsftp.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 79-80: cmd/syncsftp.go#L79-L80
Added lines #L79 - L80 were not covered by tests
[warning] 87-88: cmd/syncsftp.go#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 97-98: cmd/syncsftp.go#L97-L98
Added lines #L97 - L98 were not covered by tests
[warning] 103-104: cmd/syncsftp.go#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 126-127: cmd/syncsftp.go#L126-L127
Added lines #L126 - L127 were not covered by tests
[warning] 131-132: cmd/syncsftp.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 136-137: cmd/syncsftp.go#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 146-147: cmd/syncsftp.go#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 150-151: cmd/syncsftp.go#L150-L151
Added lines #L150 - L151 were not covered by tests
fileutil/checksum.go
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 55-56: fileutil/checksum.go#L55-L56
Added lines #L55 - L56 were not covered by tests
[warning] 65-65: fileutil/checksum.go#L65
Added line #L65 was not covered by tests
[warning] 72-73: fileutil/checksum.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 77-78: fileutil/checksum.go#L77-L78
Added lines #L77 - L78 were not covered by tests
[warning] 90-92: fileutil/checksum.go#L90-L92
Added lines #L90 - L92 were not covered by tests
[warning] 94-94: fileutil/checksum.go#L94
Added line #L94 was not covered by tests
[warning] 101-102: fileutil/checksum.go#L101-L102
Added lines #L101 - L102 were not covered by tests
[warning] 111-112: fileutil/checksum.go#L111-L112
Added lines #L111 - L112 were not covered by tests
[warning] 116-117: fileutil/checksum.go#L116-L117
Added lines #L116 - L117 were not covered by tests
dumper/mysqlnativedump.go
[warning] 352-352: dumper/mysqlnativedump.go#L352
Added line #L352 was not covered by tests
[warning] 391-391: dumper/mysqlnativedump.go#L391
Added line #L391 was not covered by tests
🪛 golangci-lint (1.64.8)
cmd/root.go
145-145: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
146-146: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
147-147: Error return value of syncSftpCmd.MarkFlagRequired is not checked
(errcheck)
cmd/root_test.go
89-89: Error return value of cmd.Execute is not checked
(errcheck)
64-64: ineffectual assignment to err
(ineffassign)
handler/jobhandler_test.go
112-112: Error return value of req.Reply is not checked
(errcheck)
🔇 Additional comments (26)
dumper/mysqlnativedump.go (3)
13-13: No issues with the newly added import
Everything looks good with this import statement.
352-352: Typo fix and improved clarity for the create table error
This line corrects the typo from "faile" to "fail" and clarifies the error message.Since line #352 is uncovered by tests, consider adding a test scenario that triggers this error condition by passing an invalid table name or mocking the driver result. Do you want a code snippet for that?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 352-352: dumper/mysqlnativedump.go#L352
Added line #L352 was not covered by tests
391-391:❓ Verification inconclusive
Replaced log.Fatal with a returned error
Returning errors instead of forcibly exiting the process is a best practice, enabling upstream callers or tests to handle the error gracefully. However, line #391 remains uncovered by tests.Would you like to add a new test that attempts to open an invalid database to confirm that the function returns the correct error? I can provide a test snippet if needed.
Enhance Test Coverage for Error Return
Great job replacing
log.Fatalwith a returned error—it significantly improves error handling by allowing consumers of the function to manage errors gracefully. However, the error return at line 391 indumper/mysqlnativedump.gois not currently covered by tests. It would be beneficial to add a new test that simulates a failure in opening a database, thereby verifying that the function returns the expected error. Let me know if you’d like a test snippet to get started.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 391-391: dumper/mysqlnativedump.go#L391
Added line #L391 was not covered by testscmd/root_test.go (2)
46-46: Cron interval updated
Switching from "1sec" to "1s" aligns well with standard time unit usage.
59-74: Subtest for missing job file
This subtest properly verifies error handling for inaccessible job files. Good use ofSetErrand buffer to capture error messages.🧰 Tools
🪛 golangci-lint (1.64.8)
64-64: ineffectual assignment to err
(ineffassign)
handler/jobhandler_test.go (3)
38-38: SshHost port changed
Switching to "127.0.0.1:20002" is consistent with the updated listener port below, ensuring connectivity alignment.
73-73: Listener port updated
Listening on "0.0.0.0:20002" matches the changed SSH host, ensuring both ends connect on the same port.
76-76: Buffered channel for concurrency
Creating a buffered channel with capacitylen(onedump.Jobs)helps keep the concurrency flow unblocked while each job completes.storage/sftp/sftp.go (4)
27-37: Struct definitions for transfer results and SFTP config
DefiningResultandSftpConifgclearly outlines the data contract for SFTP transfers. Ensure the config fields have default behaviors or validations if they’re optional.
49-56: Constructor for SFTP client
Initializes the struct withSshHost,SshUser, andSshKey. This is straightforward. Just confirm if you need input validation for these fields.Do you want to ensure that
SshHost,SshUser, andSshKeyare non-empty before proceeding? If so, consider returning an error or logging a warning.
58-70: State management methods
reset()andattempt()correctly manage attempts andwrittencounters. This complements your retry logic well.
208-242:❓ Verification inconclusive
Directory existence check
This method correctly differentiates between a missing path and a directory. Consider also handling symlinks or other special file types if relevant for your use cases.Do you want to handle symbolic links or block devices differently? We can do a quick search to confirm whether that's desired.
🏁 Script executed:
#!/bin/bash # Checking references to IsPathDir usage across the codebase rg -A 5 "IsPathDir"Length of output: 690
Consider Symlink and Special File Type Handling
The current implementation ofIsPathDircorrectly distinguishes between non-existent paths and existing directories. However, because it relies onclient.Stat(path), symbolic links are automatically resolved, which means a symlink to a directory will be treated the same as an actual directory. If your use case requires distinct handling for symbolic links or other special file types (such as block devices), please confirm whether to adjust the logic (for example, by using anLstat-style check or additional mode validations).
- Location:
storage/sftp/sftp.go(lines 208-242)docs/sync/sftp.md (3)
33-33: LGTM! Helpful information about checksum storage.This is a good explanation of where checksums are stored, which helps users understand how the feature works internally.
37-37: Well-documented retry behavior.Clear explanation of the retry mechanism and how to limit retry attempts using the
--max-attemptsoption.
1-2: Clear and descriptive heading.The heading effectively communicates the key features of the command: resumability and concurrency for SFTP transfers.
README.md (2)
12-12: LGTM! Improved product description.The updated description better reflects the expanded capabilities of the tool, positioning it as a more comprehensive database administration solution rather than just a backup tool.
19-19: LGTM! Clear feature addition.The new feature point clearly communicates the addition of SFTP file transfer capabilities with the important attributes of resumability and concurrency.
cmd/syncsftp.go (2)
139-141: LGTM! Important validation for source/destination compatibility.This validation is crucial to prevent users from encountering unexpected behavior when trying to transfer a directory to a file destination.
110-158: Enhance test coverage for SFTP command.Many error handling paths in the command's RunE function aren't covered by tests. This could lead to undetected issues in error handling logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 126-127: cmd/syncsftp.go#L126-L127
Added lines #L126 - L127 were not covered by tests
[warning] 131-132: cmd/syncsftp.go#L131-L132
Added lines #L131 - L132 were not covered by tests
[warning] 136-137: cmd/syncsftp.go#L136-L137
Added lines #L136 - L137 were not covered by tests
[warning] 146-147: cmd/syncsftp.go#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 150-151: cmd/syncsftp.go#L150-L151
Added lines #L150 - L151 were not covered by testsfileutil/checksum_test.go (4)
71-74: Improve the test for getStateFilePath.The current test uses an empty file path which isn't realistic and doesn't properly test the functionality.
func TestGetStateFilePath(t *testing.T) { - checksum := NewChecksum("") - assert.Equal(t, "checksum.onedump", checksum.getStateFilePath()) + // Test with an actual file path to properly test the directory determination + testPath := "/path/to/some/file.txt" + checksum := NewChecksum(testPath) + expectedPath := "/path/to/some/checksum.onedump" + assert.Equal(t, expectedPath, checksum.getStateFilePath()) + + // Test with a relative path + checksum = NewChecksum("relative/path/file.txt") + assert.Equal(t, "relative/path/checksum.onedump", checksum.getStateFilePath()) }
110-114: LGTM! Simple test for DeleteState.This test verifies that DeleteState doesn't return an error. While it's a simple test, it's sufficient for the behavior being tested.
28-69: Well-structured test for checksum computation.The test thoroughly verifies that the checksum computation produces consistent results for identical content in different files and correctly handles cleanup of test resources.
10-26: LGTM! Useful helper function for file creation in tests.The
createTestFilefunction properly handles file creation, writing content, and closing the file with appropriate error handling at each step.fileutil/checksum.go (2)
26-46: Enhance testing coverage for error scenarios incomputeChecksum.Consider specifically testing cases where:
os.Openfails (lines 31–32).io.Copyfails (lines 41–42).You can force these errors by injecting a mock or a faulty file handle in your tests.
Would you like me to propose an additional test snippet to ensure these lines are covered?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: fileutil/checksum.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 37-38: fileutil/checksum.go#L37-L38
Added lines #L37 - L38 were not covered by tests
[warning] 42-43: fileutil/checksum.go#L42-L43
Added lines #L42 - L43 were not covered by tests
97-105: Confirm no concurrency conflicts inDeleteState.While this method is simple, concurrent invocations might race on file existence checks. If concurrency is expected, consider a file lock or documentation clarifying no parallel usage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 101-102: fileutil/checksum.go#L101-L102
Added lines #L101 - L102 were not covered by testscmd/root.go (1)
152-152: Validate the newsyncSftpCmdusage.Everything else looks consistent. Just ensure you have corresponding unit or integration tests verifying that
sync sftpis fully functional.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/sync/sftp.md (3)
9-15: Nitpick: Specify Language in Fenced Code Block
The fenced code block for the single file transfer example does not specify a language. Adding a language (for example,bash) after the opening triple backticks will improve readability and tool support.Consider this diff:
-``` +```bash
17-19: Typographical Enhancement for Clarity
The noteDestination must be a folder if source is a folder, otherwise command will return error
could be rephrased to improve clarity and grammatical accuracy. For example:-Destination must be a folder if source is a folder, otherwise command will return error +Destination must be a folder if the source is a folder; otherwise, the command will return an error.🧰 Tools
🪛 LanguageTool
[typographical] ~18-~18: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ination must be a folder if source is a folder, otherwise command will return error ``` onedump ...(THUS_SENTENCE)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...folder if source is a folder, otherwise command will return error ``` onedump sync sft...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...a folder, otherwise command will return error ``` onedump sync sftp \ --source="/pat...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
21-27: Nitpick: Specify Language in Fenced Code Block
The fenced code block for the folder transfer example also lacks a language specifier. Adding one (e.g.,bash) will help maintain clarity and adhere to markdown best practices.Consider this diff:
-``` +```bash
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/sync/sftp.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/sync/sftp.md
[typographical] ~18-~18: The word “otherwise” is an adverb that can’t be used like a conjunction, and therefore needs to be separated from the sentence.
Context: ...ination must be a folder if source is a folder, otherwise command will return error ``` onedump ...
(THUS_SENTENCE)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ...folder if source is a folder, otherwise command will return error ``` onedump sync sft...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...a folder, otherwise command will return error ``` onedump sync sftp \ --source="/pat...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🪛 markdownlint-cli2 (0.17.2)
docs/sync/sftp.md
8-8: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (1.24.1, windows-latest)
🔇 Additional comments (4)
docs/sync/sftp.md (4)
1-4: Introductory Section Looks Clear
The opening lines clearly introduce the SFTP file transfer functionality. No issues here.
9-15: Attention: Command Flag Syntax Consistency (Single File Transfer)
The command example uses--source="/path/to/file.txt". Previous feedback suggested using a space between the flag and its argument (i.e.,--source "/path/to/file.txt"). Please verify whether the CLI expects the equals-sign syntax or the space-separated version and update accordingly for consistency.
21-27: Attention: Command Flag Syntax Consistency (Folder Transfer)
The folder transfer example similarly uses--source="/path/to/folder/". Please confirm that using an equals sign is the intended CLI syntax. If not, update it to match the preferred style (e.g.,--source "/path/to/folder/") for consistency across examples.
29-40: General Documentation Feedback
The sections on avoiding re-transferring files, max retry attempts, and available options are well-documented and clear.
Summary by CodeRabbit
New Features
Improvements
Documentation
sync sftpcommand outlining its functionality and usage.Tests