Skip to content

Add support for /v2/events stream#91

Merged
solarkennedy merged 22 commits intothefactory:masterfrom
nuclon:master
Apr 18, 2016
Merged

Add support for /v2/events stream#91
solarkennedy merged 22 commits intothefactory:masterfrom
nuclon:master

Conversation

@nuclon
Copy link
Copy Markdown

@nuclon nuclon commented Apr 14, 2016

The PR allows to use event stream from Marathon REST API added in v0.9.0

@solarkennedy
Copy link
Copy Markdown
Contributor

Hmm. This is kinda new and different. Can you add an itest for it?
Also setup.py probably needs to change.

@solarkennedy
Copy link
Copy Markdown
Contributor

Thank you. Long term, if you need this feature to continue to work as marathon evolves, it needs to have a section in the actual itest code that actually tries to read the events and maybe asserts that there is like, something?

But this technically isn't a blocker to merging.

My last request is that you rebase, I merged in a bunch of pep8 fixes.

@nuclon
Copy link
Copy Markdown
Author

nuclon commented Apr 16, 2016

Hi,
I've added actual itest code. I take some time to make it working on mac, but it works now.

@solarkennedy
Copy link
Copy Markdown
Contributor

Ok, the last thing to do is to bring it up to pep8 compliance. Check out the flake8 issues in an example travis run: https://travis-ci.org/thefactory/marathon-python/jobs/123588467

Or run tox -e pep8

@nuclon
Copy link
Copy Markdown
Author

nuclon commented Apr 16, 2016

Tests are failing on marathon waiting timeout.

@nuclon
Copy link
Copy Markdown
Author

nuclon commented Apr 18, 2016

It looks like the feature isn't really supported till 0.11.0 due to d2iq-archive/marathon#1926
so I updated tests to run starting from 0.11.0 and above

@solarkennedy
Copy link
Copy Markdown
Contributor

This looks great to me. Thank you very much for iterating through the tests and pep8 and all the things. Thanks!

@solarkennedy solarkennedy merged commit 4d9b7ab into thefactory:master Apr 18, 2016
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.

3 participants