-
Notifications
You must be signed in to change notification settings - Fork 101
lightning: replace storage.Create by storage.New (#899) #919
lightning: replace storage.Create by storage.New (#899) #919
Conversation
Signed-off-by: ti-srebot <[email protected]>
/lgtm |
// we skip check path in favor of delaying the error to when we actually access the file. | ||
// on S3, performing "check path" requires the additional "s3:ListBucket" permission. | ||
SkipCheckPath: true, |
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.
What about BR? Does it need the change too?
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.
See #898 for details. Yes BR also needs to be fixed, but not via disabling SkipCheckPath everywhere.
/lgtm |
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 5e1d345
|
cherry-pick #899 to release-4.0
You can switch your code base to this Pull Request by using git-extras:
# In br repo: git pr https://github.com/pingcap/br/pull/919
After apply modifications, you can push your change to this PR via:
What problem does this PR solve?
storage.Create
has been deprecated... so let's replace the remaining two calls from Lightning.storage.Create(sendCreds=true)
, butskipCheckPath=false
, which leads to #898.What is changed and how it works?
Changed
storage.Create
tostorage.New
with sendCreds=false and skipCheckPath=true.Check List
Tests
Code changes
Side effects
Related changes
Release Note
s3:ListBucket
permission on the entire bucket, only the data source prefix itself.