Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add 'server_version' endpoint to admin API #4772

Merged

Conversation

jbweston
Copy link
Contributor

@jbweston jbweston commented Mar 1, 2019

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

Closes #3220

jbweston added 2 commits March 1, 2019 09:46
We will later need also to import 'register_servlets' from the
'login' module, so we un-pollute the namespace now to keep the
logical changes separate.
This is required because the 'Server' HTTP header is not always
passed through proxies.
@jbweston jbweston force-pushed the jbweston/server-version-api branch from 62fcdbd to f24721c Compare March 1, 2019 09:02
@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #4772 into develop will decrease coverage by 7.48%.
The diff coverage is 53.84%.

@@             Coverage Diff             @@
##           develop    #4772      +/-   ##
===========================================
- Coverage    75.13%   67.65%   -7.49%     
===========================================
  Files          340      340              
  Lines        34918    34931      +13     
  Branches      5722     5723       +1     
===========================================
- Hits         26237    23631    -2606     
- Misses        7067     9652    +2585     
- Partials      1614     1648      +34

@codecov
Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #4772 into develop will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #4772      +/-   ##
===========================================
- Coverage    75.13%   75.12%   -0.02%     
===========================================
  Files          340      340              
  Lines        34918    34996      +78     
  Branches      5722     5750      +28     
===========================================
+ Hits         26237    26289      +52     
- Misses        7067     7089      +22     
- Partials      1614     1618       +4

@jbweston
Copy link
Contributor Author

jbweston commented Mar 1, 2019

I'll admit the coverage report has me a little confused. I'm not sure how my changes can possibly produce a 7% coverage decrease in the whole codebase.

It seems to have un-stuck itself

@jbweston jbweston force-pushed the jbweston/server-version-api branch from f24721c to d3dcb64 Compare March 1, 2019 09:45
@@ -66,6 +69,25 @@ def on_GET(self, request, user_id):
defer.returnValue((200, ret))


class VersionServlet(ClientV1RestServlet):
PATTERNS = client_path_patterns("/admin/server_version")
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 guess a first point to make sure we agree on is what to call this endpoint.

server_version seems better than simply version IMO, because it is more explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I just realised I forgot to add documentation for this endpoint.

ret = {
'server_version': get_version_string(synapse),
'python_version': platform.python_version(),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then there's what should be returned. @hawkowl suggested to also return the Python version (IIRC) because we can, and because this is often a pain point for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is often a pain point for users.

i.e. that troubleshooting is harder without this information, so we may as well return it

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

this looks great, though please could you add a file in https://github.com/matrix-org/synapse/tree/develop/docs/admin_api to document it?

@jbweston
Copy link
Contributor Author

jbweston commented Mar 1, 2019

this looks great, though please could you add a file in https://github.com/matrix-org/synapse/tree/develop/docs/admin_api to document it?

For sure! I got distracted, but I'll do it now :)

Signed-off-by: Joseph Weston <[email protected]>
@erikjohnston
Copy link
Member

Thanks!

@erikjohnston erikjohnston merged commit 16c8b4e into matrix-org:develop Mar 5, 2019
@neilisfragile neilisfragile added z-p2 (Deprecated Label) A-Docs things relating to the documentation and removed A-Docs things relating to the documentation z-p2 (Deprecated Label) labels Mar 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants