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

Check if port is occupied before starting Kolibri #3302

Merged
merged 2 commits into from
Mar 8, 2018
Merged

Check if port is occupied before starting Kolibri #3302

merged 2 commits into from
Mar 8, 2018

Conversation

lyw07
Copy link
Contributor

@lyw07 lyw07 commented Mar 6, 2018 •
edited
Loading

Summary

This PR is to implement one of the sanity checks before starting Kolibri.
In this PR, before launching Kolibri:

  1. I check if a Kolibri server has been running using server.get_status(). If a Kolibri server is running, then users should not be able to run kolibri start again. The program will exit with an error message.
  2. If there is no Kolibri server running currently, then I continue to check if the port is occupied, since it could be possible that something other than Kolibri occupies the port. If the port is occupied, the program will exit with an error message.

Additionally, I added a line in the function stop() of cli.py to indicate that kolibri server has been stopped.

Reviewer guidance

  1. Start a kolibri instance using installers other than the ones from this PR
  2. Get an installer from this PR and try to start another kolibri instance

Running with two pex files could be an easier approach.

References

Issues:
#3285
#2867
#3048

Contributor Checklist

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • Contributor has fully tested the PR manually
  • Screenshots of any front-end changes are in the PR description
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')

Reviewer Checklist

  • Automated test coverage is satisfactory
  • Reviewer has fully tested the PR manually
  • PR has been tested for accessibility regressions
  • External dependencies files were updated (yarn and pip)
  • Documentation is updated
  • Link to diff of internal dependency change is included
  • CHANGELOG.rst is updated for high-level changes
  • Contributor is in AUTHORS.rst

@lyw07 lyw07 added the work-in-progress Not ready for review label Mar 6, 2018
@lyw07 lyw07 added this to the 0.8.0 milestone Mar 6, 2018
@codecov
Copy link

codecov bot commented Mar 6, 2018 •
edited
Loading

Codecov Report

Merging #3302 into release-v0.8.x will increase coverage by 0.01%.
The diff coverage is 71.79%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-v0.8.x   #3302      +/-   ##
=================================================
+ Coverage           74.48%   74.5%   +0.01%     
=================================================
  Files                 209     210       +1     
  Lines                8220    8251      +31     
  Branches             1000    1002       +2     
=================================================
+ Hits                 6123    6147      +24     
- Misses               1897    1904       +7     
  Partials              200     200
Impacted Files Coverage Δ
kolibri/utils/cli.py 69.31% <68.75%> (+0.56%) ⬆️
kolibri/utils/sanity_checks.py 73.91% <73.91%> (ø)

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 8e7de99...1a84fae. Read the comment docs.

@lyw07 lyw07 force-pushed the check_port branch 2 times, most recently from 863516b to 545a1dc Compare March 6, 2018 18:45
@lyw07 lyw07 added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Mar 6, 2018
indirectlylit
indirectlylit reviewed Mar 7, 2018
View reviewed changes
kolibri/utils/cli.py Outdated
@@ -314,6 +312,7 @@ def stop():
pid, __, __ = server.get_status()
server.stop(pid=pid)
stopped = True
logger.info("Kolibri server has been successfully stoppped.")

This comment was marked as spam.

Sign in to view

aronasorman
aronasorman approved these changes Mar 7, 2018
View reviewed changes
Copy link
Collaborator

@aronasorman aronasorman left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise this looks good.

kolibri/utils/sanity_checks.py Outdated
logger.error(
"Port {} is occupied.\n"
"Please check that you do not have other processes "
"running on this port and try again.\n".format(port))

This comment was marked as spam.

Sign in to view

kolibri/utils/sanity_checks.py Outdated
pid, listen_address, listen_port = get_status()
logger.error(
"There is another Kolibri server running. "
"Please use `kolibri stop` and try again.")

This comment was marked as spam.

Sign in to view

@indirectlylit
Copy link
Contributor

looks great!

image

@indirectlylit indirectlylit merged commit 343b94f into learningequality:release-v0.8.x Mar 8, 2018
benjaoming
benjaoming reviewed Mar 8, 2018
View reviewed changes
kolibri/utils/sanity_checks.py
"There is another Kolibri server running. "
"Please use `kolibri stop` and try again."
)
sys.exit(1)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@indirectlylit indirectlylit added changelog Important user-facing changes and removed TODO: needs review Waiting for review labels Mar 14, 2018
@lyw07 lyw07 deleted the check_port branch March 26, 2019 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indirectlylit indirectlylit indirectlylit left review comments

@benjaoming benjaoming benjaoming left review comments

@aronasorman aronasorman aronasorman approved these changes

@jamalex jamalex Awaiting requested review from jamalex

Assignees
No one assigned
Labels
changelog Important user-facing changes
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lyw07 @indirectlylit @aronasorman @benjaoming