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.

Sign up for GitHub

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

Jump to bottom

BOM-1025 #436

Merged
merged 1 commit into from
Nov 14, 2019
Merged

BOM-1025 #436

merged 1 commit into from
Nov 14, 2019

Conversation

awais786
Copy link
Contributor

@awais786 awais786 commented Nov 11, 2019 •
edited
Loading

Adding tox for django20,21,22.
run make upgrade

@awais786 awais786 force-pushed the awais786/1025 branch 2 times, most recently from 804f9c3 to f65e54f Compare November 12, 2019 10:33
@awais786
Copy link
Contributor Author

@jmbowman make upgrade is failing for this repo.

There are incompatible versions in the resolved dependencies:
  python-dateutil==2.8.1 (from -r requirements/base.txt (line 15))
  python-dateutil<2.8.1,>=2.1 (from botocore==1.13.15->boto3==1.10.15->fs-s3fs==1.1.1->django-pyfs==2.0->-r requirements/django.in (line 7))

It seems boto is using python-dateutil to 2.8.0. You can see the discussion here.

awais786
awais786 commented Nov 12, 2019
View reviewed changes
requirements/base.in Outdated
@@ -11,3 +11,4 @@ simplejson
six
web-fragments
webob>=1.6.0
boto3<1.13.9 # latest boto release cap upper version of python-dateutil to 2.8.0. That's why adding a cap here for boto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

boto/botocore@00eade6
Add upper cap to avoid this release.

Copy link
Contributor

Choose a reason for hiding this comment

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

This constraint should go in requirements/constraints.txt, which still needs to be added to this repo.

@awais786 awais786 force-pushed the awais786/1025 branch 2 times, most recently from 5eb0609 to 45eae12 Compare November 12, 2019 13:47
jmbowman
jmbowman reviewed Nov 12, 2019
View reviewed changes
requirements/base.in Outdated
@@ -11,3 +11,4 @@ simplejson
six
web-fragments
webob>=1.6.0
boto3<1.13.9 # latest boto release cap upper version of python-dateutil to 2.8.0. That's why adding a cap here for boto
Copy link
Contributor

Choose a reason for hiding this comment

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

This constraint should go in requirements/constraints.txt, which still needs to be added to this repo.

requirements/doc.txt Outdated
@@ -4,49 +4,36 @@
#
# make upgrade
#
alabaster==0.7.12 # via sphinx
Copy link
Contributor

Choose a reason for hiding this comment

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

A lot of dependencies went missing in this file, please add --rebuild to the pip-compile commands in Makefile to avoid any caching of bad dependency data.

tox.ini Outdated
@@ -37,7 +35,8 @@ commands =

[testenv:quality]
deps =
Django>=1.11,<2.0
django111: Django>=1.11,<2.0
django20: Django>=2.0,<2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually run the quality checks with 2 different Django versions? Doesn't seem like we need two cases for this.

@awais786 awais786 force-pushed the awais786/1025 branch 3 times, most recently from 5288b98 to a077fa0 Compare November 13, 2019 11:00
@awais786 awais786 requested a review from jmbowman November 13, 2019 12:33
@awais786
Copy link
Contributor Author

@jmbowman you can review this PR.

jmbowman
jmbowman reviewed Nov 14, 2019
View reviewed changes
Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Just a few more minor questions and concerns, otherwise looks good.

requirements/django.txt
@@ -6,30 +6,24 @@
#
appdirs==1.4.3
backports.os==0.1.1
boto3==1.10.0 # via fs-s3fs
botocore==1.13.0 # via boto3, s3transfer
Copy link
Contributor

Choose a reason for hiding this comment

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

This time all the dependencies of boto3 vanished from this file; the --rebuild option to the pip-compile commands in Makefile is likely to fix this.

xblock/core.py Outdated
@@ -173,6 +173,7 @@ def load_tagged_classes(cls, tag, fail_silently=True):
if tag in class_._class_tags:
yield name, class_

# pylint: disable = pylint: disable=keyword-arg-before-vararg
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidental repetition of content in this comment?

xblock/runtime.py
@@ -612,6 +612,7 @@ def load_aside_type(self, aside_type):
"""
return XBlockAside.load_class(aside_type, select=self.select)

# pylint: disable = pylint: disable=keyword-arg-before-vararg
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question on the comments in this file.

@awais786
BOM-1025
f616db6 
Adding tox for django20,21,22
@awais786 awais786 merged commit d614064 into master Nov 14, 2019
@Ayub-Khan Ayub-Khan deleted the awais786/1025 branch December 11, 2019 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmbowman jmbowman jmbowman left review comments

@Ayub-Khan Ayub-Khan Ayub-Khan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@awais786 @jmbowman @Ayub-Khan