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

Docker compose for travis #2039

Merged
merged 3 commits into from
Feb 14, 2018

Conversation

sbellem
Copy link
Contributor

@sbellem sbellem commented Feb 12, 2018

Resolves #2038

@sbellem sbellem force-pushed the docker-compose-for-travis branch 4 times, most recently from c38510f to 16c9bea Compare February 13, 2018 11:09
if [[ -z ${TOXENV} ]]; then
docker pull python:3.6
docker pull tendermint/tendermint:0.13
docker pull mongo:3.4.3
Copy link
Contributor

@vrde vrde Feb 13, 2018

Choose a reason for hiding this comment

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

Why pulling images here? I thought this was already covered by the docker-compose file. If we can pin dependencies only in one place it's better, so we have less things to do if we update the version of any of those images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, sure. I added this when I was troubleshooting to separate more clearly the different steps. Will take care of it.


@pytest.fixture
def tendermint_ws_url(tendermint_host, tendermint_port):
return f'ws://{tendermint_host}:{tendermint_port}/websocket'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I didn't know about f strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to @kansi for using them first!

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful about using f strings for now. ReadTheDocs is failing to build the docs in the tendermint branch and I think it's because it uses an older version of Python 3.x that doesn't understand f strings. Not sure. But I took a look at the ReadTheDocs error logs for building the tendermint branch, and it was saying syntax error on a line with an f string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use the build option of readthedocs (http://docs.readthedocs.io/en/latest/yaml-config.html#build-image) to use Python 3.6.

msg_data_dict = json.loads(msg.data)
assert msg_data_dict['id'] == stream_id
assert msg_data_dict['jsonrpc'] == '2.0'
# TODO What else should be there? Right now, getting error.
Copy link
Contributor

@kansi kansi Feb 13, 2018

Choose a reason for hiding this comment

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

This seems enough for now.
But a more rigrous test would post a transaction to tendermint, once the transaction is commited we should assert that an update was received in the event stream for that transaction.

BIGCHAINDB_WSSERVER_HOST: 0.0.0.0
BIGCHAINDB_START_TENDERMINT: 0
TENDERMINT_HOST: tendermint
TENDERMINT_PORT: 46657
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind to use BIGCHAINDB_TENDERMINT_HOST and BIGCHAINDB_TENDERMINT_PORT? In this way the env_config function can pick it up easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sure. But we should address this in a different PR as some code in multiple places will need to be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, we can have it right after this one.

@vrde vrde merged commit 6e3f9e8 into bigchaindb:tendermint Feb 14, 2018
vrde added a commit to vrde/bigchaindb that referenced this pull request Feb 14, 2018
Solution: add `BIGCHAINDB_` to new tendermint vars. Reference:
bigchaindb#2039 (comment)
sbellem pushed a commit that referenced this pull request Feb 14, 2018
Solution: add `BIGCHAINDB_` to new tendermint vars. Reference:
#2039 (comment)
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.

None yet

4 participants