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

requirements: Upgrade boto to boto3 #3706

Closed
wants to merge 1 commit into from
Closed

Conversation

sinwar
Copy link
Contributor

@sinwar sinwar commented Feb 17, 2017

Fixes #3490

@smarx
Copy link

smarx commented Feb 17, 2017

Automated message from Dropbox CLA bot

@sinwar, it looks like you've already signed the Dropbox CLA. Thanks!

@@ -26,6 +26,10 @@ backports.ssl-match-hostname==3.5.0.1

# Needed for S3 file uploads
boto==2.45.0
boto3==1.4.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we require boto because there is still no equivalent of boto.utils.get_instance_metadata() in boto3.
boto/boto3#313

@sinwar sinwar force-pushed the botoupgrade branch 2 times, most recently from d6da3da to 1a54637 Compare February 17, 2017 05:47
@@ -26,6 +26,10 @@ backports.ssl-match-hostname==3.5.0.1

# Needed for S3 file uploads
boto==2.45.0
botocore==1.5.7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we require boto because there is still no equivalent of boto.utils.get_instance_metadata() in boto3.
boto/boto3#313

# in situations where buckets don't exist, but that shouldn't be
# an issue for us.)
bucket = conn.get_bucket(bucket_name, validate=False)
return bucket
Copy link
Member

Choose a reason for hiding this comment

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

What's the story with removing this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://boto3.readthedocs.io/en/latest/guide/migrations3.html#accessing-a-bucket
boto3 doesn't automatically validate whether a bucket exists so in normal calls validate will be false and for validation we have to check manually as mentioned in above link.

@timabbott
Copy link
Member

@sinwar huge thanks for working on this, and sorry for the very delayed review!

I've been slow to review this I think because I'm worried about it potentially breaking things, because we don't have a good way to test the boto setup not in the production (only half of the existing S3 backend has tests, which is pretty scary for this level of change).

As a path forward, would you be up for working on bringing the test coverage for zerver/lib/upload.py up to 100% (at least for the S3 code path)? I'm imagining you could do this on a separate branch, since it'd be mostly just adding code to the test suite, and then we can rebase this onto master once that's done.

I think it'd be a good experience for you, the test coverage is highly valuable independent of this PR, and I think once we did that, merging this would be a lot less scary :).

@sinwar
Copy link
Contributor Author

sinwar commented Mar 6, 2017

@timabbott I will be up for this now but I haven't decide my GSoC proposal idea and I want to make a good proposal from now. I want to work with python, Django area. Can you suggest me any guidance for this so I can confidently do other work.
I can do affter my pending some other work.

@timabbott
Copy link
Member

Yeah, let's chat about GSoC on chat.zulip.org when convenient.

@zulipbot
Copy link
Member

Hello @sinwar, a Zulip maintainer reviewed this pull request over a week ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@zulipbot
Copy link
Member

Heads up @sinwar, we just merged some commits (latest: 510b7a0) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@timabbott
Copy link
Member

@sinwar can you rebase and update this PR? It'd be nice to merge it soon.

@zulipbot
Copy link
Member

zulipbot commented Jun 8, 2017

Hello @sinwar, a Zulip maintainer reviewed this pull request over a week ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

1 similar comment
@zulipbot
Copy link
Member

Hello @sinwar, a Zulip maintainer reviewed this pull request over a week ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@zulipbot
Copy link
Member

zulipbot commented Aug 1, 2017

Heads up @sinwar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@zulipbot-test
Copy link

Hello @sinwar, a Zulip maintainer reviewed this pull request over a week ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@zulipbot
Copy link
Member

Hello @sinwar, a Zulip maintainer reviewed this pull request over 7 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@zulipbot
Copy link
Member

Heads up @sinwar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@zulipbot
Copy link
Member

zulipbot commented Nov 21, 2017

Heads up @sinwar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@zulipbot
Copy link
Member

zulipbot commented Dec 4, 2017

Hello @sinwar, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@zulipbot-test
Copy link

Hello @sinwar, a Zulip maintainer reviewed this pull request over 10 days ago, but you haven't updated your pull request since. Please take a look at the requested changes and update your pull request accordingly.

Thank you for your valuable contributions to Zulip!

@timabbott
Copy link
Member

Closing this due to long inactivity.

@timabbott timabbott closed this Feb 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants