Skip to content
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

Feat presign #634

Merged
merged 6 commits into from
Aug 21, 2023
Merged

Feat presign #634

merged 6 commits into from
Aug 21, 2023

Conversation

zemul
Copy link
Contributor

@zemul zemul commented Aug 2, 2023

Used to share object urls.

@zemul zemul requested a review from a team as a code owner August 2, 2023 02:21
@zemul zemul requested review from igungor and seruman and removed request for a team August 2, 2023 02:21
storage/s3.go Show resolved Hide resolved
Comment on lines +570 to +571
url, err := req.Presign(expire)
return url, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may return directly here: return req.Presign(expire).


func validatePresignCommand(c *cli.Context) error {
if c.Args().Len() != 1 {
return fmt.Errorf("expected only one argument")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return fmt.Errorf("expected only one argument")
return fmt.Errorf("expected a remote object url")

@@ -558,6 +558,19 @@ func (s *S3) Read(ctx context.Context, src *url.URL) (io.ReadCloser, error) {
return resp.Body, nil
}

// Presign fetches the remote object url and returns its.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may remove the comment altogether.

@igungor
Copy link
Member

igungor commented Aug 11, 2023

@zemul It'd be great to have a testcase for this command. You may take a look at e2e directory for command tests.

@igungor
Copy link
Member

igungor commented Aug 21, 2023

I'll go ahead and merge this PR without waiting for the OP because it's a useful feature and the more we wait, the more this PR will receive conflicts. I'll apply the changes myself later.

@igungor igungor merged commit 6a1ea3c into peak:master Aug 21, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants