Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add PresignRequest API #119

Merged
merged 11 commits into from
Mar 24, 2022
Merged

Add PresignRequest API #119

merged 11 commits into from
Mar 24, 2022

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Mar 18, 2022

Expose PreSignRequest for storage clients

Signed-off-by: Haytham Abuelfutuh [email protected]

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#2263

Signed-off-by: Haytham Abuelfutuh <[email protected]>
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #119 (286706f) into master (d304944) will increase coverage by 0.35%.
The diff coverage is 73.91%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   68.21%   68.57%   +0.35%     
==========================================
  Files          66       66              
  Lines        3250     3274      +24     
==========================================
+ Hits         2217     2245      +28     
+ Misses        865      863       -2     
+ Partials      168      166       -2     
Flag Coverage Δ
unittests 67.26% <73.68%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
storage/cached_rawstore.go 82.14% <ø> (ø)
storage/mem_store.go 79.31% <0.00%> (-5.88%) ⬇️
storage/storage.go 100.00% <ø> (+28.57%) ⬆️
storage/utils.go 80.76% <ø> (ø)
storage/stow_store.go 74.87% <77.77%> (+1.86%) ⬆️
storage/rawstores.go 82.85% <100.00%> (ø)
storage/url_path.go 100.00% <100.00%> (+47.05%) ⬆️
cli/pflags/api/generator.go 71.30% <0.00%> (-0.68%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d304944...286706f. Read the comment docs.

EngHabu added 2 commits March 18, 2022 19:47
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu requested a review from wild-endeavor March 19, 2022 02:50
@EngHabu EngHabu marked this pull request as ready for review March 19, 2022 02:50
@EngHabu EngHabu marked this pull request as draft March 19, 2022 14:18
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu marked this pull request as ready for review March 21, 2022 13:37
@EngHabu EngHabu removed request for katrogan and kumare3 March 21, 2022 13:37
}, false, testScope)
assert.NoError(t, err)

actual, err := s.CreateSignedURL(context.TODO(), DataReference("https://container/path"), SignedURLProperties{})
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm confused why this happy path starts with https://... shouldn't this be s3 or gcs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any valid url format is ok.. no?

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess, i just find it confusing. would this ever be called like this?

@katrogan
Copy link
Contributor

oh this is great! we should move admin to use this for signed urls too once this is in

Signed-off-by: Haytham Abuelfutuh <[email protected]>
katrogan
katrogan previously approved these changes Mar 23, 2022
Signed-off-by: Haytham Abuelfutuh <[email protected]>
EngHabu added 4 commits March 22, 2022 20:54
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
katrogan
katrogan previously approved these changes Mar 23, 2022
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu merged commit 4edc011 into master Mar 24, 2022
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Add PresignRequest API

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Unit test and cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* more cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* More unit tests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update to latest stow commit

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update setup-go action

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* update go

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update github actions

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update go version

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Pinning go to 1.17

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update stow version to created release

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants