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

[BB-3375] Add boto3 as individual dependency #2882

Conversation

farhaanbukhsh
Copy link
Member

For now boto3 comes as a dependency of django-ses, but it is as well require to make use of S3 storage medium class.
Especially if you want to store media in S3 bucket. Something like:


DISCOVERY_MEDIA_STORAGE_BACKEND:
  DEFAULT_FILE_STORAGE: 'storages.backends.s3boto3.S3Boto3Storage'

JIRA tickets: Mention Jira ticket(s) here.

Screenshots: Always include screenshots if there is any change to the UI. Can be useful for people that are not reviewer but want to know what's going on.

Sandbox URL: TBD - sandbox is being provisioned (if needed).

Testing instructions:

Just dependency addition

Author notes and concerns:

This is just to make sure to use the latest storage with S3

Reviewers

@farhaanbukhsh farhaanbukhsh requested a review from a team as a code owner January 12, 2021 18:20
@openedx-webhooks
Copy link

Thanks for the pull request, @farhaanbukhsh! I've created OSPR-5357 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jan 12, 2021
@natabene
Copy link

@farhaanbukhsh Thank you for your contribution. Can you please look into resolving the conflict?

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Jan 13, 2021
@farhaanbukhsh farhaanbukhsh force-pushed the farhaan/bb-3375-fix-to-use-boto3 branch from a34a038 to 71d9c83 Compare January 13, 2021 16:02
In order to support differnt storage medium and class especially S3
boto3 should be a independent dependency.

Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>
@farhaanbukhsh farhaanbukhsh force-pushed the farhaan/bb-3375-fix-to-use-boto3 branch from 5a47b9f to a2a2e9b Compare January 14, 2021 17:04
@farhaanbukhsh
Copy link
Member Author

@natabene I have resolved the conflicts

Copy link

@shimulch shimulch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

  • I read through the code
  • [N/A] I checked for accessibility issues
  • [N/A] Includes documentation
  • [N/A] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jan 15, 2021
@natabene
Copy link

@jmyatt Here is another OSPR for your squad, could please review?

xss-utils==0.2.0 # via -r requirements/base.in
zope.event==4.5.0 # via gevent
zope.interface==5.2.0 # via gevent
algoliasearch-django==1.7.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa what happened with this formatting? Is this a different (newer?) version of pip-compile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @mikix thanks for replying I think pip-compile is normally behaving like that I saw the PR https://github.com/edx/course-discovery/pull/2879/files which also have the same formatting.

I am using:

➜ pip-compile --version
pip-compile, version 5.5.0

Let me know if I need to use any other command or pass some option. 😓

@mikix
Copy link
Contributor

mikix commented Jan 15, 2021

If a changed config is introducing an S3 dependency, why not just also install boto3 as part of those customizations?

Like... if someone out there wants to use Azure to store media, it wouldn't feel appropriate to add an azure dep in the main code.

@farhaanbukhsh
Copy link
Member Author

If a changed config is introducing an S3 dependency, why not just also install boto3 as part of those customizations?

Like... if someone out there wants to use Azure to store media, it wouldn't feel appropriate to add an azure dep in the main code.

@mikix that's a valid point but then how can one install selected dependency for course-discovery while deploying? Also boto3 anyhow is getting introduced through django-ses.

@mikix
Copy link
Contributor

mikix commented Jan 15, 2021

how can one install selected dependency for course-discovery while deploying?

Good question. I'm not super familiar with the instructions for deploying open edx. But I assume there's some scripting one could add in there or something in edx/configuration you could override?

In our docs about it, we talk like running pip would be no problem during the deploy process:
https://edx.readthedocs.io/projects/edx-installing-configuring-and-running/en/latest/configuration/install_xblock.html

Also boto3 anyhow is getting introduced through django-ses.

Well true. But that reminds me - why this change, if boto3 is coming through anyway?

@farhaanbukhsh
Copy link
Member Author

how can one install selected dependency for course-discovery while deploying?

Good question. I'm not super familiar with the instructions for deploying open edx. But I assume there's some scripting one could add in there or something in edx/configuration you could override?

I couldn't find the options when I looked through the playbook.

In our docs about it, we talk like running pip would be no problem during the deploy process:
https://edx.readthedocs.io/projects/edx-installing-configuring-and-running/en/latest/configuration/install_xblock.html

It's not scalable to manually run pip in each machine

Also boto3 anyhow is getting introduced through django-ses.

Well true. But that reminds me - why this change, if boto3 is coming through anyway?

So what we were trying to do was make media asset to use a S3 backend and in order to do that we need to use 'storages.backends.s3boto3.S3Boto3Storage' and for that boto3 was required. All of these were being done on Juniper and the version of django-ses that is pinned doesn't have boto3 dependency so we ended up adding boto3 in our fork to help the backend run smoothly.

We thought that people should not face this issue, right now it's okay because boto3 anyhow come as a side-effect.

So if we explicitly keep it out there it would be better.

And I find your point of adding the backend supporting dependency in a different way. But I couldn't find any other way. So either we add a way through edx/configuration or we explicitly mention and document what one needs to do to support different storage solution.

What do you feel about it?

Thanks a ton @mikix for replying 😄 means a lot.

@mikix
Copy link
Contributor

mikix commented Jan 19, 2021

It's not scalable to manually run pip in each machine

Yeah not manually. I was just saying using pip as part of some deploy script. I honestly don't know enough about open edx deployment to be able to point to where you'd do this. But I know it's possible. edx.org installs non-default xblocks and stuff. So it should be a part of the sequence somewhere.

Maybe ask in the open edx discourse, to find someone more knowledgable than me, on how to install custom dependencies during deploy? That could clear our questions up.

@nedbat
Copy link
Contributor

nedbat commented Jan 27, 2021

I don't understand all the factors at work here, but it sounds like we shouldn't add a dependency in case some operators want to use it with their customized settings. If it's difficult to customize the set of extra packages installed for a particular service, we should work on improving that. We won't be able to add all the dependencies that people might need.

@farhaanbukhsh
Copy link
Member Author

@nedbat @mikix

It totally makes sense to not add the dependency of a 3rd party storage service in the main repo. I struggled to find a way to install a 3rd party dependency (Aws s3 or Azure) for IDA's (course-discovery here) in edx-configuration repo as well. So if you folks know of a way to do it and if you can point out to me that will be great else we should file a ticket to add it I believe. I hope I am able convey my pain points. Thanks again for spending time on this and for answering my question. :) Means a lot to me ❤️

I am closing this PR for now.

@openedx-webhooks
Copy link

@farhaanbukhsh Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@mikix
Copy link
Contributor

mikix commented Feb 1, 2021

@farhaanbukhsh unfortunately, I only ever really deal with open edx installations from a devstack perspective - I don't have too much context from an operational perspective. I think you should ask on the forums: https://discuss.openedx.org/

@nedbat
Copy link
Contributor

nedbat commented Feb 1, 2021

@farhaanbukhsh I would ask in Discourse how people deal with these sorts of needs.

@farhaanbukhsh
Copy link
Member Author

@nedbat @mikix https://discuss.openedx.org/t/how-to-install-private-dependencies-while-deploying-idas/4177

I have put up a question on discourse thanks for guiding me here 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U rejected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants