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

Add SlaveID field in Task struct #140

Merged
merged 1 commit into from
Apr 15, 2016

Conversation

ohmystack
Copy link
Contributor

Marathon has a "slaveId" field in "task" since v0.11.0-RC1

@ohmystack
Copy link
Contributor Author

Sorry. I'll pull the latest master and fix the conflict.

@ohmystack
Copy link
Contributor Author

Fixed.

@gambol99 @timoreimann @bobrik, would you mind reviewing?

@timoreimann timoreimann self-assigned this Apr 14, 2016
@timoreimann
Copy link
Collaborator

We don't seem to use the majority of the message fields defined in the YML spec. In fact, I can remove the slaveId properties (and many others) without any tests turning red.

IMHO, we need tests that make sure our deserialization is working correctly. Along the way, we can probably clean up a lot of YML code.

Thus, my suggestion would be to not include the slaveId properties in the YML file, merge the PR, and create an issue to improve deserialization testing separately. @gambol99, WDYT?

Apart from this open question, the PR LGTM. 👍

Marathon has a "slaveId" field in "task" since v0.11.0-RC1
@ohmystack
Copy link
Contributor Author

@timoreimann That makes sense. In the new change, I've removed the slaveId in YML file.

@timoreimann
Copy link
Collaborator

Thanks, appreciate the contribution!

@timoreimann timoreimann merged commit 8953f68 into gambol99:master Apr 15, 2016
@ohmystack
Copy link
Contributor Author

@timoreimann 😀 Thanks to you

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.

2 participants