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

feat: Add endpoint for server version #6524

Merged
merged 12 commits into from Oct 19, 2019
Merged

feat: Add endpoint for server version #6524

merged 12 commits into from Oct 19, 2019

Conversation

adhibhuta
Copy link
Contributor

@adhibhuta adhibhuta commented Oct 15, 2019 •
edited

Fixes #6470

Short description of what this resolves:

Added an API which shows the current version of the server

Changes proposed in this pull request:

  • Added a new endpoint for server version

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@auto-label auto-label bot added the feature label Oct 15, 2019
houndci-bot
houndci-bot reviewed Oct 15, 2019
View reviewed changes
app/api/server_version.py Outdated
@staticmethod
@server_version_route.route('/server_version')
def version():
return jsonify(server_version = SERVER_VERSION)
Copy link

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

app/api/server_version.py Outdated

server_version_route = Blueprint('server_version', __name__, url_prefix='/v1')

class ServerVersion:
Copy link

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

app/api/server_version.py Outdated
@@ -0,0 +1,13 @@
from flask import jsonify, Blueprint

from app.api.bootstrap import api_v1 as app
Copy link

Choose a reason for hiding this comment

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

'app.api.bootstrap.api_v1 as app' imported but unused

app/__init__.py Outdated
@@ -48,6 +48,7 @@
from app.views.redis_store import redis_store
from app.views.celery_ import celery
from app.templates.flask_ext.jinja.filters import init_filters
from app.api.server_version import ServerVersion
Copy link

Choose a reason for hiding this comment

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

'app.api.server_version.ServerVersion' imported but unused

@codecov
Copy link

codecov bot commented Oct 15, 2019 •
edited

Codecov Report

Merging #6524 into development will increase coverage by 0.24%.
The diff coverage is 87.5%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6524      +/-   ##
===============================================
+ Coverage         64.9%   65.15%   +0.24%     
===============================================
  Files              294      296       +2     
  Lines            15199    15292      +93     
===============================================
+ Hits              9865     9963      +98     
+ Misses            5334     5329       -5
Impacted Files Coverage Δ
app/__init__.py 87.69% <100%> (-0.21%) ⬇️
app/api/server_version.py 83.33% <83.33%> (ø)
app/factories/session.py 100% <0%> (ø) ⬆️
app/factories/speaker.py 100% <0%> (ø) ⬆️
app/factories/microlocation.py 100% <0%> (ø) ⬆️
app/models/utils.py 41.37% <0%> (ø)
app/api/helpers/scheduled_jobs.py 26.56% <0%> (+3.12%) ⬆️
...s/all/integration/api/helpers/test_pentabarfxml.py 98.68% <0%> (+3.22%) ⬆️
... and 2 more

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 95eb0cb...3dee4df. Read the comment docs.

@adhibhuta
Copy link
Contributor Author

@prateekj117 @iamareebjamal Please review.

iamareebjamal
iamareebjamal reviewed Oct 15, 2019
View reviewed changes
app/api/server_version.py Outdated

class ServerVersion:
@staticmethod
@server_version_route.route('/server_version')
Copy link
Member

Choose a reason for hiding this comment

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

Make it /info

This JSON should be the output

{
  "build": {
    "version": "2.2.3-SNAPSHOT",
    "time": "2019-10-15T07:26:58.080Z"
  }
}

@adhibhuta
Copy link
Contributor Author

I have made the changes, @iamareebjamal please review. The output json is of the following format:

{ "build": { "time": "2019-10-17 08:28:57.087928", "version": "2.2.3-SNAPSHOT" } }

iamareebjamal
iamareebjamal reviewed Oct 17, 2019
View reviewed changes
app/api/server_version.py Outdated

server_version_route = Blueprint('server_version', __name__, url_prefix='/v1')
info_route = Blueprint('info', __name__, url_prefix='/v1')
_build = {'time': str(datetime.now()), 'version': SERVER_VERSION}
Copy link
Member

Choose a reason for hiding this comment

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

time should be the time of release

Copy link
Member

Choose a reason for hiding this comment

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

Since it'll be a little bit complex for you, ignore the time field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iamareebjamal sure no issues. Although if you give me some hint about the vx pipeline I can try to figure out something.

Copy link
Member

Choose a reason for hiding this comment

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

vx pipeline?

Copy link
Member

Choose a reason for hiding this comment

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

Simplest solution is to invoke git to find last commit time

Copy link
Member

Choose a reason for hiding this comment

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

But we need to finalize the release today, so let it be and do it in a future release

iamareebjamal
iamareebjamal reviewed Oct 17, 2019
View reviewed changes
app/__init__.py Outdated Show resolved Hide resolved
@iamareebjamal iamareebjamal self-assigned this Oct 17, 2019
iamareebjamal
iamareebjamal reviewed Oct 18, 2019
View reviewed changes
app/__init__.py Outdated
@@ -133,6 +133,7 @@ def create_app():

# development api
with app.app_context():
from app.api.server_version import info_route
Copy link
Member

Choose a reason for hiding this comment

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

Move to last

iamareebjamal
iamareebjamal previously approved these changes Oct 18, 2019
View reviewed changes
prateekj117
prateekj117 previously approved these changes Oct 18, 2019
View reviewed changes
@fossasia fossasia deleted a comment Oct 18, 2019
@fossasia fossasia deleted a comment Oct 18, 2019
iamareebjamal
iamareebjamal reviewed Oct 18, 2019
View reviewed changes
app/__init__.py Outdated
@@ -167,6 +167,8 @@ def create_app():
app.register_blueprint(admin_blueprint)
app.register_blueprint(alipay_blueprint)
app.register_blueprint(admin_misc_routes)
app.register_blueprint(import_routes)
Copy link
Member

Choose a reason for hiding this comment

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

Revert this to where it was

@fossasia fossasia deleted a comment Oct 18, 2019
iamareebjamal
iamareebjamal reviewed Oct 18, 2019
View reviewed changes
app/__init__.py Outdated
app.register_blueprint(celery_routes)
app.register_blueprint(auth_routes)
app.register_blueprint(event_statistics)
app.register_blueprint(info_route)
Copy link
Member

Choose a reason for hiding this comment

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

Still at the wrong place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved the /info_route and /import_route to its original place. Do you want me to put /info_route end?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

info_route had no original place as it's a new route and should be placed in the end

@fossasia fossasia deleted a comment Oct 19, 2019
@iamareebjamal iamareebjamal merged commit 75229f0 into fossasia:development Oct 19, 2019
@fossasia fossasia deleted a comment Oct 19, 2019
@adhibhuta
Copy link
Contributor Author

@iamareebjamal @prateekj117 This is the first Open Source PR I have ever done in my life. Thanks guys for being so patient. You guys are awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@iamareebjamal iamareebjamal iamareebjamal approved these changes

@prateekj117 prateekj117 prateekj117 left review comments

Assignees

@iamareebjamal iamareebjamal

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add server version and an API to show it
4 participants
@adhibhuta @iamareebjamal @houndci-bot @prateekj117