Problem: Users trying to use an incompatible version of Tendermint by muawiakh · Pull Request #2541 · bigchaindb/bigchaindb

muawiakh · 2018-09-13T14:43:58Z

Solution

Hard-wire the supported Tendermint version(s) right in the code of BigchainDB Server. Check the version of Tendermint and disconnect if Tendermint version is an unsupported one.
Expose a CLI command bigchaindb tendermint-version to display the supported Tendermint versions.
PR also takes care the long list of warnings we get when we run tests.
- Updated deprecated pymongo methods
- Do not call pytest fixtures directly.
Also added the doc for the new cli command

Issues Resolved

Resolves #2525

- depracated pymongo methods - direct use of fixtures

docs/server/source/server-reference/bigchaindb-cli.md

tests/backend/localmongodb/test_queries.py

codecov-io · 2018-09-13T15:02:30Z

Codecov Report

Merging #2541 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2541      +/-   ##
==========================================
+ Coverage   93.64%   93.67%   +0.02%     
==========================================
  Files          44       44              
  Lines        2582     2593      +11     
==========================================
+ Hits         2418     2429      +11     
  Misses        164      164

kansi

It would be good to have a test which passes unsupported Tendermint version and asserts system exit

ldmberman · 2018-09-13T15:06:14Z

bigchaindb/core.py

+                             f" Currently, BigchainDB only supports {__tm_supported_versions__} .Exiting!")
+                sys.exit(1)
+        except AttributeError:
+            logger.debug("Skipping Tendermint version check.")


According to the docs, the version field is not optional, so I would as well panic when there is no version.

ldmberman · 2018-09-13T15:10:08Z

bigchaindb/utils.py

+    :type running_tm_ver: str
+    :return: True/False depending on the compatability with BigchainDB server
+    :rtype: bool
+    """


A nitpick:

if not tm_ver: return False for ver in __tm_supported_versions__: if version.parse(ver) == version.parse(tm_ver[0]): return True return False

Also, in this version it does not fail if the first match fails but only if it does not match any version in the list.

ldmberman · 2018-09-13T15:21:49Z

bigchaindb/commands/bigchaindb.py

+        "description": "BigchainDB supports the following Tendermint version(s)",
+        "tendermint": [],
+    }
+    [supported_tm_ver["tendermint"].append(ver) for ver in __tm_supported_versions__]


It may be slightly more concise:

supported_tm_ver = { 'description': 'BigchainDB supports the following Tendermint version(s)', 'tendermint': __tm_supported_versions__ }

On a side note, PEP8 recommends to stick to a single type of quotes, and we generally use '.

ttmc

This pull request is doing two different things so it really should have been two pull requests. I'm not going to ask you to split this pull request apart, just to be mindful of that principle in future pull requests.

ttmc · 2018-09-13T15:11:02Z

bigchaindb/core.py

+                logger.info(f"Tendermint version: {request.version}")
+            else:
+                logger.error(f"Error: Tendermint version {request.version} not supported."
+                             f" Currently, BigchainDB only supports {__tm_supported_versions__} .Exiting!")


The spacing is slightly off near the end of this line. It should end with }. Exiting!")

ttmc · 2018-09-13T15:20:55Z

bigchaindb/core.py


+        # Check if BigchainDB supports the Tendermint version
+        try:
+            if check_tendermint_version(request.version):


This function (currently named check_tendermint_version()) returns a Boolean, so it could be renamed so that lines like this read more like plain English, e.g.

if tendermint_version_is_compatible(request.version):

In other words, I'm suggesting renaming the function check_tendermint_version() as tendermint_version_is_compatible().

ttmc · 2018-09-13T15:22:17Z

bigchaindb/core.py

+                             f" Currently, BigchainDB only supports {__tm_supported_versions__} .Exiting!")
+                sys.exit(1)
+        except AttributeError:
+            logger.debug("Skipping Tendermint version check.")


Maybe we should say why the Tendermint version check is being skipped: apparently the request from Tendermint didn't include a version attribute.

ttmc · 2018-09-13T15:33:23Z

bigchaindb/utils.py

+    :return: True/False depending on the compatability with BigchainDB server
+    :rtype: bool
+    """
+    tm_ver = running_tm_ver.split('-')


Maybe put a comment with an example value of running_tm_ver so it's clear why you're doing this split.

- neatpicks - Add tests for sys exit

- Update name of tm version check function - some typos

ldmberman · 2018-09-14T10:11:22Z

bigchaindb/core.py


        self.abort_if_abci_chain_is_not_synced()

+        # Check if BigchainDB supports the Tendermint version


One more nitpick:

if not hasattr(request, 'version') or not tendermint_version_is_compatible(request.version): logger.error('Unsupported Tendermint version: {getattr(request, "version", "no version")} ..') sys.exit(1)

A slight correction:

not (hasattr(request, 'version') and tendermint_version_is_compatible(request.version))

ldmberman · 2018-09-14T10:12:44Z

bigchaindb/utils.py

+    for ver in __tm_supported_versions__:
+        if version.parse(ver) == version.parse(tm_ver[0]):
+            return True
+    else:


return False belongs to the outer scope, not to if-else.

- neatpicks

muawiakh added 7 commits September 13, 2018 11:37

Problem: No way to check Tendermint comptability with BigchainDB server

41b35bb

Add docs for bigchaindb tendermint-version

2105cdf

Fix flake8 errors

31eaeaa

Merge branch 'master' into tm_version_compatible

73352c8

Fix the long list of warnings from test run

c431e22

- depracated pymongo methods - direct use of fixtures

Fix flake8 again :)

a7463d8

Merge branch 'master' into tm_version_compatible

8e8461b

muawiakh requested review from kansi, ttmc, vrde and z-bowen September 13, 2018 14:43

kansi reviewed Sep 13, 2018

View reviewed changes

docs/server/source/server-reference/bigchaindb-cli.md Outdated Show resolved Hide resolved

kansi reviewed Sep 13, 2018

View reviewed changes

tests/backend/localmongodb/test_queries.py Show resolved Hide resolved

Add example for new cli command

42af61b

kansi suggested changes Sep 13, 2018

View reviewed changes

ldmberman reviewed Sep 13, 2018

View reviewed changes

ttmc suggested changes Sep 13, 2018

View reviewed changes

muawiakh added 2 commits September 13, 2018 19:30

Address comments I

e5d725a

- neatpicks - Add tests for sys exit

Address comments II

01213e3

- Update name of tm version check function - some typos

ttmc approved these changes Sep 13, 2018

View reviewed changes

Fix import for tm version check method

de11366

ldmberman reviewed Sep 14, 2018

View reviewed changes

Address comments III

ae49295

- neatpicks

ldmberman approved these changes Sep 14, 2018

View reviewed changes

kansi approved these changes Sep 14, 2018

View reviewed changes

kansi merged commit bd39076 into bigchaindb:master Sep 14, 2018


		self.abort_if_abci_chain_is_not_synced()

		# Check if BigchainDB supports the Tendermint version

Conversation

muawiakh commented Sep 13, 2018

Solution

Issues Resolved

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kansi left a comment

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ttmc left a comment

Choose a reason for hiding this comment

Uh oh!

ttmc Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ttmc Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ttmc Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ttmc Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldmberman Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Sep 13, 2018 •

edited

Loading

ldmberman Sep 13, 2018 •

edited

Loading

ldmberman Sep 14, 2018 •

edited

Loading

ldmberman Sep 14, 2018 •

edited

Loading