-
Notifications
You must be signed in to change notification settings - Fork 250
Fixed panic for s3 and gcp transfers #3276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes protocol compatibility validation for Azure File Shares by adding conditional checks to skip validation when File locations are not involved, and simplifies the validation logic. The changes prevent unnecessary service client operations for S3, GCP, Blob, and Local transfers.
Key changes:
- Adds conditional guards to skip validation when neither source nor destination is a File location
- Simplifies
validateProtocolCompatibility()logic by removing complex switch statements - Fixes a potential nil service client bug in
getShareProtocolType() - Adds comprehensive test coverage for the conditional validation logic
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| common/util.go | Returns initialized ServiceClient struct instead of nil for unsupported locations in the default case |
| cmd/syncEnumerator.go | Adds conditional check to skip protocol validation when File locations are not involved |
| cmd/copy.go | Adds conditional check to skip protocol validation when File locations are not involved |
| cmd/flagsValidation.go | Simplifies validation logic, adds nil check for service client, and fixes error handling |
| cmd/flagsValidation_test.go | Adds comprehensive test coverage for the new conditional validation logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Resolves #3273
With the NFS over REST changes, we were checking share protocol compatibility when we didnt have to. This is changing the behavior of validateProtocolCompatibility to just validate File/FileNFS locations.
Feature / Bug Fix: (Brief description of the feature or issue being addressed)
Related Links:
Issues
Team thread
Documents
[Email Subject]
Type of Change
How Has This Been Tested?
Added unit tests for the fix
Thank you for your contribution to AzCopy!