Remove lakectl direct deprecated functionality #6623
Conversation
nopcoder
added
the
include-changelog
|
♻️ PR Preview 2eec97b has been successfully destroyed since this PR has been closed.
🤖 By surge-preview |
esti/adapters_test.go
Outdated
| @@ -8,6 +8,8 @@ import ( | |||
| "strings" | |||
| "time" | |||
|
|
|||
| "github.com/treeverse/lakefs/pkg/api/helpers" | |||
|
|
|||
| @@ -1,4 +1,4 @@ | |||
| package helpers | |||
| package esti | |||
There was a problem hiding this comment.
- Do we use all the functions defined here only in Esti?
- I'm not sure about the new file name, it says adapter_test but there are no tests
There was a problem hiding this comment.
The helper package had adapters that include upload direct.
All the tests that used direct were removed. Except the GC test that I wasn't sure if we like to remove this case - until we will enable different way, like presign.
I've moved the code into the system test (esti) so I've kept the name and added _test as this code should be built only when running test.
| require.Equalf(t, objContent, body, "path: %s, expected: %s, actual:%s", objPath, objContent, body) | ||
| }) | ||
| } | ||
| ctx, _, repo := setupTest(t) |
There was a problem hiding this comment.
Unrelated question to this PR- how do we check that everything works with pre-signed mode?
There was a problem hiding this comment.
I think that a better way to test it is to run all the tests in another suit with the pre-signed flag on so we don't have to remember to add a new test for every command/feature we add but again, it doesn't related to this PR
| if direct { | ||
| stats, err := helpers.ClientUploadDirect(ctx, client, repo, branch, objPath, nil, "", strings.NewReader(objContent)) | ||
| stats, err := uploadContentDirect(ctx, client, repo, branch, objPath, nil, "", strings.NewReader(objContent)) |
There was a problem hiding this comment.
why you didn't remove the direct flag from the uploadFileAndReport and uploadFileRandomDataAndReport functions?
There was a problem hiding this comment.
There are specific GC tests that upload direct so I've moved the implementation and kept the test helper function.
| // uploadContentDirect uploads contents as a file using client-side ("direct") access to underlying storage. | ||
| // It requires credentials both to lakeFS and to underlying storage, but considerably reduces the load on the lakeFS | ||
| // server. | ||
| func uploadContentDirect(ctx context.Context, client apigen.ClientWithResponsesInterface, repoID, branchID, objPath string, metadata map[string]string, contentType string, contents io.ReadSeeker) (*apigen.ObjectStats, error) { |
There was a problem hiding this comment.
I'm not sure I understand why we still need this
There was a problem hiding this comment.
If we do not require direct upload for GC testing, I'll remove this functionality.
Fix #6479