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

Switch to github.com/aws/aws-sdk-go-v2 #467

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lebauce
Copy link

@lebauce lebauce commented Oct 24, 2023

The aws-sdk-go-v2 was announced as general availability
in 2021 and brings few advantages such as not storing
the endpoints list as a global variable, freeing memory.

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 24, 2023

CLA assistant check
All committers have signed the CLA.

@DanOfir
Copy link

DanOfir commented Jan 31, 2024

is this going to be merged soon? there is a timeline for this?

@joemiller
Copy link

AWS announced an EOL date for go sdk v1 recently - https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-go-v1-on-july-31-2025/

In alignment with our SDKs and Tools Maintenance Policy, AWS SDK for Go (v1) will enter maintenance mode on July 31, 2024 and reach end-of-support on July 31, 2025.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I don't have the capacity to review this more thoroughly at this point but one thing which stands out to me on a quick skim over is the old SDK v1 is still present in go.mod. It looks like all there's needed to address that is go mod tidy though.

@lebauce
Copy link
Author

lebauce commented Mar 27, 2024

@radeksimko Thank you for taking the time to look at the PR. I updated the PR, updating the file go.mod and applying the suggestion from @paulcacheux

get_s3.go Outdated Show resolved Hide resolved
@gdavison gdavison self-requested a review April 3, 2024 22:27
Copy link

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

Overall looks good, @lebauce. I have a few suggestions

get_s3.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
get_s3.go Outdated Show resolved Hide resolved
get_s3.go Outdated Show resolved Hide resolved
@radeksimko radeksimko removed their request for review April 4, 2024 06:11
@radeksimko radeksimko removed their request for review April 4, 2024 12:25
get_s3.go Outdated
Bucket: aws.String(bucket),
Prefix: aws.String(path),
}
if lastMarker != "" {
req.Marker = aws.String(lastMarker)
req.Delimiter = aws.String(lastMarker)
Copy link
Member

Choose a reason for hiding this comment

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

I don't have as much experience with AWS SDK as Graham does but I'd think that delimiters are not typically used in the context of pagination. Graham mentioned earlier that we could use the native paginators, as documented at https://aws.github.io/aws-sdk-go-v2/docs/making-requests/#using-paginators

@lebauce have you considered that?

Copy link
Author

@lebauce lebauce Apr 9, 2024

Choose a reason for hiding this comment

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

@radeksimko Pagination is not supported in the current version if I'm not mistaken. Do we want to implement this in this PR ?

Copy link

Choose a reason for hiding this comment

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

@lebauce in this case pagination is making multiple requests to ListObjectsV2 while ContinuationToken is set (was Marker in ListObjects). In aws-sdk-go-v2, AWS has added the NewListObjectsV2Paginator that can be used to handle pagination instead of using a for loop.

@GideonStowell
Copy link

I am also very interested in this functionality being added. Can we bubble this up?

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.

8 participants