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

Update s3manager's iface with missing methods #2612

Merged
merged 8 commits into from
Jun 13, 2019

Conversation

alexieyizhe
Copy link
Contributor

This addresses #2273 and adds some new methods on top of it:

  • NewDownloader
  • NewDownloaderWithClient
  • NewUploader
  • NewUploaderWithClient

that were missing in the interface before.

It was suggested in #2273 that since this interface was not autogenerated and had no warning, the methods should be added as separate interfaces that can be composed together by the user to avoid breaking when the API changes.

@alexieyizhe alexieyizhe changed the title Update s3manager's iface with new methods Update s3manager's iface with missing methods May 21, 2019
Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to create this PR @alexieyizhe. I think this is the right approach for this API. But I don't think we need to create interfaces for the constructor functions, just the methods.

Similar like the following.

type UploadWithIterator interface {
    UploadWithIterator(aws.Context, s3manager.BatchUploadIterator, ...func(*s3manager.Uploader)) error
}

type DownloadWithIterator interface {
    DownloadWithIterator(aws.Context, s3manager.BatchDownloadIterator, ...func(*s3manager.Downloader)) error
}

type BatchDeleter interface {
    Delete(aws.Context, s3manager.BatchDeleteIterator) error
}

@jasdel
Copy link
Contributor

jasdel commented Jun 3, 2019

I think the CI tests are failing due to Go formatting issues. Maybe try using gofmt -w ./service/s3/s3manager before committing the change.

@alexieyizhe
Copy link
Contributor Author

@jasdel that explains the tests, thank you :)

Are you referring to the NewDownloader/NewUploader interfaces when you say the constructor functions?

@jasdel
Copy link
Contributor

jasdel commented Jun 3, 2019

Correct, I was referring to the NewDownloader/NewUploader interfaces. could you go more into the reasoning behind creating interfaces for these functions?

@alexieyizhe
Copy link
Contributor Author

I was under the impression an interface would be needed for all the functions that were exposed in order to mock them out. Is this not the case? I'm not the most familiar with the sdk or Go so it may very well be that they're not needed.

@alexieyizhe
Copy link
Contributor Author

Hey @jasdel I've addressed the feedback you've given (appreciate it!)

@jasdel jasdel added needs-review This issue or pull request needs review from a core team member. and removed needs-response labels Jun 11, 2019
@jasdel
Copy link
Contributor

jasdel commented Jun 11, 2019

Thanks for the update @alexieyizhe, we'll review this update and get back with your with our feedback. Thanks!

@jasdel jasdel removed the needs-review This issue or pull request needs review from a core team member. label Jun 12, 2019
@jasdel jasdel merged commit 75c5222 into aws:master Jun 13, 2019
jasdel added a commit that referenced this pull request Jun 13, 2019
Adds PR #2612 to pending change log
@aws-sdk-go-automation aws-sdk-go-automation mentioned this pull request Jun 13, 2019
diehlaws pushed a commit to diehlaws/aws-sdk-go that referenced this pull request Jul 2, 2019
Adds the missing interface and methods from the `s3manager` Uploader, Downloader, and Batch Delete utilities.
diehlaws pushed a commit to diehlaws/aws-sdk-go that referenced this pull request Jul 2, 2019
Adds PR aws#2612 to pending change log
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.

2 participants