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

AWS-S3 v2 implementation #417

Merged
merged 9 commits into from
Jul 21, 2020
Merged

Conversation

gvolpe
Copy link
Collaborator

@gvolpe gvolpe commented Jul 19, 2020

closes #413

I removed scalatest and mockito as dependencies in favor of munit and MinIO, but only for the new s3 module.

What we really need is to run the minio instance to run the tests against it because the new tests I wrote need to access a real S3 instance (more like integration tests) as I don't think using mocks is a good idea. I did this in Redis4Cats with Github actions and it's pretty easy. I can help set it up for fs2-aws if there's any interest.

There is still room for improvement in the tests. We could remove the authentication data and have it as environment variables instead but it is not a big deal since it's only for tests.

We could also use more tests. For example, testing a very large file and see if the tests hold.

Let me know your thoughts.

@semenodm
Copy link
Member

it looks like there is a communication failure with mino in tests

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 20, 2020

@semenodm that's because there's no MinIO set up on the CI build you are using (Travis). I'd be happy to help set it up using Github actions since I've done it in Redis4Cats and it's fairly straightforward. Just needs a decision on your part.

@semenodm
Copy link
Member

sounds good, we also use testcontainers for the dockerized test suites. however we are all open for innovations, feel free to do it, i assume you need to be a part of github org to do that ?

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 20, 2020

Great @semenodm , not sure if I need to be part of the organization. I just added the GH actions file but it didn't trigger any build. Can you check if https://github.com/laserdisc-io/fs2-aws/settings/actions is enabled?

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 20, 2020

Also, I just noticed this implementation only works for Scala 2.13.x, needs to be adapted to compile with Scala 2.12.x.

@barryoneill
Copy link
Member

Hi @gvolpe - I just checked and actions are enabled. (Also - thanks for helping out!)

Screen Shot 2020-07-20 at 5 05 31 PM

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

@barryoneill thanks for confirming, I just read actions are enabled by default 😄

I think it doesn't run because I don't have merge permissions in this repo so it needs to be set up by someone with permissions first.

@barryoneill
Copy link
Member

D'oh, that's a bit of a pain. I'm fine to merge this to master, though @gvolpe can you verify that the README doc is updated for your new S3 module?

@semenodm I guess the next release shifts to a new major number (and perhaps an RC at that); I'd like to see us address #275 and #416 (actually, do this for all aws services) as part of the next major release. I can try and carve time out later in the week..

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

If you grant me permissions to the repo (can be done temporarily), it should work after I push more changes. I'll update the docs for this in the evening, haven't done it 👍 (also need to make it work with Scala 2.12.x).

@semenodm
Copy link
Member

@gvolpe i gave you write access to the repo

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

Thanks, you can remove me if you want, it's not needed. I found out GH actions don't run on PRs of forks: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories

I created a PR against master of my fork and they are now running and passing. Once I get it working with Scala 2.12.x and update the docs, I'll ping you here to see if we can get it in once and for all :)

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

I think we can also remove the TravisCI integration if you're okay with it :) All the tests are passing in GH actions: https://github.com/cr-org/fs2-aws/pull/1/checks?check_run_id=894776753

The only missing part would be the integration with Codacy, probably not hard to get it done but I don't know because I don't use it in my projects

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

This is one of the reasons why I don't like static analysis tools 😛 https://app.codacy.com/manual/semenodm/fs2-aws/pullRequest?prid=5876072

@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 21, 2020

Okay, GH actions for this PR is passing for both Scala 2.12.x and 2.13.x and I updated the docs for S3 in the README file.

Also, I went ahead and removed the Travis CI config file, hope that's okay with you. What do you think about re-enabling Codacy later on? I see it's easy to integrate it with GH actions: https://github.com/marketplace/actions/codacy-coverage-reporter

Gotta go now but if it sounds good to you feel free to merge (and squash the commits if you wish). CI build is green: cr-org#1

Copy link
Member

@semenodm semenodm left a comment

Choose a reason for hiding this comment

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

LGTM

@semenodm semenodm merged commit bf099a5 into laserdisc-io:master Jul 21, 2020
@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 22, 2020

🎉

@gvolpe gvolpe deleted the feature/aws-s3-v2 branch July 22, 2020 08:05
@gvolpe
Copy link
Collaborator Author

gvolpe commented Jul 22, 2020

Thanks for merging @semenodm ! Pull me in anytime if you need help either with the S3 module or with Github actions. With the rest I don't think I could help much since I only use S3 and I'm not really familiar with the other APIs...

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.

Split up S3 into its own module and update to v2
3 participants