Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

lightning: replace storage.Create by storage.New #899

Merged
merged 2 commits into from
Mar 24, 2021

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Mar 18, 2021

What problem does this PR solve?

  1. storage.Create has been deprecated... so let's replace the remaining two calls from Lightning.
  2. Lightning originally used storage.Create(sendCreds=true), but
    • the entire cloud access happens within Lightning, so we don't need to send credentials to anyone
    • this introduced skipCheckPath=false, which leads to #898.

What is changed and how it works?

Changed storage.Create to storage.New with sendCreds=false and skipCheckPath=true.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Code changes

Side effects

Related changes

Release Note

  • When using Lightning to import from S3, we no longer require s3:ListBucket permission on the entire bucket, only the data source prefix itself.

@glorv
Copy link
Collaborator

glorv commented Mar 22, 2021

So shall we also remote the storage.Create source code since it's not recommanded.

@kennytm
Copy link
Collaborator Author

kennytm commented Mar 22, 2021

@glorv Yes after it has been removed from Dumpling.

@glorv
Copy link
Collaborator

glorv commented Mar 23, 2021

/lgtm

@ti-srebot ti-srebot added the status/LGT1 LGTM1 label Mar 23, 2021
@3pointer
Copy link
Collaborator

/LGTM

@ti-srebot ti-srebot removed the status/LGT1 LGTM1 label Mar 23, 2021
@glorv
Copy link
Collaborator

glorv commented Mar 23, 2021

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@kennytm merge failed.

@glorv
Copy link
Collaborator

glorv commented Mar 23, 2021

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@kennytm merge failed.

@glorv
Copy link
Collaborator

glorv commented Mar 23, 2021

/run-all-tests

@glorv
Copy link
Collaborator

glorv commented Mar 23, 2021

/build

@glorv
Copy link
Collaborator

glorv commented Mar 24, 2021

/run-all-tests

@kennytm kennytm merged commit fea552d into pingcap:master Mar 24, 2021
@kennytm kennytm deleted the lightning-skip-check-paths-on-s3 branch March 24, 2021 06:16
ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Mar 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #919

ti-srebot pushed a commit to ti-srebot/br that referenced this pull request Mar 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #920

kennytm added a commit that referenced this pull request Mar 24, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: kennytm <kennytm@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants