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

Add initial support for S3 active storage #77

Merged
merged 2 commits into from
May 9, 2023
Merged

Add initial support for S3 active storage #77

merged 2 commits into from
May 9, 2023

Conversation

markgoddard
Copy link

In activestorage/config.py:

  • Set USE_S3 = True
  • Set S3_* to appropriate values for your setup

@valeriupredoi valeriupredoi added the enhancement New feature or request label May 4, 2023
Copy link
Collaborator

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

Hi @markgoddard - this is great, very many thanks! I've added a few review observations, please - I can plug in the changes, if you don't have time to do them. But one thing we need is a few test cases - am not very familiar with using the S3 interface for CI testing, so I might need your help there. If you could post one or two example tests then I can probably expand on those 👍 🍺

setup.py Outdated Show resolved Hide resolved
activestorage/s3.py Show resolved Hide resolved
activestorage/active.py Show resolved Hide resolved
activestorage/active.py Outdated Show resolved Hide resolved
Copy link
Author

@markgoddard markgoddard 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 the review.

setup.py Outdated Show resolved Hide resolved
activestorage/active.py Outdated Show resolved Hide resolved
activestorage/active.py Show resolved Hide resolved
activestorage/active.py Outdated Show resolved Hide resolved
activestorage/active.py Show resolved Hide resolved
activestorage/s3.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@markgoddard
Copy link
Author

Hi @markgoddard - this is great, very many thanks! I've added a few review observations, please - I can plug in the changes, if you don't have time to do them. But one thing we need is a few test cases - am not very familiar with using the S3 interface for CI testing, so I might need your help there. If you could post one or two example tests then I can probably expand on those +1 beer

We should be able to use the existing test cases, with USE_S3 = True and S3 active storage and minio running. It would be similar to the compliance tests in our CI: https://github.com/stackhpc/s3-active-storage-rs/blob/c62d757e5fef4861e91f4f8f064bf4abb8fc09b7/.github/workflows/pull-request.yml#L25

In activestorage/config.py:

* Set USE_S3 = True
* Set S3_* to appropriate values for your setup
@valeriupredoi
Copy link
Collaborator

Hi @markgoddard - this is great, very many thanks! I've added a few review observations, please - I can plug in the changes, if you don't have time to do them. But one thing we need is a few test cases - am not very familiar with using the S3 interface for CI testing, so I might need your help there. If you could post one or two example tests then I can probably expand on those +1 beer

We should be able to use the existing test cases, with USE_S3 = True and S3 active storage and minio running. It would be similar to the compliance tests in our CI: https://github.com/stackhpc/s3-active-storage-rs/blob/c62d757e5fef4861e91f4f8f064bf4abb8fc09b7/.github/workflows/pull-request.yml#L25

great! Cheers, let me do that then - plug a few tests here, then I'll merge 👍

Copy link
Collaborator

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

very many thanks @markgoddard - I'll plug in a couple tests based on your examples, then merge. Next step will be to update #47 to hold the S3 configuration in a yaml file rather than a Py module, and will ping you there for a review, if that's OK with you. Also, if that's OK with you, I'll add you as a contributor to this repo (make things easy for GitHub communications) 🍺

@markgoddard
Copy link
Author

very many thanks @markgoddard - I'll plug in a couple tests based on your examples, then merge. Next step will be to update #47 to hold the S3 configuration in a yaml file rather than a Py module, and will ping you there for a review, if that's OK with you. Also, if that's OK with you, I'll add you as a contributor to this repo (make things easy for GitHub communications) beer

Perfect, thanks

@valeriupredoi valeriupredoi merged commit 3332d12 into NCAS-CMS:main May 9, 2023
@valeriupredoi
Copy link
Collaborator

just merged this, so I don't have to faff around with your fork - gonna open a PR now to plug in some tests. Now that you are a contributor here, you can just open vanilla PRs w/o having to go through forks, @markgoddard - and many thanks again for your contributions 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants