Skip to content

Updated requirements to ranges - #946

Closed
clintonb wants to merge 2 commits into
masterfrom
clintonb/requirements-fix
Closed

Updated requirements to ranges#946
clintonb wants to merge 2 commits into
masterfrom
clintonb/requirements-fix

Conversation

@clintonb

Copy link
Copy Markdown

If we are going to read setup.py requirements from files, the files need to use ranges. Otherwise, we will end up with requirement conflicts when the main project wants to upgrade requirements.

@clintonb
clintonb force-pushed the clintonb/requirements-fix branch from 4d73711 to 9c47847 Compare November 21, 2016 16:17
@clintonb

Copy link
Copy Markdown
Author

This is necessary to support upgrading Django REST Framework for edx-platform. See https://github.com/edx/edx-platform/pull/13945.

@clintonb

Copy link
Copy Markdown
Author

@cahrens @efischer19 please review

@clintonb
clintonb force-pushed the clintonb/requirements-fix branch from c022991 to 8f6ad39 Compare November 21, 2016 16:33

@efischer19 efischer19 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems mostly fine to me, just a few questions about some removal changes.

Comment thread requirements/test.txt

git+https://github.com/edx/django-pyfs.git@1.0.3#egg=django-pyfs==1.0.3
git+https://github.com/edx/i18n-tools.git@56f048af9b6868613c14aeae760548834c495011#egg=i18n_tools
git+https://github.com/edx/edx-lint.git@v0.4.1#egg=edx_lint==0.4.1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why were these 2 removed? I know edx/i18n-tools is needed for translation updates at least. It probably should be updated to a more recent version like v0.3.4, and may not be in the right location, but removing it entirely seems like a problem. edx-lint I'm less concerned about.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Both of these are hosted on PyPI. I added them to the list above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I had missed that initially. Great!

Comment thread .travis.yml
branches:
only:
- master
- ora-staff-grading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For my own edification - what's the impact of this change? Just that Travis builds will run more regularly on non-master branches?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool, thanks!

Comment thread setup.py Outdated
setup(
name='ora2',
version='1.1.10',
version='1.1.11',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please leave this at the current version for this PR, and I'll take care of updating it with translations as part of the ORA release process. @cahrens, do you or your team have any work you'd like to release soon, or should I get this process started once this PR lands?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@efischer19 go ahead and start the release process-- I did merge something on Friday, so I will have something to verify.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay. Reverting.

@cahrens

cahrens commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

@clintonb Did you verify that the a11y and acceptance tests pass with these changes? Note that you will have to create a sandbox and run them manually, since they don't run as part of the travis build.

@clintonb
clintonb force-pushed the clintonb/requirements-fix branch from 8f6ad39 to 759accd Compare November 21, 2016 16:38
@clintonb

Copy link
Copy Markdown
Author

@cahrens I have not run those tests. Time permitting, I'll run them today and post back here.

@clintonb

Copy link
Copy Markdown
Author

Have you given any thought to integrating those tests here into this repo?

Comment thread requirements/test.txt Outdated
mock>=1.0.1,<2.0.0
moto>=0.3.1,<1.0.0
pep8>=1.7.0,<2.0.0
factory_boy>=2.6.1,3.0.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo-- missing <

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed.

@clintonb
clintonb force-pushed the clintonb/requirements-fix branch from 759accd to 3db29d9 Compare November 21, 2016 16:43
@cahrens

cahrens commented Nov 21, 2016

Copy link
Copy Markdown
Contributor

@clintonb The integration/a11y tests are in this repo. They just run through Jenkins jobs on master, instead of as part of the travis build.

See https://openedx.atlassian.net/wiki/display/TNL/ORA+FAQ#ORAFAQ-Runningacceptancetests

If we are going to read setup.py requirements from files, the files need to use ranges. Otherwise, we will end up with requirement conflicts when the main project wants to upgrade requirements.
@clintonb
clintonb force-pushed the clintonb/requirements-fix branch from 761400f to 413de84 Compare November 22, 2016 03:25
@efischer19

Copy link
Copy Markdown
Contributor

Implemented in #1023, containing the latest DRF updates as certified by the Platform team

@efischer19 efischer19 closed this Aug 3, 2017
@efischer19
efischer19 deleted the clintonb/requirements-fix branch August 3, 2017 20:30
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.

3 participants