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

Set KOLIBRI_LISTEN_PORT default to 8080 and use it in cli.start() #1729

Merged

Conversation

benjaoming
Copy link
Contributor

@benjaoming benjaoming commented Jun 26, 2017

Summary

FIxes #1724

TODO

  • Have tests been written for the new code?
  • Has documentation been written/updated?
  • New dependencies (if any) added to requirements file
  • Add an entry to CHANGELOG.rst
  • Add yourself it AUTHORS.rst if you don't appear there

Issues addressed

#1724

Documentation

Looking through docs to see if there's any changes to be made...

@benjaoming benjaoming added this to the 0.5.0 milestone Jun 26, 2017
@benjaoming benjaoming requested a review from jamalex June 26, 2017 14:15
@codecov-io
Copy link

codecov-io commented Jun 26, 2017

Codecov Report

Merging #1729 into release-v0.5.x will increase coverage by 0.1%.
The diff coverage is 53.84%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-v0.5.x    #1729     +/-   ##
=================================================
+ Coverage           73.73%   73.83%   +0.1%     
=================================================
  Files                 152      152             
  Lines                5516     5522      +6     
  Branches              650      654      +4     
=================================================
+ Hits                 4067     4077     +10     
+ Misses               1337     1334      -3     
+ Partials              112      111      -1
Impacted Files Coverage Δ
kolibri/deployment/default/settings/base.py 100% <ø> (ø) ⬆️
...libri/deployment/default/settings/postgres_test.py 100% <ø> (ø) ⬆️
kolibri/core/webpack/test/base.py 100% <100%> (ø) ⬆️
kolibri/utils/server.py 43.33% <20%> (+1.31%) ⬆️
kolibri/utils/cli.py 66.51% <69.23%> (+1.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d97ecd6...31a56a8. Read the comment docs.

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev386.exe

Python packages
Pex: kolibri-v0.4.0-386-g3f4ebfe.pex
Whl file: kolibri-0.4.0.dev386-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev386.zip
Tar file: kolibri-0.4.0.dev386.tar.gz

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev387.exe

Python packages
Pex: kolibri-v0.4.0-387-g43cb4c2.pex
Whl file: kolibri-0.4.0.dev387-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev387.zip
Tar file: kolibri-0.4.0.dev387.tar.gz

@benjaoming
Copy link
Contributor Author

benjaoming commented Jun 26, 2017

Looking into a few things to understand the loss of test coverage, added a more deliberate test of update_channel_metadata_cache although it doesn't actually check that data is correctly annotated :/ I'm not sure if that sort of coverage is super nice, but alternatively, I'd suggest to open an issue about it.

@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 43cb4c2 to 399660c Compare June 26, 2017 16:16
@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev387.exe

Python packages
Pex: kolibri-v0.4.0-387-g399660c.pex
Whl file: kolibri-0.4.0.dev387-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev387.zip
Tar file: kolibri-0.4.0.dev387.tar.gz

@benjaoming benjaoming changed the base branch from develop to release-v0.5.x June 27, 2017 13:05
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Looks good, just need to resolve merge conflict on the CHANGELOG.

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev443.exe

Python packages
Pex: kolibri-v0.4.0-443-g47bbff7.pex
Whl file: kolibri-0.4.0.dev443-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev443.zip
Tar file: kolibri-0.4.0.dev443.tar.gz

@rtibbles
Copy link
Member

And another merge conflict resolved!

@rtibbles
Copy link
Member

Except I broke everything. Sadness :(

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev450.exe

Python packages
Pex: kolibri-v0.4.0-450-ge7fe035.pex
Whl file: kolibri-0.4.0.dev450-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev450.zip
Tar file: kolibri-0.4.0.dev450.tar.gz

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev451.exe

Python packages
Pex: kolibri-v0.4.0-451-gaf15a91.pex
Whl file: kolibri-0.4.0.dev451-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev451.zip
Tar file: kolibri-0.4.0.dev451.tar.gz

@benjaoming
Copy link
Contributor Author

Working on it now!

@eduard-buildbot
Copy link

Build Artifacts

Kolibri Installers
Windows Installer: KolibriSetup-0.4.0.dev452.exe

Python packages
Pex: kolibri-v0.4.0-452-g6c77cd8.pex
Whl file: kolibri-0.4.0.dev452-py2.py3-none-any.whl
Zip file: kolibri-0.4.0.dev452.zip
Tar file: kolibri-0.4.0.dev452.tar.gz

@benjaoming
Copy link
Contributor Author

Some strange stuff going on with the postgres tests:

kolibri/utils/tests/test_cli.py:73: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
kolibri/utils/cli.py:237: in start
    update()
kolibri/utils/cli.py:211: in update
    call_command("migrate", interactive=False, database="ormq")
.tox/postgres/lib/python2.7/site-packages/django/core/management/__init__.py:119: in call_command
    return command.execute(*args, **defaults)
.tox/postgres/lib/python2.7/site-packages/django/core/management/base.py:399: in execute
    output = self.handle(*args, **options)
.tox/postgres/lib/python2.7/site-packages/django/core/management/commands/migrate.py:66: in handle
    connection = connections[db]
.tox/postgres/lib/python2.7/site-packages/django/db/utils.py:209: in __getitem__
    self.ensure_defaults(alias)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <django.db.utils.ConnectionHandler object at 0x7fa9760f3050>
alias = 'ormq'
    def ensure_defaults(self, alias):
        """
            Puts the defaults into the settings dictionary for a given connection
            where no settings is provided.
            """
        try:
            conn = self.databases[alias]
        except KeyError:
>           raise ConnectionDoesNotExist("The connection %s doesn't exist" % alias)
E           ConnectionDoesNotExist: The connection ormq doesn't exist

@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 6c77cd8 to 277347d Compare July 6, 2017 22:04
@benjaoming
Copy link
Contributor Author

Rebased and force-pushed. Please have patience.. will figure out what's up with the postgres test.

@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 277347d to 8f09f11 Compare July 17, 2017 20:28
@benjaoming
Copy link
Contributor Author

Rebased to upstream to resolve conflicts, seems ready! @rtibbles

@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 8f09f11 to 408037b Compare July 17, 2017 21:01
@benjaoming
Copy link
Contributor Author

benjaoming commented Jul 17, 2017

Seemed to still fail in the postgres tests (kolibri/core/webpack/test/test_webpack_tags.py::TestHook: cannot collect test class 'TestHook' because it has a __init__ constructor), so I changed some things

Ref of build fail: https://travis-ci.org/learningequality/kolibri/jobs/254600284

@@ -61,6 +61,12 @@ def start(port=8080):
with open(PID_FILE, 'w') as f:
f.write("%d\n%d" % (os.getpid(), port))

# TODO: not sure where it should ideally be located. Question is if it
# should be run every time the server is started or if it depends on some
# kind of state change.

This comment was marked as spam.

This comment was marked as spam.

@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 37e74f3 to 2ed42cf Compare July 24, 2017 12:49
@benjaoming benjaoming force-pushed the bug/port-env-setting branch from 2ed42cf to 31a56a8 Compare July 24, 2017 13:12
@benjaoming
Copy link
Contributor Author

Notice that I actually improved test coverage even though in an area where it was already very low :)

@benjaoming
Copy link
Contributor Author

@jamalex @rtibbles please merge at will, I have nothing more to add here, will pick up on test coverage as #1595 is implemented.

@benjaoming
Copy link
Contributor Author

The PR also removes creation of a legacy ormq database which belonged to django-q.

@rtibbles rtibbles merged commit 2a66512 into learningequality:release-v0.5.x Jul 24, 2017
@benjaoming benjaoming deleted the bug/port-env-setting branch August 1, 2017 13:13
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.

5 participants